Re: [PATCH 1/3] ref-filter: free memory from used_atom
чт, 18 окт. 2018 г. в 9:51, Junio C Hamano : > > Jeff King 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
Re: [PATCH 1/3] ref-filter: free memory from used_atom
Jeff King 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
Re: [PATCH 1/3] ref-filter: free memory from used_atom
On Fri, Oct 12, 2018 at 11:35:01AM +0900, Junio C Hamano wrote:
> Junio C Hamano writes:
>
> > Olga Telezhnaya 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, >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
Re: [PATCH 1/3] ref-filter: free memory from used_atom
Junio C Hamano writes: > Olga Telezhnaya 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: 0x55cca3a9f920 *** === 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: 55cca347-55cca36cc000 r-xp 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 00:00 0 55cca3a9e000-55cca3abf000 rw-p 00:00 0 [heap] 7f052fb24000-7f052fb3b000 r-xp 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 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 00:00 0 7f05300db000-7f05300e2000 r-xp fe:00 392487 /lib/x86_64-linux-gnu/librt-2.24.so 7f05300e2000-7f05302e1000 ---p 7000 fe:00 392487 /lib/x86_64-linux-gnu/librt-2.24.so 7f05302e1000-7f05302e2000 r--p 6000 fe:00 392487 /lib/x86_64-linux-gnu/librt-2.24.so 7f05302e2000-7f05302e3000 rw-p 7000 fe:00 392487 /lib/x86_64-linux-gnu/librt-2.24.so 7f05302e3000-7f05302fb000 r-xp 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-7f053050 rw-p 00:00 0 7f053050-7f0530519000 r-xp 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 fe:00 392461 /lib/x86_64-linux-gnu/ld-2.24.so 7f0530916000-7f0530918000 rw-p 00:00 0 7f0530939000-7f053093d000 rw-p 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-7f053094 rw-p 00:00 0 7ffe894ee000-7ffe8951 rw-p 00:00 0 [stack] 7ffe8959e000-7ffe895a1000 r--p 00:00 0 [vvar] 7ffe895a1000-7ffe895a3000 r-xp 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.clean && # test_cmp expected actual.clean #
Re: [PATCH 1/3] ref-filter: free memory from used_atom
Olga Telezhnaya 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
> ---
> 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
[PATCH 1/3] ref-filter: free memory from used_atom
Release memory from used_atom variable.
Signed-off-by: Olga Telezhnaia
---
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
