Potential handle leaks in dup_worker

Corinna Vinschen corinna-cygwin@cygwin.com
Tue Feb 9 15:04:19 GMT 2021


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


More information about the Cygwin-developers mailing list