[Bug 3405] clientloop's client_wait_until_can_do_something uses 100 % CPU with ssh 2> >({exec 1>&2})

[bugzilla-daemon at mindrot.org]

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
             Status|NEW                         |ASSIGNED

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
Created attachment 3581
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3581&action=edit
avoid polling fds we're not ready to service events on

I did get it to reproduce, I was just holding it wrong.

Thanks for the report and the reproduction instructions. Here's my
analysis and a fix is attached:

> I think the sequence of operations is something like:
> 
> 1. Channel opens, stderr is attached
> 2. Stderr goes away
> 3. We poll with pfd[stderr].events including POLLOUT
> 4. We get back POLLHUP (POLLERR on Linux)
> 5. channel_handle_efd_write() sees sshbuf_len(c->extended)==0, returns
> 6. GOTO 3 forever
> 
> One problem is that there is no way to propagate a POLLHUP condition
> back to a writable channel when there is no output pending.
> 
> A more fundamental problem is step 3, where we unconditionally included
> the fd in the poll array, regardless of whether the channel code had any
> intention of attempting a write() later. When I was doing the conversion
> from select(), I kept the fds in there because it made the matching of
> pfd entries and channels easier but this was probably a mistake.
> 
> The patch corrects the mistake (hopefully). It will only
> set up a pollfd entry if IO was requested for the fd, and as a
> consequence, should avoid spurious events. It also forced me to make
> the pollfd bookkeeping less brittle :)
> 
> PS. an alternate approach would be to leave the pollfd entries in the
> array and find a way to handle POLLERR/POLLHUP events. I have a diff
> that implements this approach too, by treating such a condition on
> a channel that wasn't ready to write something as an automatic write
> failure. I shelved this in favour of this approach.

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