Re: [patch] Memory leak fixes

[Ali Lakhia <lakhia@alumni.utexas.net>]

On Tue, Oct 5, 2010 at 3:47 PM, Tom Tromey <tromey@redhat.com> wrote:

> Ali> diff -ru gdb-7.1/gdb/dwarf2read.c gdb-7.1/gdb/dwarf2read.c
> Ali> --- gdb-7.1/gdb/dwarf2read.c       2010-09-14 16:59:40.000000000 -0700
> Ali> +++ gdb-7.1/gdb/dwarf2read.c       2010-06-15 14:53:54.000000000 -0700
> Ali> @@ -5462,6 +5462,7 @@
> Ali>    set_descriptive_type (type, die, cu);
> Ali>    do_cleanups (back_to);
>
> Ali> +  xfree (range_types);
>
> This one seems to be fixed in CVS.
> There is a cleanup covering range_types.

Are you referring to the call to make_cleanup() ? That gets called
only if ndim == 0 and I can't really figure out what it is supposed to
do with the arg other than saving it in a cleanup struct and returning
that struct.

> Actually, I see this cleanup in 7.1 as well.
> How did you find this leak?
>
> Maybe the logic around creating the cleanup is wrong.

I found the leak using a static checker. It is known to have false
positives!  :-)

> Ali> diff -ru gdb-7.1/gdb/printcmd.c gdb-7.1/gdb/printcmd.c
> Ali> --- gdb-7.1/gdb/printcmd.c 2010-09-14 17:03:28.000000000 -0700
> Ali> +++ gdb-7.1/gdb/printcmd.c 2010-06-15 14:53:54.000000000 -0700
> Ali> @@ -608,6 +608,7 @@
> Ali>      fputs_filtered (">", stream);
>
> Ali>    do_cleanups (cleanup_chain);
> Ali> +  xfree (filename);
> Ali>  }
>
> This one is also covered by a cleanup.

I don't see any cleanup calls. filename gets allocated by
build_address_symbolic() and nothing is freeing it as far as I can
tell.

> Ali> diff -ru gdb-7.1/gdb/python/py-type.c gdb-7.1/gdb/python/py-type.c
> Ali> --- gdb-7.1/gdb/python/py-type.c   2010-09-14 17:03:28.000000000 -0700
> Ali> +++ gdb-7.1/gdb/python/py-type.c   2010-06-14 14:04:28.000000000 -0700
> Ali> @@ -580,6 +580,7 @@
>
> Ali>        type_print (type_object_to_type (self), "", stb, -1);
>
> Ali> +      xfree (thetype);
> Ali>        thetype = ui_file_xstrdup (stb, &length);
> Ali>        do_cleanups (old_chain);
>
> I didn't check this one, but if you want to insert a free before a call
> to do_cleanups, usually that means you should create a cleanup at the
> point of allocation.

The allocation is happing at the very next line at: ui_file_xstrdup()
and the variable is used after the loop (I'm assuming TRY_CATCH macro
can loop). So, it was safer to free to memory before the leak happens
rather than after allocation,

-Ali