Re: [PATCH 3/3] sub-process: allocate argv on the heap

[Johannes Sixt <j6t@kdbg.org>]

Am 04.10.2017 um 06:59 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:
>>> diff --git a/sub-process.c b/sub-process.c
>>> index 6dde5062be..4680af8193 100644
>>> --- a/sub-process.c
>>> +++ b/sub-process.c
>>> @@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
>>>    {
>>>    	int err;
>>>    	struct child_process *process;
>>> -	const char *argv[] = { cmd, NULL };
>>> +	const char **argv = xmalloc(2 * sizeof(char *));
>>> +	argv[0] = cmd;
>>> +	argv[1] = NULL;
>>>      	entry->cmd = cmd;
>>>    	process = &entry->process;
>>>
>>
>> Perhaps this should become
>>
>> 	argv_array_push(&process->args, cmd);
>>
>> so that there is no new memory leak?
> 
> Sounds like a good idea (if I am not grossly mistaken as to what is
> being suggested).
> 
> Here is what I am planning to queue.
> 
> -- >8 --
> From: Johannes Sixt <j6t@kdbg.org>
> Date: Tue, 3 Oct 2017 22:24:57 +0200
> Subject: [PATCH] sub-process: use child_process.args instead of child_process.argv
> 
> Currently the argv is only allocated on the stack, and then assigned to
> process->argv.  When the start_subprocess function goes out of scope,
> the local argv variable is eliminated from the stack, but the pointer is
> still kept around in process->argv.
> 
> Much later when we try to access the same process->argv in
> finish_command, this leads us to access a memory location that no longer
> contains what we want.  As argv0 is only used for printing errors, this
> is not easily noticed in normal git operations.  However when running
> t0021-conversion.sh through valgrind, valgrind rightfully complains:
> 
> ==21024== Invalid read of size 8
> ==21024==    at 0x2ACF64: finish_command (run-command.c:869)
> ==21024==    by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)
> ==21024==    by 0x2AB41E: cleanup_children (run-command.c:45)
> ==21024==    by 0x2AB526: cleanup_children_on_exit (run-command.c:81)
> ==21024==    by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)
> ==21024==    by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)
> ==21024==    by 0x11A9EF: handle_builtin (git.c:550)
> ==21024==    by 0x11ABCC: run_argv (git.c:602)
> ==21024==    by 0x11AD8E: cmd_main (git.c:679)
> ==21024==    by 0x1BF125: main (common-main.c:43)
> ==21024==  Address 0x1ffeffec00 is on thread 1's stack
> ==21024==  1504 bytes below stack pointer
> ==21024==
> 
> These days, the child_process structure has its own args array, and
> the standard way to set up its argv[] is to use that one, instead of
> assigning to process->argv to point at an array that is outside.
> Use that facility automatically fixes this issue.
> 
> Reported-by: Thomas Gummerer <t.gummerer@gmail.com>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   sub-process.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/sub-process.c b/sub-process.c
> index fcc4832c14..648b3a3943 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -74,13 +74,12 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
>   {
>   	int err;
>   	struct child_process *process;
> -	const char *argv[] = { cmd, NULL };
>   
>   	entry->cmd = cmd;
>   	process = &entry->process;
>   
>   	child_process_init(process);
> -	process->argv = argv;
> +	argv_array_push(&process->args, cmd);
>   	process->use_shell = 1;
>   	process->in = -1;
>   	process->out = -1;
> 

Thank you very much! That looks good. Just to be on the safe side:

Signed-off-by: Johannes Sixt <j6t@kdbg.org>

-- Hannes