Message ID | 20190222163645.9349-1-tromey@adacore.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 81389 invoked by alias); 22 Feb 2019 16:36:53 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 81329 invoked by uid 89); 22 Feb 2019 16:36:52 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=HContent-Transfer-Encoding:8bit X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 22 Feb 2019 16:36:51 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id DE708116A15; Fri, 22 Feb 2019 11:36:49 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 4F39XnYHi0Gb; Fri, 22 Feb 2019 11:36:49 -0500 (EST) Received: from murgatroyd.Home (75-166-72-210.hlrn.qwest.net [75.166.72.210]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPSA id 8AF5A116A09; Fri, 22 Feb 2019 11:36:49 -0500 (EST) From: Tom Tromey <tromey@adacore.com> To: gdb-patches@sourceware.org Cc: Tom Tromey <tromey@adacore.com> Subject: [RFC] Fix BFD leak in solib-darwin.c Date: Fri, 22 Feb 2019 09:36:45 -0700 Message-Id: <20190222163645.9349-1-tromey@adacore.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Tom Tromey
Feb. 22, 2019, 4:36 p.m. UTC
commit 192b62ce0b4bb5c61188f570e127a26d2c32f716 ("Use class to manage BFD reference counts") changed darwin_get_dyld_bfd to use: + dyld_bfd.release (); rather than - do_cleanups (cleanup); However, using release here leaks the BFD. Instead I believe reset should be used instead. I can't readily test this, so please take a look and let me know what you think. gdb/ChangeLog 2019-02-22 Tom Tromey <tromey@adacore.com> * solib-darwin.c (darwin_get_dyld_bfd): Use reset, not release. --- gdb/ChangeLog | 4 ++++ gdb/solib-darwin.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)
Comments
On 2/22/19 8:36 AM, Tom Tromey wrote: > commit 192b62ce0b4bb5c61188f570e127a26d2c32f716 ("Use class to manage > BFD reference counts") changed darwin_get_dyld_bfd to use: > > + dyld_bfd.release (); > > rather than > > - do_cleanups (cleanup); > > However, using release here leaks the BFD. Instead I believe reset > should be used instead. > > I can't readily test this, so please take a look and let me know what > you think. > > gdb/ChangeLog > 2019-02-22 Tom Tromey <tromey@adacore.com> > > * solib-darwin.c (darwin_get_dyld_bfd): Use reset, not release. > --- > gdb/ChangeLog | 4 ++++ > gdb/solib-darwin.c | 2 +- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c > index d3060604bad..da410ad4660 100644 > --- a/gdb/solib-darwin.c > +++ b/gdb/solib-darwin.c > @@ -455,7 +455,7 @@ darwin_get_dyld_bfd () > if (sub != NULL) > dyld_bfd = sub; > else > - dyld_bfd.release (); > + dyld_bfd.reset (nullptr); > } > return dyld_bfd; > } I haven't tested, but this LGTM. Simon has noticed several instances of this bug where release() was used instead of reset(). (The most recent one was in the build-id separate debug file code that I think hasn't been committed yet.) It might be worth doing a quick sweep of current 'release' calls (if there aren't too many) to check for other leaks.
On 02/22/2019 04:36 PM, Tom Tromey wrote: > if (sub != NULL) > dyld_bfd = sub; > else > - dyld_bfd.release (); > + dyld_bfd.reset (nullptr); These 4 lines are now equivalent to just doing: dyld_bfd = sub; Thanks, Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> On 02/22/2019 04:36 PM, Tom Tromey wrote: >> if (sub != NULL) >> dyld_bfd = sub; >> else >> - dyld_bfd.release (); >> + dyld_bfd.reset (nullptr); Pedro> These 4 lines are now equivalent to just doing: Pedro> dyld_bfd = sub; I made this change and I'm going to commit it shortly. Tom
>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:
Re-replying...
John> Simon has noticed several instances of this bug where release()
John> was used instead of reset().
I wonder whether we ought to mark the release method as
"warn_unused_result" to try to avoid future bugs like this. Another
idea is to try to reduce the number of calls to release generally, by
changing more things to take rvalue references or the like.
Tom
On 2/26/19 12:18 PM, Tom Tromey wrote: >>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes: > > Re-replying... > > John> Simon has noticed several instances of this bug where release() > John> was used instead of reset(). > > I wonder whether we ought to mark the release method as > "warn_unused_result" to try to avoid future bugs like this. Another > idea is to try to reduce the number of calls to release generally, by > changing more things to take rvalue references or the like. I think the warning would be useful if it doesn't trigger a lot of false positives. I feel like it wouldn't today. When I was grep'ing for 'release ()' I only focused on the calls that weren't assigning a value to a result and I feel like there weren't many of those (and some were assigning, just not obvious in the one line of grep context).
>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:
John> I think the warning would be useful if it doesn't trigger a lot of false
John> positives. I feel like it wouldn't today. When I was grep'ing for
John> 'release ()' I only focused on the calls that weren't assigning a value
John> to a result and I feel like there weren't many of those (and some were
John> assigning, just not obvious in the one line of grep context).
Yeah. I went ahead and did this... I had to fix up a few spots, nothing
too terrible. It caught one bug, in varobj.c:
if (pretty_printer == Py_None)
pretty_printer.release ();
install_visualizer (var->dynamic, NULL, pretty_printer.release ());
I think that first release should be .reset(nullptr) instead.
I'll send it through the buildbot and see what happens there.
Tom
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c index d3060604bad..da410ad4660 100644 --- a/gdb/solib-darwin.c +++ b/gdb/solib-darwin.c @@ -455,7 +455,7 @@ darwin_get_dyld_bfd () if (sub != NULL) dyld_bfd = sub; else - dyld_bfd.release (); + dyld_bfd.reset (nullptr); } return dyld_bfd; }