Re: Potential handle leaks in dup_worker

[Ken Brown <kbrown@cornell.edu>]

On 2/9/2021 12:13 PM, Ken Brown via Cygwin-developers wrote:
> On 2/9/2021 11:12 AM, Corinna Vinschen via Cygwin-developers wrote:
>> On Feb  9 10:31, Ken Brown via Cygwin-developers wrote:
>>> On 2/9/2021 10:02 AM, Corinna Vinschen via Cygwin-developers wrote:
>>>> On Feb  9 09:19, Ken Brown via Cygwin-developers wrote:
>>>>> On 2/9/2021 4:47 AM, Corinna Vinschen via Cygwin-developers wrote:
>>>>>> On Feb  8 12:39, Ken Brown via Cygwin-developers wrote:
>>>>>>> I've had occasion to work through dtable::dup_worker, and I'm seeing the
>>>>>>> potential for leaks of path_conv handles.  I haven't seen any evidence that
>>>>>>> the leaks actually occur, but the code should probably be cleaned up if I'm
>>>>>>> right.
>>>>>>>
>>>>>>> dup_worker calls clone to create newfh from oldfh.  clone calls copyto,
>>>>>>> which calls operator=, which calls path_conv::operator=, which duplicates
>>>>>>> the path_conv handle from oldfh to newfh.  Then copyto calls reset, which
>>>>>>> calls path_conv::operator<<, which again duplicates the path_conv handle
>>>>>>> from oldfh to newfh without first closing the previous one.  That's the
>>>>>>> first leak.
>>>>>>>
>>>>>>> Further on, dup_worker calls newfh->pc.reset_conv_handle (), which sets the
>>>>>>> path_conv handle of newfh to NULL without closing the existing handle.  So
>>>>>>> that's a second leak.  This one is easily fixed by calling close_conv_handle
>>>>>>> instead of reset_conv_handle.
>>>>>>
>>>>>> Nice detective work, you're right.  For fun, this is easily testable.
>>>>>> Apply this patch to Cygwin:
>>>>>> [...]
>>>>>>> As a practical matter, I think the path_conv handle of oldfh is always NULL
>>>>>>> when dup_worker is called, so there's no actual leak.
>>>>>>
>>>>>> Right, because conv_handle should only be non-NULL in calls to stat(2)
>>>>>> and friends.
>>>>>>
>>>>>> Nevertheless, it's a bad idea to keep this code.  So the question is
>>>>>> this:  Do we actually *need* to duplicate the conv_handle at all?
>>>>>> It doesn't look like this is ever needed.  Perhaps the code should
>>>>>> just never duplicate conv_handle and just always reset it to NULL
>>>>>> instead?
>>>>>
>>>>> I've come across one place where I think it's needed.  Suppose build_fh_name
>>>>> is called with PC_KEEP_HANDLE.  It calls build_fh_pc, which calls set_name,
>>>>> which calls path_conv::operator<<.  I think we need to duplicate conv_handle
>>>>> here.
>>>>
>>>> Indeed, you're right.  I just found that the fhandler_base::reset method
>>>> is only called from copyto.  Given that fhandler::operator= already
>>>> calls path_conv::operator=, and that duplicates the conv handle, why
>>>> call path_conv::operator<< from fhandler_base::reset at all?  It looks
>>>> like this is only duplicating what already has been done.
>>>
>>> I think that's right.  It looks like operator<< differs from operator= only
>>> in being careful not to overwrite an existing path.  So I can't see that it
>>> ever makes sense to call operator<< right after calling operator=.
>>
>> It might be helpful not only to move reset to a protected inline method,
>> but also to rename it, making entirely clear that this is just a copyto
>> helper and nothing else.  I. e., something like _copyto_reset_helper().
>>
>> Are you going to create the patch or shall I?
> 
> I'll do it.

Trying to make _copyto_reset_helper() protected leads to errors like this:

In file included from ../../../../newlib-cygwin/winsup/cygwin/spawn.cc:22:
../../../../newlib-cygwin/winsup/cygwin/fhandler.h: In member function ‘virtual 
void fhandler_pty_master::copyto(fhandler_base*)’:
../../../../newlib-cygwin/winsup/cygwin/fhandler.h:2461:30: error: ‘void 
fhandler_base::_copyto_reset_helper()’ is protected within this context
  2461 |     x->_copyto_reset_helper ();

As usual, my C++ knowledge is limited, but I guess the issue is that we're 
calling _copyto_reset_helper() on behalf of x.  As an experiment, I removed 
'x->', and the error message went away.  I don't fully understand this.  Do you 
see an easy way around it, or should I just leave _copyto_reset_helper() public?

Ken