Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 30, 2024, 15:27 UTC
Message-ID: <xmqqr0bagark.fsf@gitster.g>
In-Reply-To: <08a12be13c2fed247d6086967e7a3f03fa6519e1.1721995576.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 14 quoted lines

> When calling `git describe --contains=`, we end up invoking
> `cmd_name_rev()` with some munged argv array. This array may contain
> allocated strings and furthermore will likely be modified by the called
> function. This results in two memory leaks:
>
>   - First, we leak the array that we use to assemble the arguments.
>
>   - Second, we leak the allocated strings that we may have put into the
>     array.
>
> Fix those leaks by creating a separate copy of the array that we can
> hand over to `cmd_name_rev()`. This allows us to free all strings
> contained in the `strvec`, as the original vector will not be modified
> anymore.

OK, the separate copy has to be a shallow copy, as its purpose is not to lose pointers to the contained strings.

Show 5 quoted lines

> Furthermore, free both the `strvec` and the copied array to fix the
> first memory leak.
> ...
> +		strvec_clear(&args);
> +		free(argv_copy);

So, calling cmd_name_rev() may shuffle the argv_copy[] array but at least it will not free any element in it (as expected---it is typically the (argc, argv[]) the process receives from getting exec'ed) [*]. Freeing the argv_copy shell itself is sufficient to discard what we used to call cmd_name_rev(). And we discard args both its content strings and the array. OK.

> +		return ret;
>  	}
>  
>  	hashmap_init(&names, commit_name_neq, NULL, 0);

[Footnote]

 * The fact that cmd_foo() is called is not a hygiene thing to do in
   the first place, and in the longer term #leftoverbits we may need
   to refactor the thing further, into a proper library-ish reusable
   helper function that can be used to compute name_rev() any number
   of times, plus cmd_name_rev() and this caller that call it.