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

lib/pty-session: Don't ignore SIGHUP. #2836

Closed
wants to merge 1 commit into from

Conversation

q2ven
Copy link
Contributor

@q2ven q2ven commented Mar 11, 2024

The blamed commit converted script to use the generic pty code that was added by commit 6954895 ("lib/pty-session: add generic PTY container code").

Commit ec10634 says the new pty stuff is based on script. However, there is difference in signal handling.

Before the commit, only the signals that the script was interested in were blocked and handled by signalfd.

After the commit, all signals are blocked and only the interested signals are handled.

This causes regression in the following scenario:

  1. run script via /etc/profile.d for ssh session
  2. login to a ssh session using bash
  3. run sudo -i
  4. wait until ssh session timeout

After 3., the process tree will be like this.

  $ pstree -p | grep script
  |-sshd(2652)-sshd(637993)---sshd(637996)---bash(637997)---script(638028)---bash(638029)---sudo(638057)---sudo(638059)-+

The notable thing here is that script is in the parent's process group (637997).

  $ ps -eo pid,pgid,ppid,stat,sig,comm | grep script
  638028  637997  637997 S+   0000000000000000 script

Thus, the parent bash will send SIGHUP to script, when the timeout occurs at 4.

  $ sudo /usr/share/bcc/tools/killsnoop
  TIME      PID      COMM             SIG  TPID     RESULT
  18:46:57  637997   bash             1    637997   0

  $ ps -eo pid,pgid,ppid,stat,sig,comm | grep script
  638028  637997       1 S    0000000000020001 script

However, this is not handled, so the script is reaped but remains forever.

  $ pstree -p | grep script
  |-script(638028)---bash(638029)

To avoid such a situation, we need to handle SIGHUP in script.

With this patch, script will receive SIGHUP from the parrent and kill its child processes.

  $ pstree -p | grep script
  |-sshd(2652)-sshd(638400)---sshd(638404)---bash(638405)---script(638436)---bash(638437)---sudo(638465)---sudo(638467)-+

  $ sudo /usr/share/bcc/tools/killsnoop
  TIME      PID      COMM             SIG  TPID     RESULT
  18:50:11  638405   bash             1    638405   0
  18:50:11  638436   script           15   638437   0
  18:50:11  638436   script           15   638437   0
  18:50:13  638436   script           9    638437   0

Fixes: ec10634 ("script: use lib/pty-session")
Reported-by: Ayame Suzuki ayameszk@amazon.co.jp

The blamed commit converted script to use the generic pty code that
was added by commit 6954895 ("lib/pty-session: add generic PTY
container code").

Commit ec10634 says the new pty stuff is based on script.
However, there is difference in signal handling.

Before the commit, only the signals that the script was interested
in were blocked and handled by signalfd.

After the commit, all signals are blocked and only the interested
signals are handled.

This causes regression in the following scenario:

  1. run `script` via /etc/profile.d for ssh session
  2. login to a ssh session using bash
  3. run `sudo -i`
  4. wait until ssh session timeout

After 3., the process tree will be like this.

  $ pstree -p | grep script
  |-sshd(2652)-sshd(637993)---sshd(637996)---bash(637997)---script(638028)---bash(638029)---sudo(638057)---sudo(638059)-+

The notable thing here is that script is in the parent's process
group (637997).

  $ ps -eo pid,pgid,ppid,stat,sig,comm | grep script
  638028  637997  637997 S+   0000000000000000 script

Thus, the parent bash will send SIGHUP to script, when the timeout
occurs at 4.

  $ sudo /usr/share/bcc/tools/killsnoop
  TIME      PID      COMM             SIG  TPID     RESULT
  18:46:57  637997   bash             1    637997   0

  $ ps -eo pid,pgid,ppid,stat,sig,comm | grep script
  638028  637997       1 S    0000000000020001 script

However, this is not handled, so the script is reaped but remains
forever.

  $ pstree -p | grep script
  |-script(638028)---bash(638029)

To avoid such a situation, we need to handle SIGHUP in script.

With this patch, script will receive SIGHUP from the parrent and
kill its child processes.

  $ pstree -p | grep script
  |-sshd(2652)-sshd(638400)---sshd(638404)---bash(638405)---script(638436)---bash(638437)---sudo(638465)---sudo(638467)-+

  $ sudo /usr/share/bcc/tools/killsnoop
  TIME      PID      COMM             SIG  TPID     RESULT
  18:50:11  638405   bash             1    638405   0
  18:50:11  638436   script           15   638437   0
  18:50:11  638436   script           15   638437   0
  18:50:13  638436   script           9    638437   0

Fixes: ec10634 ("script: use lib/pty-session")
Reported-by: Ayame Suzuki <ayameszk@amazon.co.jp>
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
@q2ven
Copy link
Contributor Author

q2ven commented Mar 19, 2024

Hi @t-8ch,
it would be appreciated if you take a look. Please let me know something unclear. Thanks in advance.

@q2ven
Copy link
Contributor Author

q2ven commented Mar 19, 2024

Hi @t-8ch,

ASAN test is failing in other PRs, so it seems the failure is not specific to this change.
If vm.mmap_rnd_bits sysctl knob is 32, could you give it another shot with 28 like this ?
h2o/quicly@4c3fa98

For details, please see this issue: google/sanitizers#1716

The util-linux CI is using 6.5 kernel,

        kernel: 6.5.0-1016-azure 

and llvm-project also faced the same issue on 6.5.
llvm/llvm-project#85013

@t-8ch
Copy link
Member

t-8ch commented Mar 19, 2024

#2847 tries to fix the ASAN issues.

@karelzak
Copy link
Collaborator

Merged as: 90ebced

@karelzak karelzak closed this Mar 20, 2024
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.

None yet

3 participants