From: Derrick Stolee <stolee@gmail.com>
Date: Mon, 12 Nov 2018 15:26:47 GMT
Subject: Re: [PATCH 1/9] fsck: do not reuse child_process structs
Message-ID: <6b32adb9-60a9-5c05-48dc-fe90ef10277f@gmail.com>
In-Reply-To: <20181112144654.GA7400@sigill.intra.peff.net>

On 11/12/2018 9:46 AM, Jeff King wrote:
> The run-command API makes no promises about what is left in a struct
> child_process after a command finishes, and it's not safe to simply
> reuse it again for a similar command. In particular:
>
>   - if you use child->args or child->env_array, they are cleared after
>     finish_command()
>
>   - likewise, start_command() may point child->argv at child->args->argv;
>     reusing that would lead to accessing freed memory
>
>   - the in/out/err may hold pipe descriptors from the previous run

Thanks! This is helpful information.

> These two calls are _probably_ OK because they do not use any of those
> features. But it's only by chance, and may break in the future; let's
> reinitialize our struct for each program we run.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   builtin/fsck.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 06eb421720..b10f2b154c 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -841,6 +841,9 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
>   
>   		prepare_alt_odb(the_repository);
>   		for (alt =  the_repository->objects->alt_odb_list; alt; alt = alt->next) {
> +			child_process_init(&commit_graph_verify);
> +			commit_graph_verify.argv = verify_argv;
> +			commit_graph_verify.git_cmd = 1;
>   			verify_argv[2] = "--object-dir";
>   			verify_argv[3] = alt->path;
>   			if (run_command(&commit_graph_verify))
> @@ -859,6 +862,9 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
>   
>   		prepare_alt_odb(the_repository);
>   		for (alt =  the_repository->objects->alt_odb_list; alt; alt = alt->next) {
> +			child_process_init(&midx_verify);
> +			midx_verify.argv = midx_argv;
> +			midx_verify.git_cmd = 1;
>   			midx_argv[2] = "--object-dir";
>   			midx_argv[3] = alt->path;
>   			if (run_command(&midx_verify))

Looks good to me.

-Stolee