Subject: Re: [PATCH 05/11] builtin/repack.c: avoid leaking child arguments

From: Derrick Stolee <[email-masked]>
Date: Thu, 21 Oct 2021 09:32:16 -0400

On 10/20/2021 11:39 PM, Taylor Blau wrote:
> `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 <[email-masked]>
> ---
>  builtin/repack.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 0b2d1e5d82..d16bab09a4 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -244,6 +244,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
>  	struct child_process cmd = CHILD_PROCESS_INIT;
>  	FILE *out;
>  	struct strbuf line = STRBUF_INIT;
> +	int ret = 0;

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".

> -	return finish_command(&cmd);
> +	ret = finish_command(&cmd);
> +
> +cleanup:
> +	child_process_clear(&cmd);
> +
> +	return ret;

Here, you are taking the result of an inner call, but actually
returning it. This makes sense to be "ret".

> +cleanup:
>  	string_list_clear(&names, 0);
>  	string_list_clear(&rollback, 0);
>  	string_list_clear(&existing_nonkept_packs, 0);
>  	string_list_clear(&existing_kept_packs, 0);
>  	clear_pack_geometry(geometry);
>  	strbuf_release(&line);
> +	child_process_clear(&cmd);
>  
> -	return 0;
> +	return ret;

Also here is good.

Thanks,
-Stolee