From: Taylor Blau <hidden> Date: 2021-10-21 03:39:48
Here is a topic that Ævar got me interested when he mentioned that t5319 is
leaky[1].
This is the result of trying to get t5319 to pass when compiling with
SANITIZE=leak. About 50% of the fixes are in the MIDX code, another 40% are in
the pack-bitmap code, and the remaining 10% are sort of random.
I tried to separate these out based on their respective areas. The last 10% are
all from leaking memory in the rev_info structure, which I punted on for now by
just UNLEAK()-ing it. That's all done in the last patch. If we choose to take
that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a
little gross, so I'm happy to leave it out if others would prefer.
This is based on tb/fix-midx-rename-while-mapped, with a back-merge of
js/windows-ci-path-fix in order to appease CI. It also contains a little bit of
overlap with a patch[2] from Ævar in another series, which he and I discovered
independently. I cherry-picked his patch, since I haven't seen much action on
that series lately.
[1]: https://lore.kernel.org/git/87wnmph73b.fsf@evledraar.gmail.com/
[2]: https://lore.kernel.org/git/patch-v3-08.10-e0a3510dd88-20211013T222329Z-avarab@gmail.com/
Taylor Blau (10):
midx.c: clean up chunkfile after reading the MIDX
midx.c: don't leak MIDX from verify_midx_file
t/helper/test-read-midx.c: free MIDX within read_midx_file()
builtin/pack-objects.c: don't leak memory via arguments
builtin/repack.c: avoid leaking child arguments
builtin/multi-pack-index.c: don't leak concatenated options
pack-bitmap.c: avoid leaking via midx_bitmap_filename()
pack-bitmap.c: don't leak type-level bitmaps
pack-bitmap.c: more aggressively free in free_bitmap_index()
t5319: UNLEAK() the remaining leaks
Ævar Arnfjörð Bjarmason (1):
pack-bitmap-write.c: don't return without stop_progress()
builtin/log.c | 1 +
builtin/multi-pack-index.c | 4 ++++
builtin/pack-objects.c | 12 ++++++++----
builtin/repack.c | 35 ++++++++++++++++++++++++-----------
builtin/rev-list.c | 2 ++
midx.c | 7 +++++--
pack-bitmap-write.c | 8 +++++---
pack-bitmap.c | 18 ++++++++++++++++--
t/helper/test-read-midx.c | 3 ++-
t/t5319-multi-pack-index.sh | 1 +
10 files changed, 68 insertions(+), 23 deletions(-)
--
2.33.0.96.g73915697e6
From: Taylor Blau <hidden> Date: 2021-10-21 03:39:50
In order to read the contents of a MIDX, we initialize a chunkfile
structure which can read the table of contents and assign pointers into
different sections of the file for us.
We do call free(), since the chunkfile struct is heap allocated, but not
the more appropriate free_chunkfile(), which also frees memory that the
structure itself owns.
Call that instead to avoid leaking memory in this function.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Taylor Blau <redacted>
---
The patch contents are from Ævar, but the message is mine. I hope that
he doesn't mind me forging his sign-off here.
midx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Taylor Blau <hidden> Date: 2021-10-21 03:39:55
The function midx.c:verify_midx_file() allocate a MIDX struct by calling
load_multi_pack_index(). But when cleaning up, it calls free() without
freeing any resources associated with the MIDX.
Call the more appropriate close_midx() which does free those resources,
which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.
Signed-off-by: Taylor Blau <redacted>
---
midx.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Taylor Blau <hidden> Date: 2021-10-21 03:40:04
When calling `read_midx_file()` to show information about a MIDX or list
the objects contained within it we fail to call `close_midx()`, leaking
the memory allocated to store that MIDX.
Fix this by calling `close_midx()` before exiting the function. We can
drop the "early" return when `show_objects` is non-zero, since the next
instruction is also a return.
(We could just as easily put a `cleanup` label here as with previous
patches. But the only other time we terminate the function early is
when we fail to load a MIDX in the first place. `close_midx()` does
handle a NULL argument, but the extra complexity is likely not
warranted).
Signed-off-by: Taylor Blau <redacted>
---
t/helper/test-read-midx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Taylor Blau <hidden> Date: 2021-10-21 03:40:13
When constructing arguments to pass to setup_revision(), pack-objects
only frees the memory used by that array after calling
get_object_list().
Ensure that we call strvec_clear() whether or not we use the arguments
array by cleaning up whenever we exit the function (and rewriting one
early return to jump to a label which frees the memory and then
returns).
We could avoid setting this array up altogether unless we are in the
if-else block that calls get_object_list(), but setting up the argument
array is intermingled with lots of other side-effects, e.g.:
if (exclude_promisor_objects) {
use_internal_rev_list = 1;
fetch_if_missing = 0;
strvec_push(&rp, "--exclude-promisor-objects");
}
So it would be awkward to check exclude_promisor_objects twice: first to
set use_internal_rev_list and fetch_if_missing, and then again above
get_object_list() to push the relevant argument onto the array.
Instead, leave the array's construction alone and make sure to free it
unconditionally.
Signed-off-by: Taylor Blau <redacted>
---
builtin/pack-objects.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
From: Taylor Blau <hidden> Date: 2021-10-21 03:40:15
`git repack` invokes a handful of child processes: one to write the
actual pack, and optionally ones to repack promisor objects and update
the MIDX.
In none of these cases do we bother to call child_process_clear(), which
frees the memory associated with each child's arguments and environment.
In order to do so, tweak each function that spawns a child process to
have a `cleanup` label that we always visit before returning from each
function. Then, make sure that we call child_process_clear() as a part
of that label.
Signed-off-by: Taylor Blau <redacted>
---
builtin/repack.c | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
@@ -260,7 +261,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,if(cmd.in==-1)/* No packed objects; cmd was never started */-return;+gotocleanup;close(cmd.in);
@@ -293,7 +294,12 @@ static void repack_promisor_objects(const struct pack_objects_args *args,free(promisor_name);}fclose(out);-if(finish_command(&cmd))+ret=finish_command(&cmd);++cleanup:+child_process_clear(&cmd);++if(ret)die(_("could not finish pack-objects to repack promisor objects"));}
@@ -559,10 +565,10 @@ static int write_midx_included_packs(struct string_list *include,structstring_list_item*item;structpacked_git*largest=get_largest_active_pack(geometry);FILE*in;-intret;+intret=0;if(!include->nr)-return0;+gotocleanup;cmd.in=-1;cmd.git_cmd=1;
@@ -587,14 +593,19 @@ static int write_midx_included_packs(struct string_list *include,ret=start_command(&cmd);if(ret)-returnret;+gotocleanup;in=xfdopen(cmd.in,"w");for_each_string_list_item(item,include)fprintf(in,"%s\n",item->string);fclose(in);-returnfinish_command(&cmd);+ret=finish_command(&cmd);++cleanup:+child_process_clear(&cmd);++returnret;}intcmd_repack(intargc,constchar**argv,constchar*prefix)
From: Taylor Blau <hidden> Date: 2021-10-21 03:40:16
The `multi-pack-index` builtin dynamically allocates an array of
command-line option for each of its separate modes by calling
add_common_options() to concatante the common options with sub-command
specific ones.
Because this operation allocates a new array, we have to be careful to
remember to free it. We already do this in the repack and write
sub-commands, but verify and expire don't. Rectify this by calling
FREE_AND_NULL as the other modes do.
Signed-off-by: Taylor Blau <redacted>
---
builtin/multi-pack-index.c | 4 ++++
1 file changed, 4 insertions(+)
From: Taylor Blau <hidden> Date: 2021-10-21 03:40:18
To construct the path for a MIDX bitmap, midx_bitmap_filename() calls
get_midx_filename(), and uses the result as an argument to xstrfmt().
But get_midx_filename() also uses xstrfmt, and it assumes that the
caller will free the results.
get_midx_filename() could take a strbuf and write to that, but it would
make more work for every other caller. midx_bitmap_filename() could
store the result of get_midx_filename() in a temporary variable which is
freed before returning.
But that intermediate allocation feels wasteful. Since this function is
simple enough, we can instead "inline" its implementation into the
xstrfmt call itself. That is a little gross, but arguably better than
allocating memory unnecessarily.
Signed-off-by: Taylor Blau <redacted>
---
pack-bitmap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Taylor Blau <hidden> Date: 2021-10-21 03:40:20
test_bitmap_walk() is used to implement `git rev-list --test-bitmap`,
which compares the result of the on-disk bitmaps with ones generated
on-the-fly during a revision walk.
In fa95666a40 (pack-bitmap.c: harden 'test_bitmap_walk()' to check type
bitmaps, 2021-08-24), we hardened those tests to also check the four
special type-level bitmaps, but never freed those bitmaps. We should
have, since each required an allocation when we EWAH-decompressed them.
Free those, plugging that leak, and also free the base (the scratch-pad
bitmap), too.
Signed-off-by: Taylor Blau <redacted>
---
pack-bitmap.c | 6 ++++++
1 file changed, 6 insertions(+)
From: Taylor Blau <hidden> Date: 2021-10-21 03:40:21
The function free_bitmap_index() is somewhat lax in what it frees. There
are two notable examples:
- While it does call kh_destroy_oid_map on the "bitmaps" map (which
maps) commit OIDs to their corresponding bitmaps, the bitmaps
themselves are not freed. Note here that we recycle already-freed
ewah_bitmaps into a pool, but these are handled correctly by
ewah_pool_free().
- We never bother to free the extended index's "positions" map, which
we always allocate in load_bitmap().
Fix both of these.
Signed-off-by: Taylor Blau <redacted>
---
pack-bitmap.c | 8 ++++++++
1 file changed, 8 insertions(+)
From: Taylor Blau <hidden> Date: 2021-10-21 03:40:22
This resolves the remaining leaks uncovered by running t5319 when
building with SANITIZE=leak. All of the leaks below are related to
allocated memory stored in 'struct rev_info' which is never freed.
We could alternatively implement a function which frees all allocated
memory associated with the rev_info struct, but doing this properly is a
bigger undertaking than I want to deal with right now.
Instead, let's just UNLEAK() the few ones that are holding up t5319,
which enables us to mark that test as TEST_PASSES_SANITIZE_LEAK.
Signed-off-by: Taylor Blau <redacted>
---
builtin/log.c | 1 +
builtin/pack-objects.c | 1 +
builtin/rev-list.c | 2 ++
pack-bitmap-write.c | 2 ++
t/t5319-multi-pack-index.sh | 1 +
5 files changed, 7 insertions(+)
From: Taylor Blau <hidden> Date: 2021-10-21 03:40:25
From: Ævar Arnfjörð Bjarmason <redacted>
Fix a bug that's been here since 7cc8f971085 (pack-objects: implement
bitmap writing, 2013-12-21), we did not call stop_progress() if we
reached the early exit in this function.
We could call stop_progress() before we return, but better yet is to
defer calling start_progress() until we need it.
This will matter in a subsequent commit where we BUG(...) out if this
happens, and matters now e.g. because we don't have a corresponding
"region_end" for the progress trace2 event.
Suggested-by: SZEDER Gábor <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Taylor Blau <redacted>
---
pack-bitmap-write.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Eric Sunshine <hidden> Date: 2021-10-21 05:00:59
On Wed, Oct 20, 2021 at 11:39 PM Taylor Blau [off-list ref] wrote:
The function midx.c:verify_midx_file() allocate a MIDX struct by calling
load_multi_pack_index(). But when cleaning up, it calls free() without
freeing any resources associated with the MIDX.
s/allocate/allocates/
quoted hunk
Call the more appropriate close_midx() which does free those resources,
which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.
Signed-off-by: Taylor Blau <redacted>
---
@@ -1689,7 +1689,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag stop_progress(&progress);+cleanup: free(pairs);+ close_midx(m); return verify_midx_error; }
Using the `goto` idiom to ensure cleanup makes perfect sense. For a
few reasons[*], I did spend a some moments wondering if the cognitive
load might be a bit lower by instead adding two close_midx() calls --
one at the early return and one just before the normal return --
rather than using a `goto`, but it's so subjective that it's not worth
worrying about.
FOOTNOTES
[*] First, unlike most cases in which the `goto` jumps over relatively
short blocks of code, the distance in this case between `goto` and the
new label is significant and it's not easy to see at a glance what is
being skipped and how important it might be. Second, `pairs` is still
NULL at the point of the `goto`; I spent extra time checking and
double-checking what free(pairs) was doing in this instance. Finally,
there are enough start/stop-progress calls in this function that it
requires extra mental effort to determine that this `goto` is indeed
safe (at least for the present).
From: Eric Sunshine <hidden> Date: 2021-10-21 05:10:39
On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau [off-list ref] wrote:
The function free_bitmap_index() is somewhat lax in what it frees. There
are two notable examples:
- While it does call kh_destroy_oid_map on the "bitmaps" map (which
maps) commit OIDs to their corresponding bitmaps, the bitmaps
themselves are not freed. Note here that we recycle already-freed
ewah_bitmaps into a pool, but these are handled correctly by
ewah_pool_free().
The parentheses placement seems off; it's not clear what the intent
is. Perhaps either move the closing parenthesis to just before the
comma or drop them altogether.
- We never bother to free the extended index's "positions" map, which
we always allocate in load_bitmap().
Fix both of these.
Signed-off-by: Taylor Blau <redacted>
From: Eric Sunshine <hidden> Date: 2021-10-21 05:12:49
On Wed, Oct 20, 2021 at 11:40 PM Taylor Blau [off-list ref] wrote:
From: Ævar Arnfjörð Bjarmason <redacted>
Fix a bug that's been here since 7cc8f971085 (pack-objects: implement
bitmap writing, 2013-12-21), we did not call stop_progress() if we
reached the early exit in this function.
We could call stop_progress() before we return, but better yet is to
defer calling start_progress() until we need it.
This will matter in a subsequent commit where we BUG(...) out if this
happens, and matters now e.g. because we don't have a corresponding
"region_end" for the progress trace2 event.
Nit: There is no subsequent commit in this series which "BUGS out", so
this comment is confusing.
Suggested-by: SZEDER Gábor <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Taylor Blau <redacted>
From: Ævar Arnfjörð Bjarmason <redacted>
Fix a bug that's been here since 7cc8f971085 (pack-objects: implement
bitmap writing, 2013-12-21), we did not call stop_progress() if we
reached the early exit in this function.
We could call stop_progress() before we return, but better yet is to
defer calling start_progress() until we need it.
This will matter in a subsequent commit where we BUG(...) out if this
happens, and matters now e.g. because we don't have a corresponding
"region_end" for the progress trace2 event.
Suggested-by: SZEDER Gábor <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Taylor Blau <redacted>
---
pack-bitmap-write.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Looks good :) Just a note that this is in
"ab/only-single-progress-at-once" already in case you didn't spot it.
That series is marked for "next?" (with a question mark) by Junio in the
latest what's cooking, so *hint* *hint* for any last minute review :)
The patch contents are from Ævar, but the message is mine. I hope that
he doesn't mind me forging his sign-off here.
I don't mind, and (and maybe we should have an in-tree text file for
this) like I think Jeff King has noted for his inline-posted patches for
discussion you can assume my SOB for anything I post here on list
(except noted otherwise) in terms of copyright.
I tried to separate these out based on their respective areas. The last 10% are
all from leaking memory in the rev_info structure, which I punted on for now by
just UNLEAK()-ing it. That's all done in the last patch. If we choose to take
that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a
little gross, so I'm happy to leave it out if others would prefer.
I'll defer to you, but I've got a mild preference for keeping it out. An
upcoming series of patches I'm submitting (I'm waiting on the current
leak fixes to land) will fix most of those rev_info leaks. So not having
UNLEAK() markings in-flight makes things a bit easier to manage.
`git repack` invokes a handful of child processes: one to write the
actual pack, and optionally ones to repack promisor objects and update
the MIDX.
In none of these cases do we bother to call child_process_clear(), which
frees the memory associated with each child's arguments and environment.
In order to do so, tweak each function that spawns a child process to
have a `cleanup` label that we always visit before returning from each
function. Then, make sure that we call child_process_clear() as a part
of that label.
Signed-off-by: Taylor Blau <redacted>
---
builtin/repack.c | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
nit: "ret" is short for "return" which makes me think "this will
be used as 'return ret;'" but we don't do that. Instead, we use
it as the _result_ of an inner call:
- if (finish_command(&cmd))
+ ret = finish_command(&cmd);
+
+cleanup:
+ child_process_clear(&cmd);
+
+ if (ret)
die(_("could not finish pack-objects to repack promisor objects"));
For that reason, I would rename this to "res" or "result".
Here is a topic that Ævar got me interested when he mentioned that t5319 is
leaky[1].
This is the result of trying to get t5319 to pass when compiling with
SANITIZE=leak. About 50% of the fixes are in the MIDX code, another 40% are in
the pack-bitmap code, and the remaining 10% are sort of random.
I tried to separate these out based on their respective areas. The last 10% are
all from leaking memory in the rev_info structure, which I punted on for now by
just UNLEAK()-ing it. That's all done in the last patch. If we choose to take
that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a
little gross, so I'm happy to leave it out if others would prefer.
This is based on tb/fix-midx-rename-while-mapped, with a back-merge of
js/windows-ci-path-fix in order to appease CI. It also contains a little bit of
overlap with a patch[2] from Ævar in another series, which he and I discovered
independently. I cherry-picked his patch, since I haven't seen much action on
that series lately.
In my reading, I only found one nitpick that wasn't already caught by
other reviewers. Feel free to ignore my comment as it was just something
that caused me to pause while reading, but isn't that unusual.
Thanks,
-Stolee
From: Taylor Blau <hidden> Date: 2021-10-22 04:39:28
On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
On Wed, Oct 20 2021, Taylor Blau wrote:
quoted
I tried to separate these out based on their respective areas. The last 10% are
all from leaking memory in the rev_info structure, which I punted on for now by
just UNLEAK()-ing it. That's all done in the last patch. If we choose to take
that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a
little gross, so I'm happy to leave it out if others would prefer.
I'll defer to you, but I've got a mild preference for keeping it out. An
upcoming series of patches I'm submitting (I'm waiting on the current
leak fixes to land) will fix most of those rev_info leaks. So not having
UNLEAK() markings in-flight makes things a bit easier to manage.
If you don't mind, I think keeping this in (as a way to verify that we
really did make t5319 leak-free) may be good for reviewers. When you
clean these areas up, you'll have to remember to remove those UNLEAK()s,
but hopefully they produce conflicts with your in-flight work that serve
as not-too-annoying reminders.
I would certainly rather not have to UNLEAK() those at all, so I am very
excited to see a series from you which handles freeing these resources
appropriately. It was just too big a bite for me to chew off when
preparing this quick series.
Thanks,
Taylor
On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
On Wed, Oct 20 2021, Taylor Blau wrote:
quoted
I tried to separate these out based on their respective areas. The last 10% are
all from leaking memory in the rev_info structure, which I punted on for now by
just UNLEAK()-ing it. That's all done in the last patch. If we choose to take
that last patch, then t5319 passes even under SANITIZE=leak builds. But it's a
little gross, so I'm happy to leave it out if others would prefer.
I'll defer to you, but I've got a mild preference for keeping it out. An
upcoming series of patches I'm submitting (I'm waiting on the current
leak fixes to land) will fix most of those rev_info leaks. So not having
UNLEAK() markings in-flight makes things a bit easier to manage.
If you don't mind, I think keeping this in (as a way to verify that we
really did make t5319 leak-free) may be good for reviewers. When you
clean these areas up, you'll have to remember to remove those UNLEAK()s,
but hopefully they produce conflicts with your in-flight work that serve
as not-too-annoying reminders.
I would certainly rather not have to UNLEAK() those at all, so I am very
excited to see a series from you which handles freeing these resources
appropriately. It was just too big a bite for me to chew off when
preparing this quick series.
Having looked at this a bit more carefully I've upgraded that a bit from
"I'll defer to you" to strongly objecting to this specific approach (but
read to the end, I think you can have your cake and eat it too).
I just glanced at this series in my previous pass-over, and evidently
didn't read the CL to the end. I thought based on the commit subject
that it was just a change to some midx code impacting t5319.
But it's a bigger change across the whole test suite than that.
If you run:
rm .prove; GIT_SKIP_TESTS=t0027 prove -j8 --state=save t[0-9]*.sh :: --immediate
Without this patch (t0027 will take forever), and then:
GIT_SKIP_TESTS=t0027 prove -j8 --state=failed
You'll see that (I use GNU screen's logging feature to get the output)
we made these tests pass, not just your t5319:
$ grep -w ok screenlog.11
t0056-git-C.sh .................................... ok
t1060-object-corruption.sh ........................ ok
t4212-log-corrupt.sh .............................. ok
t4207-log-decoration-colors.sh .................... ok
t5313-pack-bounds-checks.sh ....................... ok
t5506-remote-groups.sh ............................ ok
t5513-fetch-track.sh .............................. ok
t5319-multi-pack-index.sh ......................... ok
t5532-fetch-proxy.sh .............................. ok
t5900-repo-selection.sh ........................... ok
t6011-rev-list-with-bad-commit.sh ................. ok
t6100-rev-list-in-order.sh ........................ ok
t6114-keep-packs.sh ............................... ok
t6131-pathspec-icase.sh ........................... ok
t6003-rev-list-topo-order.sh ...................... ok
t7702-repack-cyclic-alternate.sh .................. ok
t9108-git-svn-glob.sh ............................. ok
t9109-git-svn-multi-glob.sh ....................... ok
t9127-git-svn-partial-rebuild.sh .................. ok
t9133-git-svn-nested-git-repo.sh .................. ok
t9168-git-svn-partially-globbed-names.sh .......... ok
I've got subsequent patches on top of what's now in-flight that fix
those bigger leaks incrementally, and as part of that add
"TEST_PASSES_SANITIZE_LEAK=true" to /all/ the relevant passing
tests.
I.e. there isn't a 1=1 correspondance between all current passing tests
and "TEST_PASSES_SANITIZE_LEAK=true" markings (some are still left
unmarked). But there has been a 1=1 mapping between freshly pasing tests
as a result of leak fixes and tests that get that
"TEST_PASSES_SANITIZE_LEAK=true" marking.
I think it helps a lot to review those patches & assess their impact if
code changes can be paired with relevant test suite changes, which isn't
the case if we've got simultaneous UNLEAK() markings for major leaks
in-flight.
So the practical effect of that is that I'll need to rebase all that,
change my testing setup to test those one-at-at-time with "git rebase -i
--exec" before submission (which I do anyway), but now with some
selective revert of an earlier UNLEAK() to assert that I'm still fixing
the things I expected.
Then either drop the parts of the commit messages that say "this fixes
leak XYZ, making tests A, B, C pass" to omit any mention of "A, B, C",
or reword it for the earlier now-landed UNLEAK() markings.
These are also just the fully passing tests I ran as a one-off for this
E-Mail. For those I do more exhaustive runs where I'll e.g. spot if a
test goes from N failing to N-X, and note it if that gets us much closer
to 0.
But on the "cake and eat it too" front I also realize that it sucks to
not be able to mark tests you made leak-free as passing just because
I've got some larger more general leak fixes planned, and even if none
of your code leaks, it might just be "git log" or whatever.
But we can just use LSAN_OPTIONS with a suppressions file for that. This
diff below (assume my SOB yadayada) makes your tests pass if I eject the
11/11 and replace that with this:
I think that would be a much better approach here, and would really
prefer if we went for some version of that if you really want to test
these right away with the linux-leaks job.
I think the better thing to do here is to just leave it, i.e. we've got
tons of these tests that don't leak themselves, but leak due to "git
log" or whatever already, but anyway, if what you're doing in
t5319-multi-pack-index.sh doesn't cause much more work for me as noted
above I really don't mind.
If you want to pick that approach up and run with it I think it would
probably make sense to factor that suggested test_expect_success out
into a function in test-lib-functions.sh or whatever, and call it as
e.g.:
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
declare_known_leaks <<-\EOF
add_rev_cmdline
[...]
EOF
Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for
you.
Doing it that way would be less boilerplate for each test that wants it,
and is also more likely to work with other non-LSAN leak appoaches,
i.e. as long as something can take a list of lines matching stack traces
we can feed that to that leak checker's idea of a whitelist.
Extend the SANITIZE=leak testing mode added in 956d2e4639b (tests: add
a test mode for SANITIZE=leak, run it in CI, 2021-09-23) to optionally
be able to add a "suppressions" file to the $LSAN_OPTIONS.
This allows for marking tests as passing with
"TEST_PASSES_SANITIZE_LEAK=true" when they still have failure due more
general widespread memory leaks, such as from the "git log" family of
commands. We can now mark the "git -C" tests as passing.
For getting specific tests to pass this is preferable to using
UNLEAK() in these codepaths, as I'll have fixes for those leaks soon,
and being able to atomically mark relevant tests as passing with
"TEST_PASSES_SANITIZE_LEAK=true" helps to explain those changes. See
[1] for more details.
1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
On Fri, Oct 22 2021, Ævar Arnfjörð Bjarmason wrote:
On Fri, Oct 22 2021, Taylor Blau wrote:
quoted
On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
On Wed, Oct 20 2021, Taylor Blau wrote:
[...]
If you want to pick that approach up and run with it I think it would
probably make sense to factor that suggested test_expect_success out
into a function in test-lib-functions.sh or whatever, and call it as
e.g.:
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
declare_known_leaks <<-\EOF
add_rev_cmdline
[...]
EOF
Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for
you.
Doing it that way would be less boilerplate for each test that wants it,
and is also more likely to work with other non-LSAN leak appoaches,
i.e. as long as something can take a list of lines matching stack traces
we can feed that to that leak checker's idea of a whitelist.
I just went ahead and hacked that up. If you're OK with that approach
it would really help reduce the work for leak changes I've got
planned, and as noted gives you the end-state of a passing 5319.
I don't know if it makes more sense for you to base on top of this
if/when Junio picks it up, or to integrate it into your series
etc. Maybe Junio will chime in ...
t/t0056-git-C.sh | 4 ++++
t/test-lib-functions.sh | 42 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+)
@@ -2,7 +2,11 @@test_description='"-C <path>" option and its effects on other path-related options'+TEST_PASSES_SANITIZE_LEAK=true ../test-lib.sh+todo_leaks<<-\EOF+^cmd_log_init_finish$+EOF test_expect_success'"git -C <path>" runs git from the directory <path>''test_create_repodir1&&
@@ -243,6 +243,48 @@ debug () {done}+# Declare known "general" memory leaks, for use with TEST_PASSES_SANITIZE_LEAK=true.+#+# Matches lines in a stack trace that leaks. Intended for+# LSAN_OPTIONS, but the format is intended to be easy to use with+# other leak checkers, so the "leak:" prefix is omitted (and added for+# you).+#+# Use it immediately after sourcing test-lib.sh (or equivalent), and+# after a "TEST_PASSES_SANITIZE_LEAK=true" has been set. E.g:+#+# TEST_PASSES_SANITIZE_LEAK=true+# . ./test-lib.sh+# todo_leaks <<-\EOF+# ^cmd_log_init_finish$+# EOF+#+# The "^" and "$" anchors don't suggest full regex syntax support,+# that's the only anchoring (or other metacharacter) understood by+# LSAN_OPTIONS,.+#+# See+# https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer#suppressions+# for the relevant LSAN_OPTIONS documentation.+todo_leaks(){+if!test_have_prereqSANITIZE_LEAK+then+return0+fi++# Try not to interfere with any test logic+suppressions=.lsan-suppressions.txt+iftest-d.git+then+suppressions=".git/$suppressions"+fi+suppressions="$PWD/$suppressions"++sed's/^/leak:/'>"$suppressions"&&+LSAN_OPTIONS="$LSAN_OPTIONS:suppressions=\"$suppressions\""&&+exportLSAN_OPTIONS+}+# Usage: test_commit [options] <message> [<file> [<contents> [<tag>]]]# -C <dir>:# Run all git commands in directory <dir>
From: Taylor Blau <hidden> Date: 2021-10-26 20:23:20
On Fri, Oct 22, 2021 at 12:32:17PM +0200, Ævar Arnfjörð Bjarmason wrote:
Extend the SANITIZE=leak testing mode added in 956d2e4639b (tests: add
a test mode for SANITIZE=leak, run it in CI, 2021-09-23) to optionally
be able to add a "suppressions" file to the $LSAN_OPTIONS.
This allows for marking tests as passing with
"TEST_PASSES_SANITIZE_LEAK=true" when they still have failure due more
general widespread memory leaks, such as from the "git log" family of
commands. We can now mark the "git -C" tests as passing.
For getting specific tests to pass this is preferable to using
UNLEAK() in these codepaths, as I'll have fixes for those leaks soon,
and being able to atomically mark relevant tests as passing with
"TEST_PASSES_SANITIZE_LEAK=true" helps to explain those changes. See
[1] for more details.
1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
On Fri, Oct 22 2021, Ævar Arnfjörð Bjarmason wrote:
quoted
On Fri, Oct 22 2021, Taylor Blau wrote:
quoted
On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
On Wed, Oct 20 2021, Taylor Blau wrote:
[...]
quoted
If you want to pick that approach up and run with it I think it would
probably make sense to factor that suggested test_expect_success out
into a function in test-lib-functions.sh or whatever, and call it as
e.g.:
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
declare_known_leaks <<-\EOF
add_rev_cmdline
[...]
EOF
Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for
you.
Doing it that way would be less boilerplate for each test that wants it,
and is also more likely to work with other non-LSAN leak appoaches,
i.e. as long as something can take a list of lines matching stack traces
we can feed that to that leak checker's idea of a whitelist.
I just went ahead and hacked that up. If you're OK with that approach
it would really help reduce the work for leak changes I've got
planned, and as noted gives you the end-state of a passing 5319.
I don't know if it makes more sense for you to base on top of this
if/when Junio picks it up, or to integrate it into your series
etc. Maybe Junio will chime in ...
Hmm. This seems neat, but I haven't been able to get it to work without
encountering a rather annoying bug along the way.
Here is the preamble of my modified t5319 to include all of the leaks I
found in the previous round (but decided not to fix):
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
todo_leaks <<-\EOF
^add_rev_cmdline$
^cmd_log_init_finish$
^prepare_revision_walk$
^prep_parse_options$
EOF
So running that when git is compiled with SANITIZE=leak *should* work,
but instead produces this rather annoying leak detection after t5319.7:
=================================================================
==1785298==ERROR: LeakSanitizer: detected memory leaks
Indirect leak of 41 byte(s) in 3 object(s) allocated from:
#0 0x7f2f2f866db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
#1 0x7f2f2f64b4aa in __GI___strdup string/strdup.c:42
-----------------------------------------------------
Suppressions used:
count bytes template
1 576 ^add_rev_cmdline$
-----------------------------------------------------
SUMMARY: LeakSanitizer: 41 byte(s) leaked in 3 allocation(s).
Aborted
Huh? Looking past __GI___strdup (which I assume is just a
symbolification failure on ASan's part), who calls strdup()? That trace
is annoyingly incomplete, and doesn't really give us anything to go off
of.
It does seem to remind me of this:
https://github.com/google/sanitizers/issues/148
though that issue goes in so many different directions that I'm not sure
whether it really is the same issue or not. In any case, that leak
*still* shows up even when suppressing xmalloc() and xrealloc(), so I
almost think that it's a leak within ASan itself.
But most interesting is that those calls go away when I stop setting
`suppressions` in $LSAN_OPTIONS by dropping the call to your todo_leaks.
So this all feels like a bug in ASan to me. I'm curious if it works on
your system, but in the meantime I think the best path forward is to
drop the last patch of my original series (the one with the three
UNLEAK() calls) and to avoid relying on this patch for the time being.
Thanks,
Taylor
From: Jeff King <hidden> Date: 2021-10-26 21:11:31
On Tue, Oct 26, 2021 at 04:23:14PM -0400, Taylor Blau wrote:
So this all feels like a bug in ASan to me. I'm curious if it works on
your system, but in the meantime I think the best path forward is to
drop the last patch of my original series (the one with the three
UNLEAK() calls) and to avoid relying on this patch for the time being.
Bugs aside, I'd much rather see UNLEAK() annotations than external ones,
for all the reasons we introduced UNLEAK() in the first place:
- it keeps the annotations near the code. Yes, that creates conflicts
when the code is changed (or the leak is actually fixed), but that's
a feature. It keeps them from going stale.
- leak-checkers only know where things are allocated, not who is
supposed to own them. So you're often annotating the wrong thing;
it's not a strdup() call which is buggy and leaking, but the
function five layers up the stack which was supposed to take
ownership and didn't.
And if we avoid any annotation bugs by doing so, that's icing on the
cake. :)
-Peff
From: Taylor Blau <hidden> Date: 2021-10-26 21:30:49
On Tue, Oct 26, 2021 at 05:11:29PM -0400, Jeff King wrote:
On Tue, Oct 26, 2021 at 04:23:14PM -0400, Taylor Blau wrote:
quoted
So this all feels like a bug in ASan to me. I'm curious if it works on
your system, but in the meantime I think the best path forward is to
drop the last patch of my original series (the one with the three
UNLEAK() calls) and to avoid relying on this patch for the time being.
Bugs aside, I'd much rather see UNLEAK() annotations than external ones,
for all the reasons we introduced UNLEAK() in the first place:
- it keeps the annotations near the code. Yes, that creates conflicts
when the code is changed (or the leak is actually fixed), but that's
a feature. It keeps them from going stale.
I agree completely. I noted as much in my message here:
https://lore.kernel.org/git/YXJAfICQN8s5Gm7s@nand.local/
but Ævar made it sound like his work would be made much easier without
the conflict. Since I'm not in any kind of rush to make t5319 leak-free,
I figured that queueing the parts of that series that wouldn't conflict
with Ævar's ongoing work would be a net-positive.
- leak-checkers only know where things are allocated, not who is
supposed to own them. So you're often annotating the wrong thing;
it's not a strdup() call which is buggy and leaking, but the
function five layers up the stack which was supposed to take
ownership and didn't.
TBH, I find this much more compelling. Either way, I don't feel strongly
enough to deviate from v2 much, and I don't want to create more work for
Ævar along the way.
Thanks,
Taylor
From: Jeff King <hidden> Date: 2021-10-26 21:48:53
On Tue, Oct 26, 2021 at 05:30:47PM -0400, Taylor Blau wrote:
quoted
Bugs aside, I'd much rather see UNLEAK() annotations than external ones,
for all the reasons we introduced UNLEAK() in the first place:
- it keeps the annotations near the code. Yes, that creates conflicts
when the code is changed (or the leak is actually fixed), but that's
a feature. It keeps them from going stale.
I agree completely. I noted as much in my message here:
https://lore.kernel.org/git/YXJAfICQN8s5Gm7s@nand.local/
but Ævar made it sound like his work would be made much easier without
the conflict. Since I'm not in any kind of rush to make t5319 leak-free,
I figured that queueing the parts of that series that wouldn't conflict
with Ævar's ongoing work would be a net-positive.
Yeah, to be clear, if there's work in progress in an area, then _not_
annotating it (with either method) is perfectly fine with me in the
meantime.
-Peff
On Fri, Oct 22, 2021 at 12:32:17PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
Extend the SANITIZE=leak testing mode added in 956d2e4639b (tests: add
a test mode for SANITIZE=leak, run it in CI, 2021-09-23) to optionally
be able to add a "suppressions" file to the $LSAN_OPTIONS.
This allows for marking tests as passing with
"TEST_PASSES_SANITIZE_LEAK=true" when they still have failure due more
general widespread memory leaks, such as from the "git log" family of
commands. We can now mark the "git -C" tests as passing.
For getting specific tests to pass this is preferable to using
UNLEAK() in these codepaths, as I'll have fixes for those leaks soon,
and being able to atomically mark relevant tests as passing with
"TEST_PASSES_SANITIZE_LEAK=true" helps to explain those changes. See
[1] for more details.
1. https://lore.kernel.org/git/211022.86sfwtl6uj.gmgdl@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
On Fri, Oct 22 2021, Ævar Arnfjörð Bjarmason wrote:
quoted
On Fri, Oct 22 2021, Taylor Blau wrote:
quoted
On Thu, Oct 21, 2021 at 01:50:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
On Wed, Oct 20 2021, Taylor Blau wrote:
[...]
quoted
If you want to pick that approach up and run with it I think it would
probably make sense to factor that suggested test_expect_success out
into a function in test-lib-functions.sh or whatever, and call it as
e.g.:
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
declare_known_leaks <<-\EOF
add_rev_cmdline
[...]
EOF
Then pipe it through sed 's/^/leak:/' and have it set LSAN_OPTIONS for
you.
Doing it that way would be less boilerplate for each test that wants it,
and is also more likely to work with other non-LSAN leak appoaches,
i.e. as long as something can take a list of lines matching stack traces
we can feed that to that leak checker's idea of a whitelist.
I just went ahead and hacked that up. If you're OK with that approach
it would really help reduce the work for leak changes I've got
planned, and as noted gives you the end-state of a passing 5319.
I don't know if it makes more sense for you to base on top of this
if/when Junio picks it up, or to integrate it into your series
etc. Maybe Junio will chime in ...
Hmm. This seems neat, but I haven't been able to get it to work without
encountering a rather annoying bug along the way.
Here is the preamble of my modified t5319 to include all of the leaks I
found in the previous round (but decided not to fix):
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
todo_leaks <<-\EOF
^add_rev_cmdline$
^cmd_log_init_finish$
^prepare_revision_walk$
^prep_parse_options$
EOF
So running that when git is compiled with SANITIZE=leak *should* work,
but instead produces this rather annoying leak detection after t5319.7:
=================================================================
==1785298==ERROR: LeakSanitizer: detected memory leaks
Indirect leak of 41 byte(s) in 3 object(s) allocated from:
#0 0x7f2f2f866db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
#1 0x7f2f2f64b4aa in __GI___strdup string/strdup.c:42
-----------------------------------------------------
Suppressions used:
count bytes template
1 576 ^add_rev_cmdline$
-----------------------------------------------------
SUMMARY: LeakSanitizer: 41 byte(s) leaked in 3 allocation(s).
Aborted
Huh? Looking past __GI___strdup (which I assume is just a
symbolification failure on ASan's part), who calls strdup()? That trace
is annoyingly incomplete, and doesn't really give us anything to go off
of.
It does seem to remind me of this:
https://github.com/google/sanitizers/issues/148
though that issue goes in so many different directions that I'm not sure
whether it really is the same issue or not. In any case, that leak
*still* shows up even when suppressing xmalloc() and xrealloc(), so I
almost think that it's a leak within ASan itself.
But most interesting is that those calls go away when I stop setting
`suppressions` in $LSAN_OPTIONS by dropping the call to your todo_leaks.
So this all feels like a bug in ASan to me. I'm curious if it works on
your system, but in the meantime I think the best path forward is to
drop the last patch of my original series (the one with the three
UNLEAK() calls) and to avoid relying on this patch for the time being.
There are similar cases where LSAN doesn't provide as meaningful of a
stacktrace as valgrind, sometimes when tracing leaks I get a relatively
bad stacktrace like that ending in some __GI___*<stdlib-name>
function. I'll usually compile without SANITIZE=leak and just run a
slower:
valgrind --leak-check=full --show-leak-kinds=all
In this case however it's not a bug or bad leak tracing, but an artifact
of us using these stacktrace exclusions.
If you run this manually you'll see:
$ ~/g/git/git -c core.multiPackIndex=false rev-list --objects --all
cd0747a9352b58d112f0010134351efc7bbad4a6
[... snipped output ...]
=================================================================
==58023==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 576 byte(s) in 1 object(s) allocated from:
#0 0x7fd0bfae50c5 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:82
#1 0x5637b3bd9163 in xrealloc /home/avar/g/git/wrapper.c:126
#2 0x5637b3b6a1e4 in add_rev_cmdline /home/avar/g/git/revision.c:1482
#3 0x5637b3b6a412 in handle_one_ref /home/avar/g/git/revision.c:1534
#4 0x5637b3b49c4e in do_for_each_ref_helper /home/avar/g/git/refs.c:1483
#5 0x5637b3b54afc in do_for_each_repo_ref_iterator refs/iterator.c:418
#6 0x5637b3b49cc8 in do_for_each_ref /home/avar/g/git/refs.c:1498
#7 0x5637b3b49d07 in refs_for_each_ref /home/avar/g/git/refs.c:1504
#8 0x5637b3b6a563 in handle_refs /home/avar/g/git/revision.c:1578
#9 0x5637b3b6e0e7 in handle_revision_pseudo_opt /home/avar/g/git/revision.c:2597
#10 0x5637b3b6e9d5 in setup_revisions /home/avar/g/git/revision.c:2738
#11 0x5637b39ebc58 in cmd_rev_list builtin/rev-list.c:550
#12 0x5637b3932a89 in run_builtin /home/avar/g/git/git.c:461
#13 0x5637b3932e98 in handle_builtin /home/avar/g/git/git.c:713
#14 0x5637b3933105 in run_argv /home/avar/g/git/git.c:780
#15 0x5637b39335ae in cmd_main /home/avar/g/git/git.c:911
#16 0x5637b3a1a898 in main /home/avar/g/git/common-main.c:52
#17 0x7fd0bf860d09 in __libc_start_main ../csu/libc-start.c:308
Indirect leak of 41 byte(s) in 3 object(s) allocated from:
#0 0x7fd0bfae4db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
#1 0x7fd0bf8c7e4a in __GI___strdup string/strdup.c:42
SUMMARY: LeakSanitizer: 617 byte(s) leaked in 4 allocation(s).
Which is why I put the "strdup" there, but forgot to tell you when I
submitted the real PATCH version that that one shouldn't have the ^$
anchoring.
FWIW this will work on top of your series:
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index a3c72b68f7c..891de720b2c 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -2,6 +2,17 @@
test_description='multi-pack-indexes'
. ./test-lib.sh
+todo_leaks <<-\EOF
+^add_rev_cmdline$
+^cmd_log_init_finish$
+^prep_parse_options$
+^prepare_revision_walk$
+^start_progress_delay$
+
+# An indirect leak reported because we excluded the leaks above
+strdup$
+EOF
+
GIT_TEST_MULTI_PACK_INDEX=0
objdir=.git/objects
On Tue, Oct 26 2021, Jeff King wrote:
It seems that I've gotten what I wanted from this side-thread, yay!
Thanks both, not to pile on, but just to clarify:
On Tue, Oct 26, 2021 at 04:23:14PM -0400, Taylor Blau wrote:
quoted
So this all feels like a bug in ASan to me. I'm curious if it works on
your system, but in the meantime I think the best path forward is to
drop the last patch of my original series (the one with the three
UNLEAK() calls) and to avoid relying on this patch for the time being.
Bugs aside, I'd much rather see UNLEAK() annotations than external ones,
for all the reasons we introduced UNLEAK() in the first place:
- it keeps the annotations near the code. Yes, that creates conflicts
when the code is changed (or the leak is actually fixed), but that's
a feature. It keeps them from going stale.
I fully agree with you in general for "good" uses of UNLEAK(). E.g. consider:
struct strbuf buf = xmalloc(...);
[...]
UNLEAK(buf);
If I was fixing leaks in that sort of code what I pointed out downthread
in [1] wouldn't apply.
So to clarify, I'm not asking in [1] that UNLEAK() not be used at all
while we have in-flight leak fixes. I.e. I'd run into a textual
conflict, but that would be trivial to resolve, and obvious what the
semantic & textual conflict was.
Rather, it's not marking specific leaks, but UNLEAK() on a container
struct that's problematic.
Depending on how they're used those structs may or may not leak. So
e.g. Taylor's upthread 11/11[2] contained:
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 36cb909eba..df3811e763 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -549,6 +549,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
argc = setup_revisions(argc, argv, &revs, &s_r_opt);
+ UNLEAK(revs);
+
memset(&info, 0, sizeof(info));
info.revs = &revs;
if (revs.bisect)
Which if you on master replace that with:
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 36cb909ebaa..3cce87e0eb7 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -548,6 +548,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
revs.do_not_die_on_missing_tree = 1;
argc = setup_revisions(argc, argv, &revs, &s_r_opt);
+ BUG("using this?");
memset(&info, 0, sizeof(info));
info.revs = &revs;
You'll run into a failure with:
GIT_TEST_PASSING_SANITIZE_LEAK=true ./t0002-gitfile.sh
I.e. our existing leak tests already take that codepath and don't run
into a leak from using "revs".
So we wouldn't just be marking a known specific leak, but hiding all
leaks & non-leaks in container from the top-level, and thus hide
potential regressions, an addition to attaining the end-goal of marking
some specific test as passing.
Now, that specific example isn't very useful right now, since we don't
have a release_revisions() at all, but e.g. the first batch of fixes
I've got for the revisions.[ch] leak fix most common cases (e.g. the
pending array), but not some obscure ones.
Being able to incrementally mark those leaks as fixed on a test-by-test
basis has the advantage over UNLEAK() that we won't have regressions
while we're not at a 100% leak free (at which point we could remove the
UNLEAK()).
- leak-checkers only know where things are allocated, not who is
supposed to own them. So you're often annotating the wrong thing;
it's not a strdup() call which is buggy and leaking, but the
function five layers up the stack which was supposed to take
ownership and didn't.
From: Jeff King <hidden> Date: 2021-10-27 09:06:23
On Wed, Oct 27, 2021 at 10:04:17AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
- it keeps the annotations near the code. Yes, that creates conflicts
when the code is changed (or the leak is actually fixed), but that's
a feature. It keeps them from going stale.
I fully agree with you in general for "good" uses of UNLEAK(). E.g. consider:
struct strbuf buf = xmalloc(...);
[...]
UNLEAK(buf);
If I was fixing leaks in that sort of code what I pointed out downthread
in [1] wouldn't apply.
I'm OK with such a use of UNLEAK(), though of course you could probably
just free the buffer in that instance.
But...
So to clarify, I'm not asking in [1] that UNLEAK() not be used at all
while we have in-flight leak fixes. I.e. I'd run into a textual
conflict, but that would be trivial to resolve, and obvious what the
semantic & textual conflict was.
Rather, it's not marking specific leaks, but UNLEAK() on a container
struct that's problematic.
...that's the whole point of UNLEAK(), IMHO. You know that the container
struct is leaked, but you don't care because the program is about to
exit, or there's no easy way to avoid the leak.
So it's not the "container" element, but rather it can be a problem if
people annotate too broadly (you will miss some leaks). In the case of
rev_info, there is no way to _not_ leak right now, because it has no
cleanup function. In the long run we should probably add one. But in the
meantime, annotating them so we can find the _interesting_ leaks is
worth doing (and that was the whole point of adding UNLEAK in the first
place.
And I think the UNLEAK() calls in Taylor's patch are fine in that sense.
While yes, _some_ runs of those commands will not leak, the point is to
say that when they do leak, they are not interesting. And they are not
interesting because that rev_info is in scope until the program exits,
so anything it points to is only leaked at a moment where we no longer
care.
So we wouldn't just be marking a known specific leak, but hiding all
leaks & non-leaks in container from the top-level, and thus hide
potential regressions, an addition to attaining the end-goal of marking
some specific test as passing.
I'd argue it _is_ a known specific leak. It is rev_info going out of
scope that causes the leak, not rev_info holding on to memory in things
like its pending array.
Now those can be interesting, too (if it no longer needs the memory, can
it perhaps get rid of it). But IMHO all of that is pretty secondary to
clearing the noise so you can find "true" leaks (ones where some
sub-function really is allocating and then losing the pointer entirely,
especially if it's called an arbitrary number of times).
quoted
- leak-checkers only know where things are allocated, not who is
supposed to own them. So you're often annotating the wrong thing;
it's not a strdup() call which is buggy and leaking, but the
function five layers up the stack which was supposed to take
ownership and didn't.
As noted in [3] this case is because the LSAN suppressions list was in
play, so we excluded the meaningful part of the stack trace (which is
shown in that mail).
I don't think that's true at all. The annotations you showed in that
message, for example, are pointing at add_rev_cmdline(). But that is
_not_ the source of the leak, nor where it should be fixed. It is a
necessary part of how rev_info works. The leak is when the rev_info goes
out of scope without anybody cleaning it up.
Hrm, now that I think about it I think that the cases where I had to
resort to valgrind to get around such crappy stacktraces (when that was
all I got, even without suppressions) were probably all because there
was an UNLEAK() in play...
I don't see how UNLEAK() would impact stack traces. It should either
make something not-leaked-at-all (in which case LSan will no longer
mention it), or it does nothing (it throws some wasted memory into a
structure which is itself not leaked).
-Peff
On Wed, Oct 27, 2021 at 10:04:17AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
quoted
- it keeps the annotations near the code. Yes, that creates conflicts
when the code is changed (or the leak is actually fixed), but that's
a feature. It keeps them from going stale.
I fully agree with you in general for "good" uses of UNLEAK(). E.g. consider:
struct strbuf buf = xmalloc(...);
[...]
UNLEAK(buf);
If I was fixing leaks in that sort of code what I pointed out downthread
in [1] wouldn't apply.
I'm OK with such a use of UNLEAK(), though of course you could probably
just free the buffer in that instance.
But...
quoted
So to clarify, I'm not asking in [1] that UNLEAK() not be used at all
while we have in-flight leak fixes. I.e. I'd run into a textual
conflict, but that would be trivial to resolve, and obvious what the
semantic & textual conflict was.
Rather, it's not marking specific leaks, but UNLEAK() on a container
struct that's problematic.
...that's the whole point of UNLEAK(), IMHO. You know that the container
struct is leaked, but you don't care because the program is about to
exit, or there's no easy way to avoid the leak.
So it's not the "container" element, but rather it can be a problem if
people annotate too broadly (you will miss some leaks). In the case of
rev_info, there is no way to _not_ leak right now, because it has no
cleanup function.
It doesn't have one, but there are uses of setup_revisions() and
rev_info usage that don't leak, as that builtin/rev-list.c case shows.
I mean, in that case it's not doing much of anything, but at least we
test that setup_revisions() itself doesn't leak right now, but wouldn't
with UNLEAK().
In the long run we should probably add one.
After the current in-flight patches I've got making diff_free() a bit
more useful, and then a revisions_release() that'll handle most cases,
which gets us started on mass-marking the tests as leak free.
So just FWIW I'm not saying "hey can we hold off on that UNLEAK() for
far future xyz", but for a thing I've got queued up that I'd rather not
start rewriting...
[...]But in the meantime, annotating them so we can find the
_interesting_ leaks is worth doing (and that was the whole point of
adding UNLEAK in the first place.
And I think the UNLEAK() calls in Taylor's patch are fine in that sense.
While yes, _some_ runs of those commands will not leak, the point is to
say that when they do leak, they are not interesting. And they are not
interesting because that rev_info is in scope until the program exits,
so anything it points to is only leaked at a moment where we no longer
care.
I don't per-se care about leaks at program exit either, but am making
them leak-free as a proxy for making the relevant APIs useful for
persistent use.
So I think the historical approach of just sprinkling UNLEAK() since
we're exiting anyway is taking too narrow of a view.
In that case if we'd like to see if revisions.[ch] is leak-free we'd
need parallel test-helper tests (or whatever) for all its features, or
we can just piggy-back on every "git log" invocation in the test
suite...
quoted
So we wouldn't just be marking a known specific leak, but hiding all
leaks & non-leaks in container from the top-level, and thus hide
potential regressions, an addition to attaining the end-goal of marking
some specific test as passing.
I'd argue it _is_ a known specific leak. It is rev_info going out of
scope that causes the leak, not rev_info holding on to memory in things
like its pending array.
Now those can be interesting, too (if it no longer needs the memory, can
it perhaps get rid of it). But IMHO all of that is pretty secondary to
clearing the noise so you can find "true" leaks (ones where some
sub-function really is allocating and then losing the pointer entirely,
especially if it's called an arbitrary number of times).
I wasn't trying to get into the philosophical discussion of whether a
struct or a struct it contains can be said to be the "real" source of a
leak :)
Yeah, I agree that from that POV there's no difference.
I'm merely referring to the practical aspect of patches I've got queued
up, which do things like fix all leaks of rev_info.pending, mark tests
that now pass, fix another rev_info.whatever, mark tests etc.
For rev_info in particular it's a bit iffy to UNLEAK() it, since it also
contains pointers to stuff it doesn't "own", sometimes things are
allocated and handed off to it, sometimes for a while, sometimes
permanently.
So by UNLEAK()ing it you're not just unleaking "its" memory, but
potentially hiding leaks in APIs that need to interact with it in that
way. E.g. mailmap.c is one of those cases.
quoted
quoted
- leak-checkers only know where things are allocated, not who is
supposed to own them. So you're often annotating the wrong thing;
it's not a strdup() call which is buggy and leaking, but the
function five layers up the stack which was supposed to take
ownership and didn't.
As noted in [3] this case is because the LSAN suppressions list was in
play, so we excluded the meaningful part of the stack trace (which is
shown in that mail).
I don't think that's true at all. The annotations you showed in that
message, for example, are pointing at add_rev_cmdline(). But that is
_not_ the source of the leak, nor where it should be fixed. It is a
necessary part of how rev_info works. The leak is when the rev_info goes
out of scope without anybody cleaning it up.
quoted
Hrm, now that I think about it I think that the cases where I had to
resort to valgrind to get around such crappy stacktraces (when that was
all I got, even without suppressions) were probably all because there
was an UNLEAK() in play...
I don't see how UNLEAK() would impact stack traces. It should either
make something not-leaked-at-all (in which case LSan will no longer
mention it), or it does nothing (it throws some wasted memory into a
structure which is itself not leaked).
Yes, I think either categorically wrong here, or it applies to some
other case I wasn't able to dig up. Or maybe not, doesn't Taylor's
example take it from "Direct leak" to "Indirect leak" with the
suppression in play? I think those were related somehow (but don't have
that in front of me as I type this out).
E.g. (to reinforce your point) try compiling with SANITIZE=leak and running:
$ TZ=UTC t/helper/test-tool date show:format:%z 1466000000 +0200
1466000000 -> +0000
+0200 -> +0000
=================================================================
==335188==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x7f31cdd21db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
#1 0x7f31cdb04e4a in __GI___strdup string/strdup.c:42
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
Now try with "SANITIZE=" and valgrind can give you the "real" source:
$ TZ=UTC valgrind --leak-check=full t/helper/test-tool date show:format:%z 1466000000 +0200
==336515== Memcheck, a memory error detector
==336515== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==336515== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==336515== Command: t/helper/test-tool date show:format:%z 1466000000 +0200
==336515==
1466000000 -> +0000
+0200 -> +0000
==336515==
==336515== HEAP SUMMARY:
==336515== in use at exit: 1,208 bytes in 13 blocks
==336515== total heap usage: 50 allocs, 37 frees, 27,593 bytes allocated
==336515==
==336515== 3 bytes in 1 blocks are definitely lost in loss record 1 of 13
==336515== at 0x483877F: malloc (vg_replace_malloc.c:307)
==336515== by 0x49C6E4A: strdup (strdup.c:42)
==336515== by 0x261E3A: xstrdup (wrapper.c:29)
==336515== by 0x152785: parse_date_format (date.c:991)
==336515== by 0x116A34: show_dates (test-date.c:39)
==336515== by 0x116DE9: cmd__date (test-date.c:116)
==336515== by 0x115227: cmd_main (test-tool.c:127)
==336515== by 0x115334: main (common-main.c:52)
==336515==
==336515== LEAK SUMMARY:
==336515== definitely lost: 3 bytes in 1 blocks
==336515== indirectly lost: 0 bytes in 0 blocks
==336515== possibly lost: 0 bytes in 0 blocks
==336515== still reachable: 1,205 bytes in 12 blocks
==336515== suppressed: 0 bytes in 0 blocks
==336515== Reachable blocks (those to which a pointer was found) are not shown.
==336515== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==336515==
==336515== For lists of detected and suppressed errors, rerun with: -s
==336515== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
I don't know why it habitually loses track of seemingly easy-to-track
leaks sometimes, but it does. There isn't an UNLEAK() in play here.
When I run into these I try it with valgrind, fix the leak, then test
again with SANITIZE=leak. I haven't found cases where it actually misses
things, just cases where the stack traces suck.
From: Jeff King <hidden> Date: 2021-10-29 20:56:33
On Wed, Oct 27, 2021 at 10:57:52PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
So it's not the "container" element, but rather it can be a problem if
people annotate too broadly (you will miss some leaks). In the case of
rev_info, there is no way to _not_ leak right now, because it has no
cleanup function.
It doesn't have one, but there are uses of setup_revisions() and
rev_info usage that don't leak, as that builtin/rev-list.c case shows.
I mean, in that case it's not doing much of anything, but at least we
test that setup_revisions() itself doesn't leak right now, but wouldn't
with UNLEAK().
I don't think that's true. If you UNLEAK() the rev_info in the caller,
then it will only affect allocations that are still reachable from
rev_info. I.e., things that are by definition not a leak in
setup_revisions().
Now you could argue that setup_revisions() is "leaking" by allocating
things and stuffing them into rev_info that it should not be. But we can
never know that until we have an actual function that cleans up a
rev_info, which defines what it's "supposed" to have ownership of.
Maybe we have callers that explicitly try to de-allocate bits of the
rev_info. But IMHO that is the source of the whole problem: how is
random code using rev_info supposed to know which of its internal
details are owned or not? This should be documented and enforced with a
single function.
So just FWIW I'm not saying "hey can we hold off on that UNLEAK() for
far future xyz", but for a thing I've got queued up that I'd rather not
start rewriting...
Just to be clear: I am totally fine with dropping Taylor's UNLEAK
patches (as I've said already). I was only arguing here that annotating
via external files is worse than just adding an UNLEAK().
I'm also trying to combat what I see as mis-conceptions or inaccuracies
about what UNLEAK() does or its implications (or even what counts as a
"leak"). But I hope in the long run that we don't need _any_ kind of
annotation, because we'll actually be leak-free. And then we don't have
to care about any of this.
quoted
I don't see how UNLEAK() would impact stack traces. It should either
make something not-leaked-at-all (in which case LSan will no longer
mention it), or it does nothing (it throws some wasted memory into a
structure which is itself not leaked).
Yes, I think either categorically wrong here, or it applies to some
other case I wasn't able to dig up. Or maybe not, doesn't Taylor's
example take it from "Direct leak" to "Indirect leak" with the
suppression in play? I think those were related somehow (but don't have
that in front of me as I type this out).
I don't think UNLEAK() can move something from "direct" to "indirect" in
LSan's terminology. If rev_info points to an array of structs, and those
structs point to allocated strings, then the array itself is a "direct"
leak, and the strings are "indirect" (they are leaked, but presumably
fixing the direct leak would also deallocate them).
If UNLEAK() makes the array not-leaked, then those indirect leaks don't
become direct. They should be transitively not-leaked, too.
E.g. (to reinforce your point) try compiling with SANITIZE=leak and running:
$ TZ=UTC t/helper/test-tool date show:format:%z 1466000000 +0200
1466000000 -> +0000
+0200 -> +0000
=================================================================
==335188==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x7f31cdd21db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
#1 0x7f31cdb04e4a in __GI___strdup string/strdup.c:42
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
So these should be real leaks. Of course with the lousy stack trace it's
hard to see what they are. But I don't see how UNLEAK() is responsible
for making the lousy stack trace. You could try compiling with LSan but
_not_ -DSUPPRESS_ANNOTATED_LEAKS and see if the result is similarly bad
(but I expect it to be, since test-date.c does not have any UNLEAK()
calls in it).
-Peff
From: Jeff King <hidden> Date: 2021-10-29 21:05:03
On Fri, Oct 29, 2021 at 04:56:31PM -0400, Jeff King wrote:
quoted
E.g. (to reinforce your point) try compiling with SANITIZE=leak and running:
$ TZ=UTC t/helper/test-tool date show:format:%z 1466000000 +0200
1466000000 -> +0000
+0200 -> +0000
=================================================================
==335188==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x7f31cdd21db0 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
#1 0x7f31cdb04e4a in __GI___strdup string/strdup.c:42
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
So these should be real leaks. Of course with the lousy stack trace it's
hard to see what they are. But I don't see how UNLEAK() is responsible
for making the lousy stack trace. You could try compiling with LSan but
_not_ -DSUPPRESS_ANNOTATED_LEAKS and see if the result is similarly bad
(but I expect it to be, since test-date.c does not have any UNLEAK()
calls in it).
I just tested this, and it is unrelated to UNLEAK(). Interestingly,
compiling with SANITIZE=address does give the correct stack trace, so I
think LSan is just buggy here for some reason.
We usually suppress leaks with ASAN_OPTIONS because there are so many
(and we want to get at the more important signal of actual memory
errors). But it wouldn't be hard to have that setting respect your
PASSING_SANITIZE_LEAK variables. The big downside is that ASan runs of
the test suite are much more expensive.
Hmm. A little googling turns up LSan's fast_unwind_on_malloc option. And
indeed:
$ LSAN_OPTIONS=fast_unwind_on_malloc=0 t/helper/test-tool date show:format:%z 1466000000 +0200
1466000000 -> +0000
+0200 -> +0000
=================================================================
==39628==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 3 byte(s) in 1 object(s) allocated from:
#0 0x7fa664e39b94 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:56
#1 0x7fa664c124aa in __GI___strdup string/strdup.c:42
#2 0x557636c40c2e in xstrdup /home/peff/compile/git/wrapper.c:29
#3 0x557636b2ae52 in parse_date_format /home/peff/compile/git/date.c:991
#4 0x557636aedb0e in show_dates t/helper/test-date.c:39
#5 0x557636aedee1 in cmd__date t/helper/test-date.c:116
#6 0x557636aec1f0 in cmd_main t/helper/test-tool.c:127
#7 0x557636aec30d in main /home/peff/compile/git/common-main.c:52
#8 0x7fa664babe49 in __libc_start_main ../csu/libc-start.c:314
#9 0x557636aebec9 in _start (/home/peff/compile/git/t/helper/test-tool+0xcec9)
SUMMARY: LeakSanitizer: 3 byte(s) leaked in 1 allocation(s).
So perhaps we ought to be setting that by default.
-Peff