From: Corinna Vinschen <corinna-cygwin@cygwin.com>
To: cygwin-developers@cygwin.com
Subject: Re: Potential handle leaks in dup_worker
Date: Tue, 9 Feb 2021 16:04:19 +0100 [thread overview]
Message-ID: <20210209150419.GU4251@calimero.vinschen.de> (raw)
In-Reply-To: <20210209150249.GT4251@calimero.vinschen.de>
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
next prev parent reply other threads:[~2021-02-09 15:04 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-08 17:39 Ken Brown
2021-02-09 9:47 ` Corinna Vinschen
2021-02-09 14:19 ` Ken Brown
2021-02-09 15:02 ` Corinna Vinschen
2021-02-09 15:04 ` Corinna Vinschen [this message]
2021-02-09 15:31 ` Ken Brown
2021-02-09 16:12 ` Corinna Vinschen
2021-02-09 17:13 ` Ken Brown
2021-02-09 19:12 ` Ken Brown
2021-02-09 20:52 ` Corinna Vinschen
2021-02-09 22:31 ` Ken Brown
2021-02-10 9:52 ` Corinna Vinschen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210209150419.GU4251@calimero.vinschen.de \
--to=corinna-cygwin@cygwin.com \
--cc=cygwin-developers@cygwin.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).