* [RFA] Fix leak in linespec.c
@ 2019-01-08 6:25 Philippe Waroquiers
2019-01-08 23:39 ` Simon Marchi
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Waroquiers @ 2019-01-08 6:25 UTC (permalink / raw)
To: gdb-patches; +Cc: Philippe Waroquiers
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. */
--
2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA] Fix leak in linespec.c
2019-01-08 6:25 [RFA] Fix leak in linespec.c Philippe Waroquiers
@ 2019-01-08 23:39 ` Simon Marchi
2019-01-09 0:40 ` Tom Tromey
2019-01-09 3:35 ` Philippe Waroquiers
0 siblings, 2 replies; 6+ messages in thread
From: Simon Marchi @ 2019-01-08 23:39 UTC (permalink / raw)
To: Philippe Waroquiers, gdb-patches, Tom Tromey
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA] Fix leak in linespec.c
2019-01-08 23:39 ` Simon Marchi
@ 2019-01-09 0:40 ` Tom Tromey
2019-01-09 3:26 ` Philippe Waroquiers
2019-01-09 3:35 ` Philippe Waroquiers
1 sibling, 1 reply; 6+ messages in thread
From: Tom Tromey @ 2019-01-09 0:40 UTC (permalink / raw)
To: Simon Marchi; +Cc: Philippe Waroquiers, gdb-patches, Tom Tromey
>>>>> "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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA] Fix leak in linespec.c
2019-01-09 0:40 ` Tom Tromey
@ 2019-01-09 3:26 ` Philippe Waroquiers
2019-01-09 23:10 ` Tom Tromey
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Waroquiers @ 2019-01-09 3:26 UTC (permalink / raw)
To: Tom Tromey, Simon Marchi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA] Fix leak in linespec.c
2019-01-08 23:39 ` Simon Marchi
2019-01-09 0:40 ` Tom Tromey
@ 2019-01-09 3:35 ` Philippe Waroquiers
1 sibling, 0 replies; 6+ messages in thread
From: Philippe Waroquiers @ 2019-01-09 3:35 UTC (permalink / raw)
To: Simon Marchi, gdb-patches, Tom Tromey
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFA] Fix leak in linespec.c
2019-01-09 3:26 ` Philippe Waroquiers
@ 2019-01-09 23:10 ` Tom Tromey
0 siblings, 0 replies; 6+ messages in thread
From: Tom Tromey @ 2019-01-09 23:10 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: Tom Tromey, Simon Marchi, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-09 23:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-08 6:25 [RFA] Fix leak in linespec.c Philippe Waroquiers
2019-01-08 23:39 ` Simon Marchi
2019-01-09 0:40 ` Tom Tromey
2019-01-09 3:26 ` Philippe Waroquiers
2019-01-09 23:10 ` Tom Tromey
2019-01-09 3:35 ` Philippe Waroquiers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).