Message ID | 20190425165256.31226-6-tromey@adacore.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 103996 invoked by alias); 25 Apr 2019 16:58:27 -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 103988 invoked by uid 89); 25 Apr 2019 16:58:27 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.5 required=5.0 tests=AWL, 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.1 spammy=HX-Languages-Length:1346, destroy, 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; Thu, 25 Apr 2019 16:58:26 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 05A081172DD; Thu, 25 Apr 2019 12:53:04 -0400 (EDT) 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 At4eA6RsLm0N; Thu, 25 Apr 2019 12:53:03 -0400 (EDT) Received: from murgatroyd.Home (97-122-168-123.hlrn.qwest.net [97.122.168.123]) (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 A4D69116529; Thu, 25 Apr 2019 12:53:03 -0400 (EDT) From: Tom Tromey <tromey@adacore.com> To: gdb-patches@sourceware.org Cc: Tom Tromey <tromey@adacore.com> Subject: [PATCH 5/5] Fix memory leak in exception code Date: Thu, 25 Apr 2019 10:52:56 -0600 Message-Id: <20190425165256.31226-6-tromey@adacore.com> In-Reply-To: <20190425165256.31226-1-tromey@adacore.com> References: <20190425165256.31226-1-tromey@adacore.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Tom Tromey
April 25, 2019, 4:52 p.m. UTC
PR gdb/24475 concerns a memory leak coming from gdb's exception handling code. The leak occurs because throw_exception_sjlj does not arrange to destroy the exception object it is passed. However, because gdb_exception has a destructor, it's undefined to longjmp in this situation. This patch fixes the problem by avoiding the need to run any destructors in gdb_rl_callback_handler, by making the gdb_exception "static". gdb/ChangeLog 2019-04-25 Tom Tromey <tromey@adacore.com> PR gdb/24475: * event-top.c (gdb_rl_callback_handler): Make "gdb_rl_expt" static. --- gdb/ChangeLog | 6 ++++++ gdb/event-top.c | 6 +++++- 2 files changed, 11 insertions(+), 1 deletion(-)
Comments
On 4/25/19 5:52 PM, Tom Tromey wrote: > PR gdb/24475 concerns a memory leak coming from gdb's exception > handling code. > > The leak occurs because throw_exception_sjlj does not arrange to > destroy the exception object it is passed. However, because > gdb_exception has a destructor, it's undefined to longjmp in this > situation. > > This patch fixes the problem by avoiding the need to run any > destructors in gdb_rl_callback_handler, by making the gdb_exception > "static". > Why fix it like this, instead of fixing it like in the guile patch? I'd think you could even use a common POD type for both guile and here? Thanks, Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> Why fix it like this, instead of fixing it like in the guile patch?
Pedro> I'd think you could even use a common POD type for both guile and here?
No particularly good reason. I'll change it.
Tom
Pedro> Why fix it like this, instead of fixing it like in the guile patch? Pedro> I'd think you could even use a common POD type for both guile and here? Tom> No particularly good reason. I'll change it. Actually, there was a reason. Converting an exception to a POD means copying the string contents. Perhaps that isn't a big concern. I guess only exceptions escaping to the top level will do this. Tom
On 4/25/19 7:30 PM, Tom Tromey wrote: > Pedro> Why fix it like this, instead of fixing it like in the guile patch? > Pedro> I'd think you could even use a common POD type for both guile and here? > > Tom> No particularly good reason. I'll change it. > > Actually, there was a reason. Converting an exception to a POD means > copying the string contents. > > Perhaps that isn't a big concern. I guess only exceptions escaping to > the top level will do this. Yeah, I don't think it's a concern here. It seemed better than adding a global, thinking about threads, but now that I think a bit more about it, I realize that we won't ever be able to have multiple threads call into readline simultaneously -- it's full of global state. So I'm fine with your patch as is, afterall. I think it'd be good to extend the comment, something like: - /* This is static to avoid undefined behavior when calling - longjmp. */ + /* This is static to avoid undefined behavior when calling + longjmp -- gdb_exception has a dtor with side effects. */ Thanks, Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> It seemed better than adding a global, thinking about threads, but now
Pedro> that I think a bit more about it, I realize that we won't ever be
Pedro> able to have multiple threads call into readline simultaneously -- it's
Pedro> full of global state. So I'm fine with your patch as is, afterall.
Alright, I'll check in that version, after making the tweaks you
requested.
thanks,
Tom
diff --git a/gdb/event-top.c b/gdb/event-top.c index 9fa46c8ad44..f4a7574741a 100644 --- a/gdb/event-top.c +++ b/gdb/event-top.c @@ -205,11 +205,15 @@ gdb_rl_callback_read_char_wrapper (gdb_client_data client_data) static void gdb_rl_callback_handler (char *rl) noexcept { - struct gdb_exception gdb_rl_expt; + /* This is static to avoid undefined behavior when calling + longjmp. */ + static struct gdb_exception gdb_rl_expt; struct ui *ui = current_ui; try { + /* Ensure the exception is reset on each call. */ + gdb_rl_expt = {}; ui->input_handler (gdb::unique_xmalloc_ptr<char> (rl)); } catch (gdb_exception &ex)