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

Interrupting the text UI terminates the child process for ssh too early #812

Closed
tleedjarv opened this issue Oct 27, 2022 · 3 comments · Fixed by #810
Closed

Interrupting the text UI terminates the child process for ssh too early #812

tleedjarv opened this issue Oct 27, 2022 · 3 comments · Fixed by #810
Labels
defect unison fails to meet its specification (but doesn't crash; see also "crash") effort-medium issue is likely resolvable with <= 20h of effort impact-low low importance

Comments

@tleedjarv
Copy link
Contributor

When interrupting the text UI with Ctrl-C (or any ^C/break in terminal input) then the generated SIGINT will terminate ssh before unison manages to do cleanup.

The issue is that unison will execute ssh as a child process. If unison is executed from a terminal then the ssh child process will be in the same process group and using the same terminal (the same session). A SIGINT generated by the terminal is sent to all processes in the same process group. The end result is that the ssh process dies before unison manages to do the cleanup with the server. You can witness this by pressing Ctrl-C (with unison in the foreground) and then receiving an error "Failed due to lost connection".

I'm not sure what the solution to this is. Creating a new process group for the child process before exec'ing ssh is an option. Even if this works, I wonder how portable it is (it's POSIX, but still) and whether this will cause endless unforeseen side-effects. Creating a new pty like is done for ssh by the GUI is another option and probably a better one. This could also work in Windows.
Edit: Just remembered that executing ssh in a different process group wouldn't work because ssh may need input from the terminal.

Originally posted by @tleedjarv in #810 (comment)

@tleedjarv
Copy link
Contributor Author

Comment by gdt:

I am not at all sure that pty is better than pgrp, if pgrp is POSIX-specified and ptys are not.

They're both in POSIX. I think ptys are widely supported, either by POSIX interface, pre-POSIX SysV interface or BSD interface. Unlike pgrp, also recent Windows has ptys.

The problem with pgrp-based solution is that if ssh has a different pgrp then it is considered a background process. Background processes don't receive the terminal-generated SIGINT (good!). They also can't read terminal input (would receive SIGTTIN or if blocking/ignoring that then EIO). ssh must do user interaction (interactive auth, host verification, ...) on the terminal, not stdin/stdout (which are forwarded to/from the remote end).

@tleedjarv
Copy link
Contributor Author

As it often happens, the most obvious and simple solutions end up being the correct and best ones. I had initially discarded the idea of blocking or ignoring SIGINT before spawning the child process (signal masks and handlers are inherited by the child) because I assumed that ssh would override the masks and handlers anyway. It turns out that at least OpenSSH doesn't. So blocking and ignoring both work. While ssh does set a handler for SIGINT, it does not do so if the signal is already being ignored. The devil is in the details and there are some caveats, as always, but in general this approach seems to fix this issue on POSIX-y platforms.

@tleedjarv
Copy link
Contributor Author

Other than making use of a pty, I don't think a solution for Windows is even possible. I'm going to close this ticket with a POSIX-only fix.

@gdt gdt added defect unison fails to meet its specification (but doesn't crash; see also "crash") impact-low low importance effort-medium issue is likely resolvable with <= 20h of effort labels Nov 7, 2022
@gdt gdt closed this as completed in #810 Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect unison fails to meet its specification (but doesn't crash; see also "crash") effort-medium issue is likely resolvable with <= 20h of effort impact-low low importance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants