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

On Mon, Nov 10, 2014 at 01:44:24PM -0800, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

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?

Yes, they behave exactly the same (I think Dscho just doesn't hit it in
his patch because he assigns argv manually).

I don't have a real problem with going in this direction as a safety
measure, but I am not sure that it is safe to reuse a child_process
after finish_command in general, without an intervening
child_process_init. For instance, calling start_command will convert a
"child_process.in" value of "-1" instead a pipe, and overwrite that "-1"
with the descriptor of the pipe. A subsequent use of the same
child_process struct will ask the second child to use that pipe (the
write-half of the pipe, mind you) as its stdin, which is nonsensical.

So I think you are much better off just using two child_process structs
(or a single one and reinitializing in between calls).

-Peff