Re: Fix leaky pipe

[Christopher Faylor <cgf-no-personal-reply-please at cygwin dot com>]

On Tue, May 30, 2006 at 02:19:14AM +0100, Dave Korn wrote:
>On 29 May 2006 23:01, Christopher Faylor wrote:
>>On Mon, May 29, 2006 at 09:48:50AM +0100, Dave Korn wrote:
>>>As discussed elsewhere, here's a patch that solves the race problem
>>>without leaking handles any more by placing the master-pipe-closing
>>>logic where it really belongs, in fhandler_tty_common::close where the
>>>send-an-EOF decision is made.  I figured 4 extra bytes in the vtable
>>>isn't too bad, it's not like you expect to have millions of terminals
>>>open at once.
>>
>>I don't know.  The more I think about this problem, the more I think
>>that the logic which you've exposed which deals with "inuse" in
>>fhandler_pty_master::close is wrong.
>>
>>It's been a while since I looked at this code (and I've always hated it
>>-- even after I sort of rewrote it) but for a pty, I don't see why it
>>should matter if there are still other things using the open master
>>handles.  I think that each parent pty should have its own copy of the
>>from_master/to_master handles which are unconditionally closed.  I'm
>>probably missing something, but I think the inuse handle for the master
>>part of the pty is probably not needed.
>
>Well, all this is entirely possible; I'm not even sufficiently familiar
>with how ptys are specified to work by the standard.  From what I
>understand of it, though, ISTM that having one set of master handles
>and using the reference count on an event (as manifested through the
>inuse handle to the master-alive event) to track users of them is
>basically equivalent to the way you suggest doing it there, with the
>cumulative reference count on the pipes imposed by the open handles
>representing the use count, and the inuse event omitted.  However all
>this is massively complicated by permutation vs.  close-on-exec and
>fork-and-dup, I don't want to make reckless predictions.

But, all of it is already done that way for the slave.

>>I probably will propose another way to handle this that will reorganize
>>the tty structure and the fhandler_[pt]tty classes.  OTOH, I haven't
>>given this as much thought as you have, so maybe I'll come around to
>>agreeing that your patch is the best way to go.
>>
>>Right now, however, this code is sending off little alarms that tell me
>>that something is wrong.  I tend to trust these alarms because they are
>>right 50% of the time.
>>
>>I know.  It's an amazing ability.
>>
>>Anyway, thanks again for the patch.  I hope to discuss this more
>>intelligently in the next couple of days.
>
>Well, I can't hear the alarm bells, but then I'm less familiar with the
>code.  This patch is less invasive and fixes the current problem; it's
>really just a revision of Pavel's patch which we all see clearly
>identified a real race condition.  If you want a low-risk option for
>the next release while you think about a serious refactoring, I reckon
>this would be a good band-aid.

The problem is that I think that the current implementation is really
flawed.  Why should it be making decisions about closing handles based
on whether something else currently is using the master?  If some other
process has the master open, that doesn't mean that the master handles
should be left open in this process.

Also, the code is just unilaterally closing the handles even though
it may not actually *own* them, which could end up closing the wrong
handle.  In practice this probably doesn't cause many problems but
it is really wrong.

cgf