Re: Potential handle leaks in dup_worker

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

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