Potential handle leaks in dup_worker

[Ken Brown]

On 2/9/2021 3:52 PM, Corinna Vinschen via Cygwin-developers wrote:
> On Feb  9 14:12, Ken Brown via Cygwin-developers wrote:
>> 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.
>>>>>>>> [...]
>>>>>>>> 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?
>>>>>>> I've come across one place where I think it's needed.
>>>>>>> [...]
>>>>>> 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().
>> [...]
>> 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?
> 
> There's no easy way as such.  I think the right approach is to turn the
> copy-to method into a copy-from method, modifying "this", rather than
> another object given as parameter.  This is a more object-oriented
> approach, IMHO.  This also automagically fixes the problem that a
> protected method can't be called in this context.
> 
> Here's a patch fixing it this way.  Can you please review it and see
> if I missed something?
> 
> Actually, on second thought, this should be split into two patches, one
> removing the call to path_conv::operator<<, and the other one
> reshuffling the code.  If the patch is ok, I'll do that before pushing
> it.

LGTM.  I agree that this is a better approach.  My only other suggestion is that 
you consider removing path_conv::reset_conv_handle and replacing its two uses by 
close_conv_handle.  Again this is to avoid the appearance of a handle leak, even 
though I'm pretty sure that the handle is always NULL when this is called.

Ken