[RFA] Fix leak in linespec.c

  Valgrind reports a leak in many tests, such as:
==9382== 16 bytes in 1 blocks are definitely lost in loss record 236 of 3,282
==9382==    at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
==9382==    by 0x4197AF: xrealloc (common-utils.c:64)
==9382==    by 0x51D16A: xresizevec<linespec_canonical_name> (poison.h:170)
==9382==    by 0x51D16A: add_sal_to_sals(linespec_state*, std::vector<symtab_and_line, std::allocator<symtab_and_line> >*, symtab_and_line*, char const*, int) (linespec.c:1041)
==9382==    by 0x51E2BF: create_sals_line_offset (linespec.c:2215)
==9382==    by 0x51E2BF: convert_linespec_to_sals(linespec_state*, linespec*) (linespec.c:2358)
==9382==    by 0x521B5D: convert_explicit_location_to_sals (linespec.c:2473)

Fix leak by xfree-ing self->canonical_names in linespec_state_destructor.
The leak probably appeared with the patch 'Remove cleanup from linespec.c',
as there was a cleanup to xfree canonical_names before the patch.

Tested on Debian/amd64, native and under valgrind.

2019-01-07  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* linespec.c (linespec_state_destructor): Free self->canonical_names.
---
 gdb/linespec.c | 1 +
 1 file changed, 1 insertion(+)
  

On 2019-01-08 1:24 a.m., Philippe Waroquiers wrote:
> Valgrind reports a leak in many tests, such as:

> ==9382== 16 bytes in 1 blocks are definitely lost in loss record 236 of 3,282

> ==9382==    at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)

> ==9382==    by 0x4197AF: xrealloc (common-utils.c:64)

> ==9382==    by 0x51D16A: xresizevec<linespec_canonical_name> (poison.h:170)

> ==9382==    by 0x51D16A: add_sal_to_sals(linespec_state*, std::vector<symtab_and_line, std::allocator<symtab_and_line> >*, symtab_and_line*, char const*, int) (linespec.c:1041)

> ==9382==    by 0x51E2BF: create_sals_line_offset (linespec.c:2215)

> ==9382==    by 0x51E2BF: convert_linespec_to_sals(linespec_state*, linespec*) (linespec.c:2358)

> ==9382==    by 0x521B5D: convert_explicit_location_to_sals (linespec.c:2473)

> 

> Fix leak by xfree-ing self->canonical_names in linespec_state_destructor.

> The leak probably appeared with the patch 'Remove cleanup from linespec.c',

> as there was a cleanup to xfree canonical_names before the patch.

> 

> Tested on Debian/amd64, native and under valgrind.

> 

> 2019-01-07  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

> 

> 	* linespec.c (linespec_state_destructor): Free self->canonical_names.

> ---

>  gdb/linespec.c | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/gdb/linespec.c b/gdb/linespec.c

> index b1ab462e66..f6ef4c2c40 100644

> --- a/gdb/linespec.c

> +++ b/gdb/linespec.c

> @@ -2767,6 +2767,7 @@ static void

>  linespec_state_destructor (struct linespec_state *self)

>  {

>    htab_delete (self->addr_set);

> +  xfree (self->canonical_names);

>  }

>  

>  /* Delete a linespec parser.  */

> 


Ah, indeed.  The original code in decode_line_full looks like:

  /* Arrange for allocated canonical names to be freed.  */
  if (!result.empty ())
    {
      int i;

      make_cleanup (xfree, state->canonical_names);
      for (i = 0; i < result.size (); ++i)
	{
	  gdb_assert (state->canonical_names[i].suffix != NULL);
	  make_cleanup (xfree, state->canonical_names[i].suffix);
	}
    }

... so you are adding the equivalent of the first cleanup.

It would be nice to be able to free the suffix strings in linespec_state_destructor, the
only problem is that we don't know the size of the canonical_names array at that point.

Anyway, LGTM, thanks!

Simon
  

>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> ... so you are adding the equivalent of the first cleanup.

Oops,sorry about that.

I hope eventually either we have ASAN and perhaps LSAN enabled in
development builds; or that it becomes a lot simpler to run tests under
valgrind...  not trying to excuse my error, just trying to see how I
could prevent it next time.

Simon> It would be nice to be able to free the suffix strings in linespec_state_destructor, the
Simon> only problem is that we don't know the size of the canonical_names array at that point.

linespec would definitely benefit from a couple more rounds of C++-ification.
Some of the changes spill out into the rest of gdb though.

Tom
  

On Tue, 2019-01-08 at 17:40 -0700, Tom Tromey wrote:
> > > > > > "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
> 
> Simon> ... so you are adding the equivalent of the first cleanup.
> 
> Oops,sorry about that.
> 
> I hope eventually either we have ASAN and perhaps LSAN enabled in
> development builds; or that it becomes a lot simpler to run tests under
> valgrind...  not trying to excuse my error, just trying to see how I
> could prevent it next time.
Those that do nothing do no errors, and you do a lot,
but you do not do a lot of errors :).
I started working on running the tests under valgrind after I pushed
a patch containing a 'jump using uninitialised data' error.

Effectively, activating tools as part of dev builds (or build bots) is
the way to go.

Regarding making it simpler to run tests under valgrind:
With the help of Simon, the problem of making a 'clean GDB quit' but
without big timeouts seems to be solved.  I still need to do some refinement,
and then I could submit an RFAv2 for 'FORCE_LOCAL_GDB_QUIT_WAIT'.
(need to discuss if this should be activated by default, that was Pedro's
suggestion/hope).

Once this patch is in, it is relatively easy to run all tests under valgrind.
I however have one remaining problem: 2 tests are systematically blocking
under valgrind : gdb.multi/multi-term-settings.exp
and gdb.multi/multi-arch-exec.exp, and must be killed-9.
(a lingering multi-arch test even prevents the linux kernel to suspend,
so rather special state it seems).

Apart of these 2, the results for other tests are relatively comparable
between native and valgrind run, but not equal :(.
So, also some analysis needed on the results (some timeouts maybe ?).

But I hope it will soon be easy/easier to run all tests under valgrind,
including a helper script to scan the gdb.log files to extract the
errors, see below.

With regards to what valgrind detects :
There are still about 110 tests definitely leaking (initially, about 680 tests
were leaking).
However, I have one suppression entry for a (small) leak of some stdio_file in
ui::ui.  This leak happens in many tests, I added a supp entry as I could not
yet solve it (without creating a dangling pointer problem).

For what concerns non leak errors, find below the list.
As far as I could quickly see, all 'Syscall param write(buf)' are
some uninitialised data in padding zone written in a core file
by the bfd lib.  So probably not a real problem, and then need to see
if this is better fixed or if a supp entry is good enough.

There are a few other more worrying errors (such as jumps using uninitialised data,
invalid read, ...), but I had no time yet to work on these:
     32 gdb/testsuite/v7_outputs/gdb.base/debug-expr/gdb.log == Conditional jump or move depends on uninitialised value(s)
      5 gdb/testsuite/v7_outputs/gdb.base/debug-expr/gdb.log == Use of uninitialised value of size 8
     54 gdb/testsuite/v7_outputs/gdb.base/reread/gdb.log == Conditional jump or move depends on uninitialised value(s)
      1 gdb/testsuite/v7_outputs/gdb.cp/inherit/gdb.log == Invalid read of size 8
      1 gdb/testsuite/v7_outputs/gdb.cp/virtbase/gdb.log == Invalid read of size 2
      5 gdb/testsuite/v7_outputs/gdb.cp/virtbase/gdb.log == Invalid read of size 8
      2 gdb/testsuite/v7_outputs/gdb.dwarf2/dw2-icc-opaque/gdb.log == Conditional jump or move depends on uninitialised value(s)
     10 gdb/testsuite/v7_outputs/gdb.linespec/cpls-abi-tag/gdb.log == Conditional jump or move depends on uninitialised value(s)
      3 gdb/testsuite/v7_outputs/gdb.linespec/cpls-abi-tag/gdb.log == Use of uninitialised value of size 8
      1 gdb/testsuite/v7_outputs/gdb.python/lib-types/gdb.log == Use of uninitialised value of size 8
      1 gdb/testsuite/v7_outputs/gdb.python/py-pp-maint/gdb.log == Use of uninitialised value of size 8
      8 gdb/testsuite/v7_outputs/gdb.python/py-type/gdb.log == Use of uninitialised value of size 8


      1 gdb/testsuite/v7_outputs/gdb.ada/task_switch_in_core/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/auxv/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/coredump-filter/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/debug-expr/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/gcore-buffer-overflow/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/gcore/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/gcore-relro/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/gcore-relro-pie/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/gcore-tls-pie/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/info-proc/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/print-symbol-loading/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/siginfo-obj/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/siginfo-thread/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/solib-search/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.base/vdso-warning/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.python/py-strfns/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.reverse/break-precsave/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.reverse/consecutive-precsave/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.reverse/finish-precsave/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.reverse/machinestate-precsave/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.reverse/sigall-precsave/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.reverse/solib-precsave/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.reverse/step-precsave/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.reverse/until-precsave/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.reverse/watch-precsave/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.threads/gcore-thread/gdb.log == Syscall param write(buf) points to uninitialised byte(s)
      1 gdb/testsuite/v7_outputs/gdb.threads/tls-core/gdb.log == Syscall param write(buf) points to uninitialised byte(s)

      1 gdb/testsuite/v7_outputs/gdb.gdb/unittest/gdb.log == Syscall param msync(start) points to unaddressable byte(s)


Philippe

> 
> Simon> It would be nice to be able to free the suffix strings in linespec_state_destructor, the
> Simon> only problem is that we don't know the size of the canonical_names array at that point.
> 
> linespec would definitely benefit from a couple more rounds of C++-ification.
> Some of the changes spill out into the rest of gdb though.
> 
> Tom
  

On Tue, 2019-01-08 at 23:39 +0000, Simon Marchi wrote:
> ... so you are adding the equivalent of the first cleanup.
> 
> It would be nice to be able to free the suffix strings in linespec_state_destructor, the
> only problem is that we don't know the size of the canonical_names array at that point.
> 
> Anyway, LGTM, thanks!
> 
> Simon
Thanks, pushed.

Philippe
  

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> 54 gdb/testsuite/v7_outputs/gdb.base/reread/gdb.log == Conditional jump or move depends on uninitialised value(s)

I think I have a fix for this one.  I plan to check it in soon, maybe
today, as it's been around for quite a while.

Philippe> 1 gdb/testsuite/v7_outputs/gdb.cp/inherit/gdb.log == Invalid read of size 8
Philippe> 1 gdb/testsuite/v7_outputs/gdb.cp/virtbase/gdb.log == Invalid read of size 2
Philippe> 5 gdb/testsuite/v7_outputs/gdb.cp/virtbase/gdb.log == Invalid read of size 8

I believe I saw these with ASAN as well.  I think what happens here is
that the pretty-printer code creates a value from a virtual base class
slice of an object, but doesn't "inflate" it to the full object,
resulting in some out-of-bounds reads.

Tom