| Message ID | 0102016657e7cfee-f1343b1e-9a85-4cae-990a-cc7177ea8487-000000@eu-west-1.amazonses.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ref-filter: reduce memory leaks | expand |
Olga Telezhnaya <olyatelezhnaya@gmail.com> writes: > Release memory from used_atom variable. That much readers would know from a quick look of the patch text. Without knowing what you are aiming at, it is impossible to judge if the patch is a good change. Seeing FREE_AND_NULL(array->items) in the same function makes me think that the designer of ref_array_clear() function would want this sequence of events to work correctly in an ideal future: * Do a ref-filter operation by calling filter_refs(), receiving the result into an array.. * Do another ref-filter by calling filter_refs(), using different criteria, receiving the result into a different array. * Iterate over the resulting refs in the first array, and call format_ref_array_item(). * ref_array_clear() the first array, as the caller is done with it. * Iterate over the resulting refs in the second array, and call format_ref_array_item(). * ref_array_clear() the second array, as the caller is done with it. However, I think it would make it impossible for the second call to work correctly if this code freed used_atom without clearing, and not re-initializing the used_atom_cnt etc. If on the other hand, the only thing you are interested in is to just discard pieces of memory we no longer use, and you are not interested in helping to move us closer to the world in which we can call filter_refs() twice, then the change this patch makes is sufficient. And the place to answer the "what are you aiming at?" question is in the proposed commit log message. In an ideal future, I _think_ the file-scope static variables in ref-filter.c like used_atom and used_atom_cnt should become fields of a new structure (say "struct ref_filter"), with initializer and uninitializer ref_filter_new() and ref_filter_destroy(). When that happens, I think FREE_AND_NULL(used_atom) + used_atom_cnt=0 should become part of ref_filter_destroy(), not part of ref_array_clear(). But we are not there yet, and a clean-up patch like this does not have to be a step towards that goal. > Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com> > --- > ref-filter.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/ref-filter.c b/ref-filter.c > index e1bcb4ca8a197..1b71d08a43a84 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -1996,6 +1996,9 @@ void ref_array_clear(struct ref_array *array) > { > int i; > > + for (i = 0; i < used_atom_cnt; i++) > + free((char *)used_atom[i].name); > + free(used_atom); > for (i = 0; i < array->nr; i++) > free_array_item(array->items[i]); > FREE_AND_NULL(array->items); > > -- > https://github.com/git/git/pull/538
Junio C Hamano <gitster@pobox.com> writes:
> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
These three patches seem to cause t6300 to fail with an attempt to
free an invalid pointer in "git for-each-ref --format='%(push)'"
(6300.25)
*** Error in `/home/gitster/w/git.git/git': free(): invalid pointer: 0x000055cca3a9f920 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x70bcb)[0x7f052fdacbcb]
/lib/x86_64-linux-gnu/libc.so.6(+0x76f96)[0x7f052fdb2f96]
/home/gitster/w/git.git/git(+0x15a866)[0x55cca35ca866]
/home/gitster/w/git.git/git(+0x15ab48)[0x55cca35cab48]
/home/gitster/w/git.git/git(+0x15b6d3)[0x55cca35cb6d3]
/home/gitster/w/git.git/git(+0x15b7dd)[0x55cca35cb7dd]
/home/gitster/w/git.git/git(+0x49e18)[0x55cca34b9e18]
/home/gitster/w/git.git/git(+0x19b20)[0x55cca3489b20]
/home/gitster/w/git.git/git(+0x1aab5)[0x55cca348aab5]
/home/gitster/w/git.git/git(+0x19809)[0x55cca3489809]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf1)[0x7f052fd5c2b1]
/home/gitster/w/git.git/git(+0x1984a)[0x55cca348984a]
======= Memory map: ========
55cca3470000-55cca36cc000 r-xp 00000000 fe:00 2760695 /home/gitster/w/git.git/git
55cca38cc000-55cca38cf000 r--p 0025c000 fe:00 2760695 /home/gitster/w/git.git/git
55cca38cf000-55cca38de000 rw-p 0025f000 fe:00 2760695 /home/gitster/w/git.git/git
55cca38de000-55cca3921000 rw-p 00000000 00:00 0
55cca3a9e000-55cca3abf000 rw-p 00000000 00:00 0 [heap]
7f052fb24000-7f052fb3b000 r-xp 00000000 fe:00 393287 /lib/x86_64-linux-gnu/libgcc_s.so.1
7f052fb3b000-7f052fd3a000 ---p 00017000 fe:00 393287 /lib/x86_64-linux-gnu/libgcc_s.so.1
7f052fd3a000-7f052fd3b000 r--p 00016000 fe:00 393287 /lib/x86_64-linux-gnu/libgcc_s.so.1
7f052fd3b000-7f052fd3c000 rw-p 00017000 fe:00 393287 /lib/x86_64-linux-gnu/libgcc_s.so.1
7f052fd3c000-7f052fed1000 r-xp 00000000 fe:00 392469 /lib/x86_64-linux-gnu/libc-2.24.so
7f052fed1000-7f05300d1000 ---p 00195000 fe:00 392469 /lib/x86_64-linux-gnu/libc-2.24.so
7f05300d1000-7f05300d5000 r--p 00195000 fe:00 392469 /lib/x86_64-linux-gnu/libc-2.24.so
7f05300d5000-7f05300d7000 rw-p 00199000 fe:00 392469 /lib/x86_64-linux-gnu/libc-2.24.so
7f05300d7000-7f05300db000 rw-p 00000000 00:00 0
7f05300db000-7f05300e2000 r-xp 00000000 fe:00 392487 /lib/x86_64-linux-gnu/librt-2.24.so
7f05300e2000-7f05302e1000 ---p 00007000 fe:00 392487 /lib/x86_64-linux-gnu/librt-2.24.so
7f05302e1000-7f05302e2000 r--p 00006000 fe:00 392487 /lib/x86_64-linux-gnu/librt-2.24.so
7f05302e2000-7f05302e3000 rw-p 00007000 fe:00 392487 /lib/x86_64-linux-gnu/librt-2.24.so
7f05302e3000-7f05302fb000 r-xp 00000000 fe:00 392485 /lib/x86_64-linux-gnu/libpthread-2.24.so
7f05302fb000-7f05304fa000 ---p 00018000 fe:00 392485 /lib/x86_64-linux-gnu/libpthread-2.24.so
7f05304fa000-7f05304fb000 r--p 00017000 fe:00 392485 /lib/x86_64-linux-gnu/libpthread-2.24.so
7f05304fb000-7f05304fc000 rw-p 00018000 fe:00 392485 /lib/x86_64-linux-gnu/libpthread-2.24.so
7f05304fc000-7f0530500000 rw-p 00000000 00:00 0
7f0530500000-7f0530519000 r-xp 00000000 fe:00 392698 /lib/x86_64-linux-gnu/libz.so.1.2.8
7f0530519000-7f0530718000 ---p 00019000 fe:00 392698 /lib/x86_64-linux-gnu/libz.so.1.2.8
7f0530718000-7f0530719000 r--p 00018000 fe:00 392698 /lib/x86_64-linux-gnu/libz.so.1.2.8
7f0530719000-7f053071a000 rw-p 00019000 fe:00 392698 /lib/x86_64-linux-gnu/libz.so.1.2.8
7f053071a000-7f053073d000 r-xp 00000000 fe:00 392461 /lib/x86_64-linux-gnu/ld-2.24.so
7f0530916000-7f0530918000 rw-p 00000000 00:00 0
7f0530939000-7f053093d000 rw-p 00000000 00:00 0
7f053093d000-7f053093e000 r--p 00023000 fe:00 392461 /lib/x86_64-linux-gnu/ld-2.24.so
7f053093e000-7f053093f000 rw-p 00024000 fe:00 392461 /lib/x86_64-linux-gnu/ld-2.24.so
7f053093f000-7f0530940000 rw-p 00000000 00:00 0
7ffe894ee000-7ffe89510000 rw-p 00000000 00:00 0 [stack]
7ffe8959e000-7ffe895a1000 r--p 00000000 00:00 0 [vvar]
7ffe895a1000-7ffe895a3000 r-xp 00000000 00:00 0 [vdso]
./test-lib.sh: line 631: 262132 Aborted git for-each-ref --format='%(push)' refs/heads/master > actual
not ok 25 - basic atom: head push
#
# git for-each-ref --format='%(push)' refs/heads/master >actual &&
# sanitize_pgp <actual >actual.clean &&
# test_cmp expected actual.clean
#
On Fri, Oct 12, 2018 at 11:35:01AM +0900, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Olga Telezhnaya <olyatelezhnaya@gmail.com> writes: > > These three patches seem to cause t6300 to fail with an attempt to > free an invalid pointer in "git for-each-ref --format='%(push)'" > (6300.25) I dug into this a bit. I think it's actually a misapplication of the patches on your side. Applying them locally works, but your ot/ref-filter-plug-leaks branch does not. The patch below on top of your branch helps. :) diff --git a/ref-filter.c b/ref-filter.c index f4ff80eca0..4255de1d75 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1567,7 +1567,6 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) continue; } free((char *)v->s); /* we will definitely re-init it on the next line */ - free((char *)v->s); fill_remote_ref_details(atom, refname, branch, &v->s); continue; } else if (starts_with(name, "color:")) { Presumably it came from the manual comment-style fixup. With that fix, the tests run fine for me under ASan/UBSan (with the exception of t5310, but that's fixed already in a parallel topic). -Peff
Jeff King <peff@peff.net> writes: > Presumably it came from the manual comment-style fixup. Wow, that was embarrassing. Thanks for catching it. > > With that fix, the tests run fine for me under ASan/UBSan (with the > exception of t5310, but that's fixed already in a parallel topic). > > -Peff
чт, 18 окт. 2018 г. в 9:51, Junio C Hamano <gitster@pobox.com>: > > Jeff King <peff@peff.net> writes: > > > Presumably it came from the manual comment-style fixup. > > Wow, that was embarrassing. Thanks for catching it. Jeff, thanks a lot! I just sent new version where I fixed all known issues including that comment. > > > > > With that fix, the tests run fine for me under ASan/UBSan (with the > > exception of t5310, but that's fixed already in a parallel topic). > > > > -Peff
diff --git a/ref-filter.c b/ref-filter.c index e1bcb4ca8a197..1b71d08a43a84 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1996,6 +1996,9 @@ void ref_array_clear(struct ref_array *array) { int i; + for (i = 0; i < used_atom_cnt; i++) + free((char *)used_atom[i].name); + free(used_atom); for (i = 0; i < array->nr; i++) free_array_item(array->items[i]); FREE_AND_NULL(array->items);
Release memory from used_atom variable. Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com> --- ref-filter.c | 3 +++ 1 file changed, 3 insertions(+) -- https://github.com/git/git/pull/538