Re: [PATCH v2 5/9] builtin/repack.c: avoid leaking child arguments

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 27, 2021, 23:44 UTC
Message-ID: <xmqqzgqut4lr.fsf@gitster.g>
In-Reply-To: <bcd12ecab81029be825a646348cb7ae69970a819.1635282024.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

Show 9 quoted lines

> @@ -586,8 +588,10 @@ static int write_midx_included_packs(struct string_list *include,
>  		strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);
>  
>  	ret = start_command(&cmd);
> -	if (ret)
> +	if (ret) {
> +		child_process_clear(&cmd);
>  		return ret;
> +	}

This happens only when start_command() returns an error. But the function always calls child_process_clear() before doing so.

So I am not sure if this hunk is needed. It didn't exist in v1, if I recall correctly. Am I missing something obvious?

Show 18 quoted lines

> @@ -608,9 +612,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	struct pack_geometry *geometry = NULL;
>  	struct strbuf line = STRBUF_INIT;
>  	struct tempfile *refs_snapshot = NULL;
> -	int i, ext, ret;
> +	int i, ext, ret = 0;
>  	FILE *out;
>  	int show_progress = isatty(2);
> +	int cmd_cleared = 0;
>  
>  	/* variables to be filled by option parsing */
>  	int pack_everything = 0;
> @@ -794,7 +799,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  
>  	ret = start_command(&cmd);
>  	if (ret)
> -		return ret;
> +		goto cleanup;

Likewise, at this point, start_command() should have already cleared cmd before it returned an error, no?

Show 8 quoted lines

> @@ -818,8 +823,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	}
>  	fclose(out);
>  	ret = finish_command(&cmd);
> +	cmd_cleared = 1;
>  	if (ret)
> -		return ret;
> +		goto cleanup;

And cmd is also cleared after we pass this point. So perhaps after the cleanup label, there is no need to call child_process_clear() at all, no?