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

[Junio C Hamano <gitster@pobox.com>]

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;
-- 
2.14.2-889-gd2948f6aa6