Potential handle leaks in dup_worker

Potential handle leaks in dup_worker

[Ken Brown]

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.

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.

I may well be confused and/or missing something.

Ken

Re: Potential handle leaks in dup_worker

[Corinna Vinschen]

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:

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 52a020f07d5e..58e993b66c42 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1475,6 +1475,10 @@ open (const char *unix_path, int flags, ...)
       int opt = PC_OPEN | PC_SYM_NOFOLLOW_PROCFD;
       opt |= (flags & (O_NOFOLLOW | O_EXCL)) ? PC_SYM_NOFOLLOW
 					     : PC_SYM_FOLLOW;
+
+      if (flags & O_NOATIME)
+      	opt |= PC_KEEP_HANDLE;
+
       /* This is a temporary kludge until all utilities can catch up
 	 with a change in behavior that implements linux functionality:
 	 opening a tty should not automatically cause it to become the

And then create an STC like this:

  #define _GNU_SOURCE 1
  #include <stdio.h>
  #include <unistd.h>
  #include <fcntl.h>

  int
  main (int argc, char **argv)
  {
    int fd, fd2;

    fd = open (argv[1], O_RDONLY | O_NOATIME);
    dup (fd);
  }

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


Thanks,
Corinna

Re: Potential handle leaks in dup_worker

[Ken Brown]

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:
> 
> diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
> index 52a020f07d5e..58e993b66c42 100644
> --- a/winsup/cygwin/syscalls.cc
> +++ b/winsup/cygwin/syscalls.cc
> @@ -1475,6 +1475,10 @@ open (const char *unix_path, int flags, ...)
>         int opt = PC_OPEN | PC_SYM_NOFOLLOW_PROCFD;
>         opt |= (flags & (O_NOFOLLOW | O_EXCL)) ? PC_SYM_NOFOLLOW
>   					     : PC_SYM_FOLLOW;
> +
> +      if (flags & O_NOATIME)
> +      	opt |= PC_KEEP_HANDLE;
> +
>         /* This is a temporary kludge until all utilities can catch up
>   	 with a change in behavior that implements linux functionality:
>   	 opening a tty should not automatically cause it to become the
> 
> And then create an STC like this:
> 
>    #define _GNU_SOURCE 1
>    #include <stdio.h>
>    #include <unistd.h>
>    #include <fcntl.h>
> 
>    int
>    main (int argc, char **argv)
>    {
>      int fd, fd2;
> 
>      fd = open (argv[1], O_RDONLY | O_NOATIME);
>      dup (fd);
>    }
> 
>> 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.

If this is the only place where duplication is needed, we could just do it in 
build_fh_pc or fhandler_base::set_name.  We would probably want to add a 
path_conv::dup_conv_handle method:

   inline void dup_conv_handle (path_conv& pc)
   {
     conv_handle.close ();
     conv_handle.dup (pc.conv_handle);
   }

Ken

Re: Potential handle leaks in dup_worker

[Corinna Vinschen]

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.


Corinna

Re: Potential handle leaks in dup_worker

[Corinna Vinschen]

On Feb  9 16:02, 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.

...and maybe we should just convert the remaining reset method
to an inline method then...


Corinna

Re: Potential handle leaks in dup_worker

[Ken Brown]

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

Ken

Re: Potential handle leaks in dup_worker

[Corinna Vinschen]

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?


Thanks,
Corinna

Re: Potential handle leaks in dup_worker

[Ken Brown]

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.

Ken

Re: Potential handle leaks in dup_worker

[Ken Brown]

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

Re: Potential handle leaks in dup_worker

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 3323 bytes --]

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.


Thanks,
Corinna

[-- Attachment #2: 0001-Cygwin-fhandler-clean-up-copyto-logic.patch --]
[-- Type: text/plain, Size: 29791 bytes --]

From ebc3ebd59a432597d5ab27f977868eb67fc160e1 Mon Sep 17 00:00:00 2001
From: Corinna Vinschen <corinna@vinschen.de>
Date: Tue, 9 Feb 2021 21:46:29 +0100
Subject: [PATCH] Cygwin: fhandler: clean up 'copyto' logic

There's a slim chance that duplicating fhandlers may end up duplicating
path_conv_handle handles twice ending up with a handle leak, due to
fhandler_base::reset calling path_conv::operator<< after the caller
already called path_conv::operator=.

Analyzing this problem lead to the observation that the reset method was
called from copyto only anyway, thus allowing to streamline this code.

Making reset a protected method uncovered the problem that the copyto
method is actually thought upside down from an object oriented POV.
Rather than calling copyto, manipulating the object given as parameter,
rename the method to copy_from, which manipulates the calling object
itself from the object given as parameter.

Eventually make reset a protected method and rename it to
_copy_from_reset_helper to clarify it's only called from copy_from.

Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
 winsup/cygwin/fhandler.cc      |  16 +-
 winsup/cygwin/fhandler.h       | 377 +++++++++++++++++----------------
 winsup/cygwin/fhandler_pipe.cc |   2 +-
 3 files changed, 197 insertions(+), 198 deletions(-)

diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
index 5dbbd4068ce1..4dac696f2b1a 100644
--- a/winsup/cygwin/fhandler.cc
+++ b/winsup/cygwin/fhandler.cc
@@ -40,18 +40,6 @@ struct __cygwin_perfile *perfile_table;
 HANDLE NO_COPY fhandler_base_overlapped::asio_done;
 LONG NO_COPY fhandler_base_overlapped::asio_close_counter;
 
-void
-fhandler_base::reset (const fhandler_base *from)
-{
-  pc << from->pc;
-  ra.rabuf = NULL;
-  ra.ralen = 0;
-  ra.raixget = 0;
-  ra.raixput = 0;
-  ra.rabuflen = 0;
-  _refcnt = 0;
-}
-
 int
 fhandler_base::puts_readahead (const char *s, size_t len)
 {
@@ -467,7 +455,7 @@ fhandler_base::open_with_arch (int flags, mode_t mode)
     {
       if (!archetype->get_handle ())
 	{
-	  copyto (archetype);
+	  archetype->copy_from (this);
 	  archetype_usecount (1);
 	  archetype->archetype = NULL;
 	  usecount = 0;
@@ -484,7 +472,7 @@ fhandler_base::open_with_arch (int flags, mode_t mode)
 	      strcpy (name, get_name ());
 	    }
 	  fhandler_base *arch = archetype;
-	  archetype->copyto (this);
+	  copy_from (archetype);
 	  if (name)
 	    set_name (name);
 	  archetype = arch;
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 2fad7d33c0af..b2c814cc2754 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -229,7 +229,6 @@ class fhandler_base
 
   path_conv pc;
 
-  void reset (const fhandler_base *);
   virtual bool use_archetype () const {return false;}
   virtual void set_name (path_conv &pc);
   virtual void set_name (const char *s)
@@ -493,18 +492,30 @@ public:
 
   fhandler_base (void *) {}
 
-  virtual void copyto (fhandler_base *x)
+ protected:
+  void _copy_from_reset_helper ()
+  {
+    ra.rabuf = NULL;
+    ra.ralen = 0;
+    ra.raixget = 0;
+    ra.raixput = 0;
+    ra.rabuflen = 0;
+    _refcnt = 0;
+  }
+
+ public:
+  virtual void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_base *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_base *> (x);
+    _copy_from_reset_helper ();
   }
 
   virtual fhandler_base *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_base));
     fhandler_base *fh = new (ptr) fhandler_base (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -752,18 +763,18 @@ class fhandler_socket_inet: public fhandler_socket_wsock
   /* from here on: CLONING */
   fhandler_socket_inet (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_socket_inet *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_socket_inet *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_socket_inet *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_socket_inet));
     fhandler_socket_inet *fh = new (ptr) fhandler_socket_inet (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -850,18 +861,18 @@ class fhandler_socket_local: public fhandler_socket_wsock
   /* from here on: CLONING */
   fhandler_socket_local (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_socket_local *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_socket_local *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_socket_local *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_socket_local));
     fhandler_socket_local *fh = new (ptr) fhandler_socket_local (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -1137,18 +1148,18 @@ class fhandler_socket_unix : public fhandler_socket
   /* from here on: CLONING */
   fhandler_socket_unix (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_socket_unix *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_socket_unix *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_socket_unix *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_socket_unix));
     fhandler_socket_unix *fh = new (ptr) fhandler_socket_unix (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -1204,19 +1215,19 @@ public:
       cfree (atomic_write_buf);
   }
 
-  virtual void copyto (fhandler_base *x)
+  virtual void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_base_overlapped *> (x) = *this;
-    reinterpret_cast<fhandler_base_overlapped *> (x)->atomic_write_buf = NULL;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_base_overlapped *> (x);
+    atomic_write_buf = NULL;
+    _copy_from_reset_helper ();
   }
 
   virtual fhandler_base_overlapped *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_base_overlapped));
     fhandler_base_overlapped *fh = new (ptr) fhandler_base_overlapped (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 
@@ -1253,19 +1264,19 @@ public:
 		       const char *, DWORD, int64_t *unique_id = NULL);
   fhandler_pipe (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_pipe *> (x) = *this;
-    reinterpret_cast<fhandler_pipe *> (x)->atomic_write_buf = NULL;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_pipe *> (x);
+    atomic_write_buf = NULL;
+    _copy_from_reset_helper ();
   }
 
   fhandler_pipe *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_pipe));
     fhandler_pipe *fh = new (ptr) fhandler_pipe (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -1506,18 +1517,18 @@ public:
 
   fhandler_fifo (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_fifo *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_fifo *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_fifo *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_fifo));
     fhandler_fifo *fhf = new (ptr) fhandler_fifo (ptr);
-    copyto (fhf);
+    fhf->copy_from (this);
     fhf->pipe_name_buf = NULL;
     return fhf;
   }
@@ -1558,18 +1569,18 @@ class fhandler_dev_raw: public fhandler_base
 
   fhandler_dev_raw (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_dev_raw *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_dev_raw *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_dev_raw *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_dev_raw));
     fhandler_dev_raw *fh = new (ptr) fhandler_dev_raw (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -1617,18 +1628,18 @@ class fhandler_dev_floppy: public fhandler_dev_raw
 
   fhandler_dev_floppy (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_dev_floppy *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_dev_floppy *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_dev_floppy *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_dev_floppy));
     fhandler_dev_floppy *fh = new (ptr) fhandler_dev_floppy (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -1665,18 +1676,18 @@ class fhandler_dev_tape: public fhandler_dev_raw
 
   fhandler_dev_tape (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_dev_tape *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_dev_tape *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_dev_tape *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_dev_tape));
     fhandler_dev_tape *fh = new (ptr) fhandler_dev_tape (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -1738,18 +1749,18 @@ class fhandler_disk_file: public fhandler_base
   fhandler_disk_file (void *) {}
   dev_t get_dev () { return pc.fs_serial_number (); }
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_disk_file *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_disk_file *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_disk_file *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_disk_file));
     fhandler_disk_file *fh = new (ptr) fhandler_disk_file (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -1774,18 +1785,18 @@ public:
   dev_t get_dev () { return dir_exists ? pc.fs_serial_number ()
 				       : get_device (); }
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_dev *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_dev *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_dev *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_dev));
     fhandler_dev *fh = new (ptr) fhandler_dev (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -1805,18 +1816,18 @@ class fhandler_cygdrive: public fhandler_disk_file
   fhandler_cygdrive (void *) {}
   dev_t get_dev () { return get_device (); }
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_cygdrive *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_cygdrive *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_cygdrive *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_cygdrive));
     fhandler_cygdrive *fh = new (ptr) fhandler_cygdrive (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -1864,18 +1875,18 @@ class fhandler_serial: public fhandler_base
 
   fhandler_serial (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_serial *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_serial *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_serial *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_serial));
     fhandler_serial *fh = new (ptr) fhandler_serial (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -1934,18 +1945,18 @@ class fhandler_termios: public fhandler_base
 
   fhandler_termios (void *) {}
 
-  virtual void copyto (fhandler_base *x)
+  virtual void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_termios *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_termios *> (x);
+    _copy_from_reset_helper ();
   }
 
   virtual fhandler_termios *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_termios));
     fhandler_termios *fh = new (ptr) fhandler_termios (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2194,18 +2205,18 @@ private:
 
   fhandler_console (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_console *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_console *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_console *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_console));
     fhandler_console *fh = new (ptr) fhandler_console (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
   input_states process_input_message ();
@@ -2267,18 +2278,18 @@ class fhandler_pty_common: public fhandler_termios
 
   fhandler_pty_common (void *) {}
 
-  virtual void copyto (fhandler_base *x)
+  virtual void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_pty_common *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_pty_common *> (x);
+    _copy_from_reset_helper ();
   }
 
   virtual fhandler_pty_common *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_pty_common));
     fhandler_pty_common *fh = new (ptr) fhandler_pty_common (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 
@@ -2348,18 +2359,18 @@ class fhandler_pty_slave: public fhandler_pty_common
 
   fhandler_pty_slave (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_pty_slave *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_pty_slave *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_pty_slave *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_pty_slave));
     fhandler_pty_slave *fh = new (ptr) fhandler_pty_slave (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
   bool setup_pseudoconsole (bool nopcon);
@@ -2444,18 +2455,18 @@ public:
 
   fhandler_pty_master (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_pty_master *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_pty_master *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_pty_master *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_pty_master));
     fhandler_pty_master *fh = new (ptr) fhandler_pty_master (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
   bool to_be_read_from_pcon (void);
@@ -2474,18 +2485,18 @@ class fhandler_dev_null: public fhandler_base
 
   fhandler_dev_null (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_dev_null *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_dev_null *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_dev_null *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_dev_null));
     fhandler_dev_null *fh = new (ptr) fhandler_dev_null (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 
@@ -2510,18 +2521,18 @@ class fhandler_dev_zero: public fhandler_base
 
   fhandler_dev_zero (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_dev_zero *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_dev_zero *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_dev_zero *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_dev_zero));
     fhandler_dev_zero *fh = new (ptr) fhandler_dev_zero (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2542,18 +2553,18 @@ class fhandler_dev_random: public fhandler_base
   fhandler_dev_random () : fhandler_base () {}
   fhandler_dev_random (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_dev_random *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_dev_random *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_dev_random *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_dev_random));
     fhandler_dev_random *fh = new (ptr) fhandler_dev_random (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2580,18 +2591,18 @@ class fhandler_dev_clipboard: public fhandler_base
 
   fhandler_dev_clipboard (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_dev_clipboard *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_dev_clipboard *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_dev_clipboard *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_dev_clipboard));
     fhandler_dev_clipboard *fh = new (ptr) fhandler_dev_clipboard (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2618,18 +2629,18 @@ class fhandler_windows: public fhandler_base
 
   fhandler_windows (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_windows *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_windows *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_windows *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_windows));
     fhandler_windows *fh = new (ptr) fhandler_windows (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2672,18 +2683,18 @@ class fhandler_dev_dsp: public fhandler_base
 
   fhandler_dev_dsp (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_dev_dsp *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_dev_dsp *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_dev_dsp *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_dev_dsp));
     fhandler_dev_dsp *fh = new (ptr) fhandler_dev_dsp (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2723,18 +2734,18 @@ class fhandler_virtual : public fhandler_base
 
   fhandler_virtual (void *) {}
 
-  virtual void copyto (fhandler_base *x)
+  virtual void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_virtual *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_virtual *> (x);
+    _copy_from_reset_helper ();
   }
 
   virtual fhandler_virtual *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_virtual));
     fhandler_virtual *fh = new (ptr) fhandler_virtual (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2755,18 +2766,18 @@ class fhandler_proc: public fhandler_virtual
 
   fhandler_proc (void *) {}
 
-  virtual void copyto (fhandler_base *x)
+  virtual void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_proc *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_proc *> (x);
+    _copy_from_reset_helper ();
   }
 
   virtual fhandler_proc *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_proc));
     fhandler_proc *fh = new (ptr) fhandler_proc (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2791,18 +2802,18 @@ class fhandler_procsys: public fhandler_virtual
 
   fhandler_procsys (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_procsys *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_procsys *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_procsys *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_procsys));
     fhandler_procsys *fh = new (ptr) fhandler_procsys (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2820,18 +2831,18 @@ class fhandler_procsysvipc: public fhandler_proc
 
   fhandler_procsysvipc (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_procsysvipc *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_procsysvipc *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_procsysvipc *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_procsysvipc));
     fhandler_procsysvipc *fh = new (ptr) fhandler_procsysvipc (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2851,18 +2862,18 @@ class fhandler_netdrive: public fhandler_virtual
 
   fhandler_netdrive (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_netdrive *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_netdrive *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_netdrive *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_netdrive));
     fhandler_netdrive *fh = new (ptr) fhandler_netdrive (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2892,18 +2903,18 @@ class fhandler_registry: public fhandler_proc
 
   fhandler_registry (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_registry *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_registry *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_registry *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_registry));
     fhandler_registry *fh = new (ptr) fhandler_registry (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2926,18 +2937,18 @@ class fhandler_process: public fhandler_proc
 
   fhandler_process (void *) {}
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_process *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_process *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_process *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_process));
     fhandler_process *fh = new (ptr) fhandler_process (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2954,18 +2965,18 @@ class fhandler_process_fd : public fhandler_process
   int __reg2 fstat (struct stat *buf);
   virtual int __reg2 link (const char *);
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_process_fd *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_process_fd *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_process_fd *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_process_fd));
     fhandler_process_fd *fh = new (ptr) fhandler_process_fd (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -2982,18 +2993,18 @@ class fhandler_procnet: public fhandler_proc
   int __reg2 fstat (struct stat *buf);
   bool fill_filebuf ();
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_procnet *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_procnet *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_procnet *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_procnet));
     fhandler_procnet *fh = new (ptr) fhandler_procnet (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -3022,18 +3033,18 @@ class fhandler_signalfd : public fhandler_base
   select_record *select_write (select_stuff *);
   select_record *select_except (select_stuff *);
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_signalfd *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_signalfd *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_signalfd *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_signalfd));
     fhandler_signalfd *fh = new (ptr) fhandler_signalfd (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
@@ -3072,18 +3083,18 @@ class fhandler_timerfd : public fhandler_base
   select_record *select_write (select_stuff *);
   select_record *select_except (select_stuff *);
 
-  void copyto (fhandler_base *x)
+  void copy_from (fhandler_base *x)
   {
-    x->pc.free_strings ();
-    *reinterpret_cast<fhandler_timerfd *> (x) = *this;
-    x->reset (this);
+    pc.free_strings ();
+    *this = *reinterpret_cast<fhandler_timerfd *> (x);
+    _copy_from_reset_helper ();
   }
 
   fhandler_timerfd *clone (cygheap_types malloc_type = HEAP_FHANDLER)
   {
     void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_timerfd));
     fhandler_timerfd *fh = new (ptr) fhandler_timerfd (ptr);
-    copyto (fh);
+    fh->copy_from (this);
     return fh;
   }
 };
diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index edbaded68950..7d0cdcd79cad 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -96,7 +96,7 @@ fhandler_pipe::open (int flags, mode_t mode)
 	  if ((rwflags == O_RDONLY && !(cfd->get_access () & GENERIC_READ))
 	      || (rwflags == O_WRONLY && !(cfd->get_access () & GENERIC_WRITE)))
 	    continue;
-	  cfd->copyto (this);
+	  copy_from (cfd);
 	  set_handle (NULL);
 	  pc.reset_conv_handle ();
 	  if (!cfd->dup (this, flags))
-- 
2.29.2

Re: 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

Re: Potential handle leaks in dup_worker

[Corinna Vinschen]

On Feb  9 17:31, Ken Brown via Cygwin-developers wrote:
> On 2/9/2021 3:52 PM, Corinna Vinschen via Cygwin-developers wrote:
> > On Feb  9 14:12, Ken Brown via Cygwin-developers wrote:
> > > 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.

Good idea.  Done.


Thanks,
Corinna