Am 04.10.2017 um 06:59 schrieb Junio C Hamano:
Johannes Sixt <j6t@xxxxxxxx> 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@xxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Johannes Sixt <j6t@xxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
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@xxxxxxxx>
-- Hannes
