Patchwork [5/5] Fix memory leak in exception code

login
register
mail settings
Submitter Tom Tromey
Date April 25, 2019, 4:52 p.m.
Message ID <20190425165256.31226-6-tromey@adacore.com>
Download mbox | patch
Permalink /patch/32415/
State New
Headers show

Comments

Tom Tromey - April 25, 2019, 4:52 p.m.
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(-)
Pedro Alves - April 25, 2019, 6:06 p.m.
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.
>>>>> "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.
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.
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.
>>>>> "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

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)