[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063 Tom de Vries changed: What|Removed |Added Status|UNCONFIRMED |RESOLVED Resolution|--- |FIXED Assignee|unassigned at gcc dot gnu.org |vries at gcc dot gnu.org Target Milestone|--- |9.0 --- Comment #15 from Tom de Vries --- Patch fixing the problem mentioned in the description committed to trunk. Other problems mentioned fixed as well in trunk. Marking resolved-fixed.
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063 --- Comment #14 from Tom de Vries --- Author: vries Date: Fri Dec 28 03:43:26 2018 New Revision: 267443 URL: https://gcc.gnu.org/viewcvs?rev=267443&root=gcc&view=rev Log: [libbacktrace] Fix memory leak in loop in build_address_map When failing in build_address_map, we free the unit that's currently being handled in the loop, but the ones that already have been allocated are leaked. Fix this by keeping track of allocated units in a vector, and releasing them upon failure. Also, now that we have a vector of allocated units, move the freeing upon failure of the abbrevs associated with each unit to build_address_map, and remove the now redundant call to free_unit_addrs_vector. Bootstrapped and reg-tested on x86_64. 2018-12-28 Ian Lance Taylor Tom de Vries PR libbacktrace/88063 * dwarf.c (free_unit_addrs_vector): Remove. (build_address_map): Keep track of allocated units in vector. Free allocated units and corresponding abbrevs upon failure. Remove now redundant call to free_unit_addrs_vector. Free addrs vector upon failure. Free allocated unit vector. Modified: trunk/libbacktrace/ChangeLog trunk/libbacktrace/dwarf.c
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063
--- Comment #13 from Tom de Vries ---
Comment on attachment 45063
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45063
combined patch
>+ // We only kept the list of units to free them on failure. On
>+ // success the units are retained, pointed to by the entries in
>+ // addrs.
>+ units.alc += units.size;
>+ units.size = 0;
>+ backtrace_vector_release (state, &units, error_callback, data);
If I force mmap.c to alloc.c like so:
...
diff --git a/libbacktrace/mmap.c b/libbacktrace/mmap.c
index 32fcba62399..e7fea31b095 100644
--- a/libbacktrace/mmap.c
+++ b/libbacktrace/mmap.c
@@ -1,3 +1,5 @@
+#include "alloc.c"
+#if 0
/* mmap.c -- Memory allocation with mmap.
Copyright (C) 2012-2018 Free Software Foundation, Inc.
Written by Ian Lance Taylor, Google.
@@ -323,3 +325,4 @@ backtrace_vector_release (struct backtrace_state *state,
vec->alc = 0;
return 1;
}
+#endif
...
we get:
...
$ ./btest
realloc: No such file or directory
realloc: No such file or directory
realloc: No such file or directory
FAIL: backtrace_full noinline
...
because realloc happens to return NULL when called with size == 0 in
backtrace_vector_release, and then backtrace_vector_release calls the error
callback:
...
vec->base = realloc (vec->base, vec->size);
if (vec->base == NULL)
{
error_callback (data, "realloc", errno);
return 0;
}
...
Fixed by patch submitted here:
https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01914.html
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063 --- Comment #12 from Tom de Vries --- Created attachment 45063 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45063&action=edit combined patch > Can you attach one single patch from trunk? Patch combining: - Updated patch https://gcc.gnu.org/bugzilla/attachment.cgi?id=45055&action=edit - Followup patch https://gcc.gnu.org/bugzilla/attachment.cgi?id=45058&action=edit - Second followup patch https://gcc.gnu.org/bugzilla/attachment.cgi?id=45059&action=edit
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063 --- Comment #11 from Ian Lance Taylor --- Sorry, I've gotten confused. Can you attach one single patch from trunk? Thanks.
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063 --- Comment #10 from Tom de Vries --- (In reply to Tom de Vries from comment #9) > Created attachment 45059 [details] > Second followup patch > > > [ OTOH, that's a memory leak in the fail > > case, but corresponds to unused memory in the success case, so we might > > wanna fix that as well, by keeping track of the number of struct unit_addr > > found, and freeing the struct unit at the end of the 'while (info.left > 0)' > > loop when it's 0. Perhaps that's worth a seperate patch. ] > > Patch implementing this memory optimization. After instrumenting the patch to print a marker when this triggers, we have these trigger counts in the tests: ... $ grep -c 'HERE!!!' *.log btest.log:94 ctesta.log:94 ctestg.log:94 dtest.log:94 edtest.log:94 stest.log:0 ttest.log:1000 ztest.log:0 ... So, this seems common enough.
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063 --- Comment #9 from Tom de Vries --- Created attachment 45059 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45059&action=edit Second followup patch > [ OTOH, that's a memory leak in the fail > case, but corresponds to unused memory in the success case, so we might > wanna fix that as well, by keeping track of the number of struct unit_addr > found, and freeing the struct unit at the end of the 'while (info.left > 0)' > loop when it's 0. Perhaps that's worth a seperate patch. ] Patch implementing this memory optimization.
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063 Tom de Vries changed: What|Removed |Added Attachment #45057|0 |1 is obsolete|| --- Comment #8 from Tom de Vries --- Created attachment 45058 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45058&action=edit Followup patch [ Oops, previous attachment did not build. ]
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063 --- Comment #7 from Tom de Vries --- Created attachment 45057 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45057&action=edit Followup patch > [ Btw, note that if we move the u and pu allocations to before read_abbrevs, > we can read the abbrevs directly into u->abbrevs, and can get rid of the > "free_abbrevs (&abbrevs)" at the end. Perhaps that's worth a separate patch. > ] Patch implementing this simplification.
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063 --- Comment #6 from Tom de Vries --- Created attachment 45055 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45055&action=edit Updated patch
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063
--- Comment #5 from Tom de Vries ---
(In reply to Tom de Vries from comment #4)
> Comment on attachment 45048 [details]
> Proposed patch
>
> >@@ -1476,6 +1483,15 @@ build_address_map (struct backtrace_stat
> >backtrace_alloc (state, sizeof *u, error_callback, data));
> > if (u == NULL)
> > goto fail;
> >+
> >+ pu = ((struct unit **)
> >+backtrace_vector_grow (state, sizeof (struct unit *),
> >+ error_callback, data, &units));
> >+ if (pu == NULL)
>
> This introduces another memory leak, this is missing here:
> ...
> {
> backtrace_free (state, u, sizeof *u, error_callback, data);
> ...
>
> >+goto fail;
>
> ...
> }
> ...
>
> [ Btw, note that if we move the u and pu allocations to before read_abbrevs,
> we can read the abbrevs directly into u->abbrevs, and can get rid of the
> "free_abbrevs (&abbrevs)" at the end. Perhaps that's worth a separate patch.
> ]
>
Hmm, likewise, if we move the pu allocation before the u allocation, we can
drop the "backtrace_free (state, u, sizeof *u, error_callback, data);".
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063
--- Comment #4 from Tom de Vries ---
Comment on attachment 45048
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45048
Proposed patch
>@@ -1476,6 +1483,15 @@ build_address_map (struct backtrace_stat
> backtrace_alloc (state, sizeof *u, error_callback, data));
> if (u == NULL)
> goto fail;
>+
>+ pu = ((struct unit **)
>+ backtrace_vector_grow (state, sizeof (struct unit *),
>+ error_callback, data, &units));
>+ if (pu == NULL)
This introduces another memory leak, this is missing here:
...
{
backtrace_free (state, u, sizeof *u, error_callback, data);
...
>+ goto fail;
...
}
...
[ Btw, note that if we move the u and pu allocations to before read_abbrevs, we
can read the abbrevs directly into u->abbrevs, and can get rid of the
"free_abbrevs (&abbrevs)" at the end. Perhaps that's worth a separate patch. ]
>@@ -1502,23 +1518,37 @@ build_address_map (struct backtrace_stat
> u, addrs))
> {
> free_abbrevs (state, &u->abbrevs, error_callback, data);
>-backtrace_free (state, u, sizeof *u, error_callback, data);
> goto fail;
> }
>
> if (unit_buf.reported_underflow)
> {
> free_abbrevs (state, &u->abbrevs, error_callback, data);
>-backtrace_free (state, u, sizeof *u, error_callback, data);
> goto fail;
> }
> }
> if (info.reported_underflow)
> goto fail;
>
> fail:
>+ if (units_count > 0)
>+{
>+ pu = (struct unit **) units.base;
>+ for (i = 0; i < units_count; i++)
>+ backtrace_free (state, *pu, sizeof **pu, error_callback, data);
"*pu" is constant in the loop, and should be "pu[i]".
Also, now that we loop over all units, the simplest solution is to free the
abbrevs in the same loop, and get rid of the two free_abbrevs above and the
free_unit_addrs_vector below.
This also fixes a memory leak of abbrevs referenced from a unit that has no
corresponding struct unit_addr. [ OTOH, that's a memory leak in the fail case,
but corresponds to unused memory in the success case, so we might wanna fix
that as well, by keeping track of the number of struct unit_addr found, and
freeing the struct unit at the end of the 'while (info.left > 0)' loop when
it's 0. Perhaps that's worth a seperate patch. ]
>+ units.alc += units.size;
>+ units.size = 0;
>+ backtrace_vector_release (state, &units, error_callback, data);
>+}
> free_abbrevs (state, &abbrevs, error_callback, data);
> free_unit_addrs_vector (state, addrs, error_callback, data);
The call to free_unit_addrs_vector does not free the actual vector, I think
that's another memory leak.
> return 0;
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063 Ian Lance Taylor changed: What|Removed |Added CC||ian at airs dot com --- Comment #3 from Ian Lance Taylor --- Created attachment 45048 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45048&action=edit Proposed patch Does this patch look right to you?
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063 --- Comment #2 from Tom de Vries --- (In reply to Tom de Vries from comment #0) > However, the allocation and deallocation is done in a loop over units, so if > find_address_ranges succeeds for the first unit, but fails for the second, > then only the first struct unit is freed, and the second struct unit is > leaked. Sorry, that should be the other way around: "then only the second struct unit is freed, and the first struct unit is leaked. "
[Bug libbacktrace/88063] Libbacktrace leak on dwarf read failure
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88063 --- Comment #1 from Tom de Vries --- Created attachment 45027 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45027&action=edit Detection patch Output: ... $ ./btest 2>&1 | sed 's%/.*/%%' alloc at state.c:66: addr: 0x7f9addf62000, size: 72, allocs: 1 alloc at backtrace.c:118: addr: 0x7f9addf61000, size: 4096, allocs: 2 free at backtrace.c:123: addr: 0x7f9addf61000, size: 4096, allocs: 1 alloc at elf.c:2952: addr: 0x7f9addf61000, size: 24, allocs: 2 alloc at elf.c:622: addr: 0x7f9addf57000, size: 5160, allocs: 3 alloc at elf.c:836: addr: 0x7f9addf58428, size: 73, allocs: 4 free at elf.c:861: addr: 0x7f9addf58428, size: 73, allocs: 3 alloc at dwarf.c:1074: addr: 0x7f9addf58428, size: 32, allocs: 4 alloc at dwarf.c:1116: addr: 0x7f9addf58478, size: 56, allocs: 5 alloc at dwarf.c:1481: addr: 0x7f9addf584b0, size: 120, allocs: 6 alloc at dwarf.c:1074: addr: 0x7f9addf586a8, size: 160, allocs: 7 alloc at dwarf.c:1116: addr: 0x7f9addf58748, size: 40, allocs: 8 alloc at dwarf.c:1116: addr: 0x7f9addf58770, size: 24, allocs: 9 alloc at dwarf.c:1116: addr: 0x7f9addf58788, size: 24, allocs: 10 alloc at dwarf.c:1116: addr: 0x7f9addf587a0, size: 8, allocs: 11 alloc at dwarf.c:1116: addr: 0x7f9addf587a8, size: 48, allocs: 12 alloc at dwarf.c:1481: addr: 0x7f9addf587d8, size: 120, allocs: 13 free at dwarf.c:646: addr: 0x7f9addf58748, size: 40, allocs: 12 free at dwarf.c:646: addr: 0x7f9addf58770, size: 24, allocs: 11 free at dwarf.c:646: addr: 0x7f9addf58788, size: 24, allocs: 10 free at dwarf.c:646: addr: 0x7f9addf587a0, size: 8, allocs: 9 free at dwarf.c:646: addr: 0x7f9addf587a8, size: 48, allocs: 8 free at dwarf.c:649: addr: 0x7f9addf586a8, size: 160, allocs: 7 free at dwarf.c:1510: addr: 0x7f9addf587d8, size: 120, allocs: 6 free at dwarf.c:649: addr: (nil), size: 0, allocs: 6 free at dwarf.c:646: addr: 0x7f9addf58478, size: 56, allocs: 5 free at dwarf.c:649: addr: 0x7f9addf58428, size: 32, allocs: 4 Expected 3 allocs, but have: 4 ... This is the struct unit alloc without corresponding free: ... alloc at dwarf.c:1481: addr: 0x7f9addf584b0, size: 120, allocs: 6 ...
