Re: [PATCH v4 08/19] worktree: fix a trivial leak in prune_worktrees()
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jan 18, 2023, 10:57 UTC
- Message-ID
- <230118.86o7qwxg4e.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqqedrs8igj.fsf@gitster.g>
On Tue, Jan 17 2023, Junio C Hamano wrote:
Show 53 quoted lines
> Æ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.