Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSH is killed on Control-C detaching #203

Closed
totaam opened this issue Nov 2, 2012 · 20 comments
Closed

SSH is killed on Control-C detaching #203

totaam opened this issue Nov 2, 2012 · 20 comments

Comments

@totaam
Copy link
Collaborator

totaam commented Nov 2, 2012

Issue migrated from trac ticket # 203

component: core | priority: minor | resolution: fixed

2012-11-02 23:48:22: onlyjob created the issue


Please don't kill SSH when detaching on Ctrl+C signal.

SSH have a powerful feature to reuse existing connections.
For example if ~/.ssh/config have the following:

host *
    controlmaster auto
    controlpath ~/.ssh/__tmp-ssh-%r@%h:%p

then SSH will create a socket file for first connection to remote host and subsequent connections will reuse the existing connection through the sock file.

When you log off from first "master" session, SSH will not exit until all other connections are closed.
If you terminate "master" SSH process then all other connections to the same host will be terminated as well.

In Xpra when I attach to remote session without any prior ssh connections to the remote host a "master" connection is created.
If later I have other SSH connections to the same host they are reusing "master" SSH connection initiated by Xpra.

The problem is that all SSH connections are terminated if I detach from "master" session by sending "Control-C" to Xpra client because it kills master SSH connection.
This do not happen when I detach using "Disconnect" from Xpra client menu.

@totaam
Copy link
Collaborator Author

totaam commented Nov 3, 2012

2012-11-03 04:12:25: antoine changed status from new to accepted

@totaam
Copy link
Collaborator Author

totaam commented Nov 3, 2012

2012-11-03 04:12:25: antoine commented


ssh may currently receive the Ctrl+C, we would have to prevent that - and doing that would mean that we may make it more difficult to interact with ssh via the terminal (password input for example).

then we have to hook up the signal handler to fire the disconnect code.

do-able but tricky.

@totaam
Copy link
Collaborator Author

totaam commented Nov 3, 2012

2012-11-03 07:22:19: antoine commented


please try r2029 and let me know if that works for you

This is more complicated than it seems, and here are some useful pointers I found along the way:

@totaam
Copy link
Collaborator Author

totaam commented Nov 3, 2012

2012-11-03 10:57:09: onlyjob commented


Well done, this fixes the issue. Thank you.

@totaam
Copy link
Collaborator Author

totaam commented Nov 3, 2012

2012-11-03 10:58:28: antoine changed status from accepted to closed

@totaam
Copy link
Collaborator Author

totaam commented Nov 3, 2012

2012-11-03 10:58:28: antoine changed resolution from ** to fixed

@totaam
Copy link
Collaborator Author

totaam commented Mar 8, 2013

2013-03-08 01:52:07: onlyjob changed status from closed to reopened

@totaam
Copy link
Collaborator Author

totaam commented Mar 8, 2013

2013-03-08 01:52:07: onlyjob changed resolution from fixed to **

@totaam
Copy link
Collaborator Author

totaam commented Mar 8, 2013

2013-03-08 01:52:07: onlyjob commented


Xpra-0.8.8 is exhibit this problem again...

When two or more sessions are attached to the host, terminating first Xpra session with Ctrl+C terminates the other one(s) as well because it closes SSH control connection.

@totaam
Copy link
Collaborator Author

totaam commented Mar 8, 2013

2013-03-08 03:04:10: antoine commented


This was caused by r2762, see #260

This probably fixes it:

--- src/xpra/scripts/main.py	(revision 2907)
+++ src/xpra/scripts/main.py	(working copy)
@@ -556,6 +556,17 @@
                 ssh_fail_cb(error_message)
                 raise IOError(error_message)
         def stop_tunnel():
+            if os.name=="posix":
+                #on posix, the tunnel may be shared with other processes
+                #so don't kill it... which may leave it behind after use.
+                #but at least make sure we close all the pipes:
+                for name,fd in {"stdin" : child.stdin, "stdout" : child.stdout, "stderr" : child.stderr}.items():
+                    try:
+                        if fd:
+                            fd.close()
+                    except Exception, e:
+                        print("error closing ssh tunnel %s: %s" % (name, e))
+                return
             try:
                 if child.poll() is None:
                     #only supported on win32 since Python 2.7

Since terminating the ssh process may or may not be the right thing to do, depending on what platform you are on, how ssh is setup, personal preference, etc.. maybe this should even be an option?

@totaam
Copy link
Collaborator Author

totaam commented Mar 8, 2013

2013-03-08 03:43:40: antoine changed status from reopened to closed

@totaam
Copy link
Collaborator Author

totaam commented Mar 8, 2013

2013-03-08 03:43:40: antoine changed resolution from ** to fixed

@totaam
Copy link
Collaborator Author

totaam commented Mar 8, 2013

2013-03-08 03:43:40: antoine commented


applied in r2908, will backport to 0.8.x

closing, feel free to re-open if not fixed

@totaam
Copy link
Collaborator Author

totaam commented Jul 15, 2013

2013-07-15 05:55:58: antoine changed status from closed to reopened

@totaam
Copy link
Collaborator Author

totaam commented Jul 15, 2013

2013-07-15 05:55:58: antoine changed resolution from fixed to **

@totaam
Copy link
Collaborator Author

totaam commented Jul 15, 2013

2013-07-15 05:55:58: antoine commented


Unfortunately, this has caused a bad regression: this prevents us from using password mode with ssh, see #380

So unless a better solution can be found, I will remove this code.

@totaam
Copy link
Collaborator Author

totaam commented Mar 20, 2014

2014-03-20 07:45:38: totaam changed status from reopened to closed

@totaam
Copy link
Collaborator Author

totaam commented Mar 20, 2014

2014-03-20 07:45:38: totaam changed resolution from ** to fixed

@totaam
Copy link
Collaborator Author

totaam commented Mar 20, 2014

2014-03-20 07:45:38: totaam commented


Since it is impossible to have both, r5862 deals with this problem by giving the user the option of having keyboard interaction (--exit-ssh - which is now the default) or having connection sharing (--no-exit-ssh).

@totaam totaam closed this as completed Mar 20, 2014
@totaam
Copy link
Collaborator Author

totaam commented Jul 22, 2018

2018-07-22 09:26:33: antoine commented


Information added here: SSH.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant