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

Restore tty I/O attributes when daemonizing #922

Closed
wants to merge 1 commit into from

Conversation

dancek
Copy link

@dancek dancek commented Jan 4, 2024

Fixes #909, ie. when sudo has the option use_pty enabled, running sshuttle --daemon would previously leave the terminal in a weird state.

Fixes sshuttle#909, ie. when sudo has the option use_pty enabled, running
sshuttle --daemon would previously leave the terminal in a weird state.
@dancek
Copy link
Author

dancek commented Jan 4, 2024

I don't fully understand why the daemonize function overwrites stdin and stdout with a null descriptor. And overall my understanding of the process hierarchy in sshuttle is limited. So this may not be the perfect place to restore terminal attributes, but it does work in my experience.

To replicate the issue, run with --daemon and check stty output afterwards. Sudo needs to have use_pty enabled for the issue to happen, ie. Defaults use_pty in /etc/sudoers or a recent version of sudo.

Before running the program:

$ stty
speed 38400 baud; line = 0;

Without this commit:

$ ./run --daemon [my-opts]
$ stty
speed 38400 baud; line = 0;
                           min = 1; time = 0;
                                             -brkint -icrnl
                                                           -opost
                                                                 -isig -icanon -iexten -echo

With this commit:

$ ./run --daemon [my-opts]
$ stty
speed 38400 baud; line = 0;

@skuhl
Copy link
Contributor

skuhl commented Jan 4, 2024

I took your code here and did some hunting to pinpoint when the terminal settings were getting garbled. It seems to be related to the socketpair that the firewall and client process use to communicate. I think I found an alternative fix that changes very few lines of code, avoids the garbled terminal in the first place, and fixes something that bothered me in the original fix for the use_pty change: That we had to use \r\n instead of \n at the end of messages to prevent verbose messages from being garbled (even when not in daemon mode). I'll try to post it a bit later today.

skuhl added a commit to skuhl/sshuttle that referenced this pull request Jan 4, 2024
This fixes sshuttle#909 and is an alternative to the sshuttle#922 pull request. When
sudo's use_pty is used with sshuttle, it causes issues with the
terminal. Pull request sshuttle#712 contains some fixes for this problem.
However, when sshuttle is run with the --daemon option, it left the
user's terminal in a non-sane state. The problem appears to be related
to a socketpair that the firewall uses for communication. By setting
it up slightly differently (see changes to client.py and firewall.py),
the terminal state is no longer disrupted. This commit also changes
line endings of the printed messages from \r\n to \n. This undoes a
change introduced by pull request sshuttle#712 and is no longer needed.
@dancek
Copy link
Author

dancek commented Jan 5, 2024

This is obsoleted by #923, which fixes the same issue by removing the root cause.

@dancek dancek closed this Jan 5, 2024
brianmay pushed a commit that referenced this pull request Jan 5, 2024
This fixes #909 and is an alternative to the #922 pull request. When
sudo's use_pty is used with sshuttle, it causes issues with the
terminal. Pull request #712 contains some fixes for this problem.
However, when sshuttle is run with the --daemon option, it left the
user's terminal in a non-sane state. The problem appears to be related
to a socketpair that the firewall uses for communication. By setting
it up slightly differently (see changes to client.py and firewall.py),
the terminal state is no longer disrupted. This commit also changes
line endings of the printed messages from \r\n to \n. This undoes a
change introduced by pull request #712 and is no longer needed.
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

Successfully merging this pull request may close these issues.

Terminal broken with python3.12
2 participants