[1/3] ref-filter: free memory from used_atom
diff mbox series

Message ID 0102016657e7cfee-f1343b1e-9a85-4cae-990a-cc7177ea8487-000000@eu-west-1.amazonses.com
State New
Headers show
Series
  • ref-filter: reduce memory leaks
Related show

Commit Message

Olga Telezhnaya Oct. 9, 2018, 8:18 a.m. UTC
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

Comments

Junio C Hamano Oct. 11, 2018, 1:03 a.m. UTC | #1
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 Oct. 12, 2018, 2:35 a.m. UTC | #2
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
#
Jeff King Oct. 18, 2018, 6:33 a.m. UTC | #3
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
Junio C Hamano Oct. 18, 2018, 6:50 a.m. UTC | #4
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
Olga Telezhnaya Oct. 18, 2018, 7:26 a.m. UTC | #5
чт, 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

Patch
diff mbox series

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);