openssh bugs - Sep 2016 - [Bug 2619] New: infinite loop, 100% cpu use in ssh if ^Z is pressed at password prompt

https://bugzilla.mindrot.org/show_bug.cgi?id=2619

            Bug ID: 2619
           Summary: infinite loop, 100% cpu use in ssh if ^Z is pressed at
                    password prompt
           Product: Portable OpenSSH
           Version: 7.3p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com

Steps to reproduce:

1. either change or create a test login with /bin/sh as
   login shell (should work regarding shell)
2. "ssh user at localhost" and login
3. "sftp user at localhost" and press ^Z in the password
   prompt

A few times it will work. It depends a bit on what code
is being executed in the readpassphrase function, at
openbsd-compat/readpassphrase.c in the openssh code.

It has been verified that exec'ing again /bin/sh with
--posix before running sftp, or exporting the environment
variable POSIXLY_CORRECT before the "ssh user at localhost"
step prevents the problem.

So, while the problem appears to be kind of expected, it
is being reported in case it was not meant to happen.

Originally reported in Red Hat bugzilla [1]. This is rare race
condition, where we stuck in the code openbsd-compat/readpassphrase.c:

        while (tcsetattr(input, _T_FLUSH, &oterm) == -1 &&
            errno == EINTR)
            continue;

but it should not happen. The possible solution would be to check the
signal received in the handler, but I am not sure about most
appropriate fallback when the above fails.

More verbose analysis is in the above mentioned bug.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1218424

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2619

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au

--- Comment #1 from Darren Tucker <dtucker at zip.com.au> ---
It looks like a subsequent change in the OpenBSD upstream source
function addresses this:

revision 1.23
date: 2010/05/14 13:30:34;  author: millert;  state: Exp;  lines: +41
-39;
Defer installing signal handlers until echo is disabled so that we
get suspended normally when not the foreground process.  Fix potential
infinite loop when restoring terminal settings if process is in the
background when restore occurs.  OK miod@
----------------------------

(the portable one is based on 1.22).  I'll look at porting these
changes.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2619

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2594
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #2 from Darren Tucker <dtucker at zip.com.au> ---
I've imported the change:
https://anongit.mindrot.org/openssh.git/commit/?id=12069e56221de207ed666c2449dedb431a2a7ca2

which should fix the problem (it includes code equivalent to the
"signo[SIGTTOU] != 1" you suggested in the redhat bug).

Thanks for the report.


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2594
[Bug 2594] Tracking bug for OpenSSH 7.4 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2619

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED

--- Comment #3 from Darren Tucker <dtucker at zip.com.au> ---
Based on the comments in the redhat bug this is not entirely fixed on
Linux at least.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2619

--- Comment #4 from Darren Tucker <dtucker at zip.com.au> ---
Created attachment 2879
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2879&action=edit
readpassphrase: mask off signals while we resend them.

The new problem is that during the "resend all the signals we got to
ourselves" part, the kill syscall hangs.

The relevant part of IEEE Std 1003.1-2008 spec for kill(2) is:

"""
If the value of pid causes sig to be generated for the sending process,
and if sig is not blocked for the calling thread and if no other thread
has sig unblocked or is waiting in a sigwait() function for sig, either
sig or at least one pending unblocked signal shall be delivered to the
sending thread before kill() returns.
"""

if we avoid this by blocking the signals we're re-sending while we send
them this should work around that problem (and seems to in my limited
testing).

jjelen: if you can confirm this solves the problem for you then I'll
look at upstreaming it.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2619

--- Comment #5 from Darren Tucker <dtucker at zip.com.au> ---
(In reply to Darren Tucker from comment #4)
> The new problem is that during the "resend all the signals we got to
> ourselves" part, the kill syscall hangs.
Actually I don't think it's the syscall itself that is hanging, so I
think that part of the man page is a red herring.

Here's an strace:

read(5, 0xbf88cb5b, 1)                  = ? ERESTARTSYS (To be
restarted if SA_RESTART is set)
--- SIGTSTP {si_signo=SIGTSTP, si_code=SI_KERNEL} ---
sigreturn({mask=[]})                    = -1 EINTR (Interrupted system
call)
write(5, "\n", 1)                       = 1
ioctl(5, SNDCTL_TMR_CONTINUE or TCSETSF, {B38400 opost isig icanon echo
...}) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
--- SIGTTOU {si_signo=SIGTTOU, si_code=SI_KERNEL} ---
sigreturn({mask=[]})                    = -1 EINTR (Interrupted system
call)
rt_sigaction(SIGALRM, {SIG_DFL, [], 0}, NULL, 8) = 0
rt_sigaction(SIGHUP, {SIG_DFL, [], 0}, NULL, 8) = 0
rt_sigaction(SIGINT, {SIG_IGN, [], 0}, NULL, 8) = 0
rt_sigaction(SIGQUIT, {SIG_DFL, [], 0}, NULL, 8) = 0
rt_sigaction(SIGPIPE, {SIG_IGN, [], 0}, NULL, 8) = 0
rt_sigaction(SIGTERM, {SIG_DFL, [], 0}, NULL, 8) = 0
rt_sigaction(SIGTSTP, {SIG_DFL, [], 0}, NULL, 8) = 0
rt_sigaction(SIGTTIN, {SIG_DFL, [], 0}, NULL, 8) = 0
rt_sigaction(SIGTTOU, {SIG_DFL, [], 0}, NULL, 8) = 0
close(5)                                = 0
kill(28963, SIGTSTP)                    = 0
--- SIGTSTP {si_signo=SIGTSTP, si_code=SI_USER, si_pid=28963,
si_uid=500} ---
--- stopped by SIGTSTP ---
--- SIGCONT {si_signo=SIGCONT, si_code=SI_USER, si_pid=28503,
si_uid=500} ---
kill(28963, SIGTTOU)                    = 0
--- SIGTTOU {si_signo=SIGTTOU, si_code=SI_USER, si_pid=28963,
si_uid=500} ---
--- stopped by SIGTTOU ---
[hung here]

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2619

--- Comment #6 from Jakub Jelen <jjelen at redhat.com> ---
I verified that the process with the last patch does not hang anymore
in our use case.

According to your strace, if I understand it well, it looks like the
SIGTTOU signal is re-send and default handler stops the process (looks
like hang). This looks like ... well ... defined behavior, though not
what we expect from ssh.

AFAIK, we should just not resend the SIGTTOU as we already ignore it on
the line 161 (probably together with the other stopping signals).
Something like this did solve the problem for me too. But insight from
somebody more experienced would we helpful.

diff --git a/openbsd-compat/readpassphrase.c
b/openbsd-compat/readpassphrase.c
index c99b4e2..fade1aa 100644
--- a/openbsd-compat/readpassphrase.c
+++ b/openbsd-compat/readpassphrase.c
@@ -179,13 +179,14 @@ restart:
         */
        for (i = 0; i < _NSIG; i++) {
                if (signo[i]) {
-                       kill(getpid(), i);
                        switch (i) {
                        case SIGTSTP:
                        case SIGTTIN:
                        case SIGTTOU:
                                need_restart = 1;
+                               continue;
                        }
+                       kill(getpid(), i);
                }
        }
        if (need_restart)

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2619

--- Comment #7 from Darren Tucker <dtucker at zip.com.au> ---
(In reply to Jakub Jelen from comment #6)
> I verified that the process with the last patch does not hang
> anymore in our use case.
> 
> According to your strace, if I understand it well, it looks like the
> SIGTTOU signal is re-send and default handler stops the process
> (looks like hang). This looks like ... well ... defined behavior
That's only true for the SIG_DFL action.  If there's a non-default
handler then it could do anything else.  I'd like to see the general
case fixed and upstreamed rather than doing something specific to
openssh.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2619

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2879|0                           |1
        is obsolete|                            |

--- Comment #8 from Darren Tucker <dtucker at zip.com.au> ---
Created attachment 2881
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2881&action=edit
Do not attempt to re-send SIGTTOU if we got it while restoring terminal
modes.

Todd Miller of OpenBSD came up with the following analysis:

"""
It's not really useful to generate SIGTTOU when restoring terminal
the mode in readpassphrase().  If we get SIGTTOU it means the process
is not in the foreground process group which, in most cases, means
that the shell has taken possession of the tty.

Basically what happens is this:

1) sftp forks ssh
2) user suspends sftp (really ssh) at the password prompt
3) sftp receives SIGTSTP and suspends
4) shell takes the controlling terminal
5) ssh receives SIGTSTP, tries to restore terminal mode, receives
SIGTTOU
6) ssh sends itself SIGTSTP and suspends
7) user resumes sftp (and the ssh child)
8) ssh sends itself SIGTTOU and suspends
9) sftp appears to be hung (though you can bg and fg it to get back)

sftp should probably install signal handlers for SIGTSTP, SIGTTOU
and SIGTTIN and wait for its child to suspend like scp does but
I'll save that for another diff.
"""

and the attached patch, which also seems to work for me.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2619

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #9 from Darren Tucker <dtucker at zip.com.au> ---
The last patch has been applied:
https://anongit.mindrot.org/openssh.git/commit/?id=8f866d8a57b9a2dc5dd04504e27f593b551618e3

plus another patch which will cause sftp to pass the signal through to
the ssh process so that it can handle it cleanly (scp already does
this):
https://anongit.mindrot.org/openssh.git/commit/?id=2c6697c443d2c9c908260eed73eb9143223e3ec9

Hopefully this should now be fully sorted!

Thanks.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2619

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 release

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.