Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 10, 2014, 21:44 UTC
Message-ID: <xmqqbnoe91dz.fsf@gitster.dls.corp.google.com>
In-Reply-To: <df1b16208e443416f7185c8c8c110e637ea97ac4.1415630072.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 8 quoted lines

> In start_command(), unset "env" fields are initialized via "env_array". In
> finish_command(), the "env_array" is cleared, therefore the "env" field
> will point to free()d data.
>
> However, start_command() will set "env" to env_array.argv only if "env"
> was unset to begin with, and if it was already set, the caller will need
> the original value. Therefore, we need to be very careful only to reset
> "env" in finish_command() when it has been initialized in start_command().

Hmph. Does the same observation apply to cmd->argv that is initialied to point to cmd->args.argv only when it is unset?

These managed argv/env arrays originate from c460c0ec (run-command: store an optional argv_array, 2014-05-15), so I am CC'ing Peff. I think this change makes sense but I suspect we should do the same for args.

Show 20 quoted lines

>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  run-command.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index 79a0a76..85578da 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -555,6 +555,9 @@ int finish_command(struct child_process *cmd)
>  {
>  	int ret = wait_or_whine(cmd->pid, cmd->argv[0]);
>  	argv_array_clear(&cmd->args);
> +	/* Avoid pointing to a stale environment */
> +	if (cmd->env == cmd->env_array.argv)
> +		cmd->env = NULL;
>  	argv_array_clear(&cmd->env_array);
>  	return ret;
>  }