openssh bugs - Apr 2022 - [Bug 3416] New: sshd freeze when build without HAVE_PPOLL

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

            Bug ID: 3416
           Summary: sshd freeze when build without HAVE_PPOLL
           Product: Portable OpenSSH
           Version: 8.9p1
          Hardware: Itanium
                OS: Other
            Status: NEW
          Severity: normal
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: yaroslav.kuzmin at vmssoftware.com

In openbsd-compat/bsd-poll.c file in ppoll() function.

After calling pselect() and converting fd_set (readfds, writefds,
exceptfds) into struct  pollfd , there is no check whether this event
was requested or not.

     if (FD_ISSET(fd, readfds))
         fds[i].revents |= POLLIN;
     if (FD_ISSET(fd, writefds))
         fds[i].revents |= POLLOUT;
     if (FD_ISSET(fd, exceptfds))
         fds[i].revents |= POLLPRI;

this causes an unsolicited event to be handled

it's better to check whether this event was requested or not

     if (FD_ISSET(fd, readfds))
       if (fds[i].events & POLLIN)
         fds[i].revents |= POLLIN;
     if (FD_ISSET(fd, writefds))
       if (fds[i].events & POLLOUT)
         fds[i].revents |= POLLOUT;
     if (FD_ISSET(fd, exceptfds))
       if (fds[i].events & POLLPRI)
         fds[i].revents |= POLLPRI;

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

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

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |3395
                 CC|                            |dtucker at dtucker.net

--- Comment #1 from Darren Tucker <dtucker at dtucker.net> ---
Fair enough.


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=3395
[Bug 3395] Tracking bug for openssh-9.0
-- 
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=3416

--- Comment #2 from Darren Tucker <dtucker at dtucker.net> ---
Created attachment 3586
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3586&action=edit
Check that returned events were actually requestd

I collapsed the checks onto the same line and put the cheaper check
first.

Does this also resolve the problem?

-- 
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=3416

--- Comment #3 from Yaroslav <yaroslav.kuzmin at vmssoftware.com> ---
yes it resolve the problem

-- 
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=3416

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org
   Attachment #3586|                            |ok+
              Flags|                            |

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 3586
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3586
Check that returned events were actually requestd

Looks good to me. Although unrelated, I think the POLLIN case should
check (revents & (POLLIN|POLLHUP)) as this can be returned too (not
sure whether without POLLIN too).

Also maybe both POLLIN and POLLOUT cases should check POLLERR in
revents. I saw this in bug #3405

-- 
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=3416

--- Comment #5 from Darren Tucker <dtucker at dtucker.net> ---
(In reply to Damien Miller from comment #4)
> Looks good to me. Although unrelated, I think the POLLIN case should
> check (revents & (POLLIN|POLLHUP)) as this can be returned too (not
> sure whether without POLLIN too).
I don't follow: it's not checking revents that will be returned from
poll(), it's checking events that were passed to poll().  According to
the man page POLLHUP is meaningless and ignored in events and only
meaningful in revents (which we're not checking).
> Also maybe both POLLIN and POLLOUT cases should check POLLERR in
> revents. I saw this in bug #3405
Good point, I'll do that as a separate patch.

-- 
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=3416

--- Comment #6 from Darren Tucker <dtucker at dtucker.net> ---
(In reply to Darren Tucker from comment #5)
[...]
> > Also maybe both POLLIN and POLLOUT cases should check POLLERR in
> > revents. I saw this in bug #3405
> 
> Good point, I'll do that as a separate patch.
I take that back :-)

Same thing, we are not checking the return from a poll(), we are
checking the return from a select() and mapping it to what would be
returned by poll().  Conditions that would cause poll() to set POLLERR
will cause select to set the bit in readfds or writefds (likely both),
but we can't tell that from what select returns.  We won't see
there's
an error condition until the calling code attempts a read or write and
gets a 0 or -1 returned.

-- 
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=3416

Darren Tucker <dtucker at dtucker.net> changed:

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

--- Comment #7 from Darren Tucker <dtucker at dtucker.net> ---
Anyway the patch has been applied (both master and V_8_9) and will be
in the next release.  Thanks for the report.

-- 
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=3416

--- Comment #8 from Damien Miller <djm at mindrot.org> ---
Closing bugs from openssh-9.1 release cycle

-- 
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=3416

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
OpenSSH 9.3 has been released. Close resolved bugs

-- 
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.