diff mbox

[5/5] Fix memory leak in exception code

Message ID 20190425165256.31226-6-tromey@adacore.com
State New
Headers show

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

Pedro Alves April 25, 2019, 6:06 p.m. UTC | #1
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
Tom Tromey April 25, 2019, 6:18 p.m. UTC | #2
>>>>> "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
Tom Tromey April 25, 2019, 6:30 p.m. UTC | #3
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
Pedro Alves April 25, 2019, 6:38 p.m. UTC | #4
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
Tom Tromey April 25, 2019, 6:50 p.m. UTC | #5
>>>>> "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 mbox

Patch

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)