| Message ID | patch-v4-08.19-1fe25bc6981-20230117T151202Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leak fixes: various simple leak fixes | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > We were leaking both the "struct strbuf" in prune_worktrees(), as well > as the "path" we got from should_prune_worktree(). Since these were > the only two uses of the "struct string_list" let's change it to a > "DUP" and push these to it with "string_list_append_nodup()". > ... > diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh > index d34d77f8934..ba8d929d189 100755 > --- a/t/t3203-branch-output.sh > +++ b/t/t3203-branch-output.sh > @@ -1,6 +1,8 @@ > #!/bin/sh > > test_description='git branch display tests' > + > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > . "$TEST_DIRECTORY"/lib-terminal.sh This is wrong, isn't it? t3203 uses --points-at, which populates filter.points_at by calling parse_opt_object_name(). Various members of the ref-filter structure is never freed (and there is no API helper function in ref-filter subsystem). Other tests that use --points-at (e.g. t6302 and t7004) are not marked with "passes_sanitize_leak", and this one shouldn't be, either. With the following squashed in, the branch seems to pass, but I am not sure which is lessor of the two evils. From the point of view of the code maintenance, UNLEAK() to mark this singleton variable is far cleaner to deal with than selectively running the leak checks with the "passes_sanitize_leak" mechanism (which always feels like a losing whack-a-mole hack). builtin/branch.c | 1 + 1 file changed, 1 insertion(+) diff --git c/builtin/branch.c w/builtin/branch.c index f63fd45edb..4fe7757670 100644 --- c/builtin/branch.c +++ w/builtin/branch.c @@ -742,6 +742,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) if (filter.abbrev == -1) filter.abbrev = DEFAULT_ABBREV; filter.ignore_case = icase; + UNLEAK(filter); finalize_colopts(&colopts, -1); if (filter.verbose) {
On Tue, Jan 17 2023, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> We were leaking both the "struct strbuf" in prune_worktrees(), as well >> as the "path" we got from should_prune_worktree(). Since these were >> the only two uses of the "struct string_list" let's change it to a >> "DUP" and push these to it with "string_list_append_nodup()". >> ... >> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh >> index d34d77f8934..ba8d929d189 100755 >> --- a/t/t3203-branch-output.sh >> +++ b/t/t3203-branch-output.sh >> @@ -1,6 +1,8 @@ >> #!/bin/sh >> >> test_description='git branch display tests' >> + >> +TEST_PASSES_SANITIZE_LEAK=true >> . ./test-lib.sh >> . "$TEST_DIRECTORY"/lib-terminal.sh > > This is wrong, isn't it? > > t3203 uses --points-at, which populates filter.points_at by calling > parse_opt_object_name(). Various members of the ref-filter > structure is never freed (and there is no API helper function in > ref-filter subsystem). > > Other tests that use --points-at (e.g. t6302 and t7004) are not > marked with "passes_sanitize_leak", and this one shouldn't be, > either. > > With the following squashed in, the branch seems to pass, but I am > not sure which is lessor of the two evils. From the point of view > of the code maintenance, UNLEAK() to mark this singleton variable is > far cleaner to deal with than selectively running the leak checks > with the "passes_sanitize_leak" mechanism (which always feels like a > losing whack-a-mole hack). > > builtin/branch.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git c/builtin/branch.c w/builtin/branch.c > index f63fd45edb..4fe7757670 100644 > --- c/builtin/branch.c > +++ w/builtin/branch.c > @@ -742,6 +742,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > if (filter.abbrev == -1) > filter.abbrev = DEFAULT_ABBREV; > filter.ignore_case = icase; > + UNLEAK(filter); > > finalize_colopts(&colopts, -1); > if (filter.verbose) { I'll send a v5 re-roll without this change, sorry. This is a case where the version of GCC I was testing with doesn't report the leak, but clang does (and probably other versions of GCC), sorry.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> diff --git c/builtin/branch.c w/builtin/branch.c >> index f63fd45edb..4fe7757670 100644 >> --- c/builtin/branch.c >> +++ w/builtin/branch.c >> @@ -742,6 +742,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) >> if (filter.abbrev == -1) >> filter.abbrev = DEFAULT_ABBREV; >> filter.ignore_case = icase; >> + UNLEAK(filter); >> >> finalize_colopts(&colopts, -1); >> if (filter.verbose) { > > I'll send a v5 re-roll without this change, sorry. I'd rather see your reroll with the above addition of UNLEAK() than without it, to fix the breakage.
On Wed, Jan 18 2023, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> diff --git c/builtin/branch.c w/builtin/branch.c >>> index f63fd45edb..4fe7757670 100644 >>> --- c/builtin/branch.c >>> +++ w/builtin/branch.c >>> @@ -742,6 +742,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) >>> if (filter.abbrev == -1) >>> filter.abbrev = DEFAULT_ABBREV; >>> filter.ignore_case = icase; >>> + UNLEAK(filter); >>> >>> finalize_colopts(&colopts, -1); >>> if (filter.verbose) { >> >> I'll send a v5 re-roll without this change, sorry. > > I'd rather see your reroll with the above addition of UNLEAK() than > without it, to fix the breakage. I don't mind that UNLEAK() being in-tree until a better fix for that leak, but doesn't the v5 I sent also address this? The issue was that I mis-marked a test as passing, when it only passed depending on my local compiler (-fsanitize=leak is fickle sometimes). Now that we're not marking that test as leak-free there's no need for the UNLEAK() for now, no? Or is there some edge case I didn't spot/notice? 1. https://lore.kernel.org/git/cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> I'd rather see your reroll with the above addition of UNLEAK() than >> without it, to fix the breakage. > > I don't mind that UNLEAK() being in-tree until a better fix for that > leak, but doesn't the v5 I sent also address this? > > The issue was that I mis-marked a test as passing, when it only passed > depending on my local compiler (-fsanitize=leak is fickle > sometimes). Now that we're not marking that test as leak-free there's no > need for the UNLEAK() for now, no? > > Or is there some edge case I didn't spot/notice? The top-level singleton instance of "filter" that is used once and never grows unbounded is a perfect use case for UNLEAK(). And with it, the test DOES get leak-free, so it would be appropriate to keep the PASSES variable added to the script (until it gets broken again). Or did you have any other uses of tools with leaks in the script, other than the one that is fixed with the UNLEAK()?
diff --git a/builtin/worktree.c b/builtin/worktree.c index 591d659faea..865ce9be22b 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -173,7 +173,7 @@ static void prune_worktrees(void) { struct strbuf reason = STRBUF_INIT; struct strbuf main_path = STRBUF_INIT; - struct string_list kept = STRING_LIST_INIT_NODUP; + struct string_list kept = STRING_LIST_INIT_DUP; DIR *dir = opendir(git_path("worktrees")); struct dirent *d; if (!dir) @@ -184,14 +184,14 @@ static void prune_worktrees(void) if (should_prune_worktree(d->d_name, &reason, &path, expire)) prune_worktree(d->d_name, reason.buf); else if (path) - string_list_append(&kept, path)->util = xstrdup(d->d_name); + string_list_append_nodup(&kept, path)->util = xstrdup(d->d_name); } closedir(dir); strbuf_add_absolute_path(&main_path, get_git_common_dir()); /* massage main worktree absolute path to match 'gitdir' content */ strbuf_strip_suffix(&main_path, "/."); - string_list_append(&kept, strbuf_detach(&main_path, NULL)); + string_list_append_nodup(&kept, strbuf_detach(&main_path, NULL)); prune_dups(&kept); string_list_clear(&kept, 1); diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh index 3d28c7f06b2..568a47ec426 100755 --- a/t/t2401-worktree-prune.sh +++ b/t/t2401-worktree-prune.sh @@ -5,6 +5,7 @@ test_description='prune $GIT_DIR/worktrees' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success initialize ' diff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh index 5c44453e1c1..8970780efcc 100755 --- a/t/t2406-worktree-repair.sh +++ b/t/t2406-worktree-repair.sh @@ -2,6 +2,7 @@ test_description='test git worktree repair' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh index d34d77f8934..ba8d929d189 100755 --- a/t/t3203-branch-output.sh +++ b/t/t3203-branch-output.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='git branch display tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh
We were leaking both the "struct strbuf" in prune_worktrees(), as well as the "path" we got from should_prune_worktree(). Since these were the only two uses of the "struct string_list" let's change it to a "DUP" and push these to it with "string_list_append_nodup()". For the string_list_append_nodup() we could also string_list_append() the main_path.buf, and then strbuf_release(&main_path) right away. But doing it this way avoids an allocation, as we already have the "struct strbuf" prepared for appending to "kept". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/worktree.c | 6 +++--- t/t2401-worktree-prune.sh | 1 + t/t2406-worktree-repair.sh | 1 + t/t3203-branch-output.sh | 2 ++ 4 files changed, 7 insertions(+), 3 deletions(-)