Bug 3416 - sshd freeze when build without HAVE_PPOLL
Summary: sshd freeze when build without HAVE_PPOLL
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: 8.9p1
Hardware: Itanium Other
: P5 normal
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks: V_9_0
  Show dependency treegraph
 
Reported: 2022-04-01 16:18 AEDT by Yaroslav
Modified: 2023-03-17 13:40 AEDT (History)
2 users (show)

See Also:


Attachments
Check that returned events were actually requestd (677 bytes, patch)
2022-04-01 16:42 AEDT, Darren Tucker
djm: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yaroslav 2022-04-01 16:18:46 AEDT
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;
Comment 1 Darren Tucker 2022-04-01 16:41:03 AEDT
Fair enough.
Comment 2 Darren Tucker 2022-04-01 16:42:36 AEDT
Created attachment 3586 [details]
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?
Comment 3 Yaroslav 2022-04-01 17:18:01 AEDT
yes it resolve the problem
Comment 4 Damien Miller 2022-04-01 18:30:33 AEDT
Comment on attachment 3586 [details]
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
Comment 5 Darren Tucker 2022-04-01 23:38:22 AEDT
(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.
Comment 6 Darren Tucker 2022-04-01 23:57:33 AEDT
(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.
Comment 7 Darren Tucker 2022-04-01 23:59:58 AEDT
Anyway the patch has been applied (both master and V_8_9) and will be in the next release.  Thanks for the report.
Comment 8 Damien Miller 2022-10-04 21:59:25 AEDT
Closing bugs from openssh-9.1 release cycle
Comment 9 Damien Miller 2023-03-17 13:40:05 AEDT
OpenSSH 9.3 has been released. Close resolved bugs