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

[Thomas Gummerer]

On 10/04, Junio C Hamano wrote:
> Johannes Sixt  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 = >process;
> >>
> >
> > Perhaps this should become
> >
> > argv_array_push(>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 
> 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 
> Signed-off-by: Johannes Sixt 
> Signed-off-by: Junio C Hamano 
> ---
>  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 = >process;
>  
>   child_process_init(process);
> - process->argv = argv;
> + argv_array_push(>args, cmd);

Thanks!  *Much* nicer than what I had :)

>   process->use_shell = 1;
>   process->in = -1;
>   process->out = -1;
> -- 
> 2.14.2-889-gd2948f6aa6
> 

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

[Johannes Sixt]

Am 04.10.2017 um 06:59 schrieb Junio C Hamano:

Johannes Sixt  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 = >process;



Perhaps this should become

argv_array_push(>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 
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 
Signed-off-by: Johannes Sixt 
Signed-off-by: Junio C Hamano 
---
  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 = >process;
  
  	child_process_init(process);

-   process->argv = argv;
+   argv_array_push(>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 

-- Hannes

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

[Jeff King]

On Wed, Oct 04, 2017 at 01:59:31PM +0900, Junio C Hamano wrote:

> > Perhaps this should become
> >
> > argv_array_push(>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 
> Date: Tue, 3 Oct 2017 22:24:57 +0200
> Subject: [PATCH] sub-process: use child_process.args instead of 
> child_process.argv

This looks good (and is exactly the type of case for which I added
"args" to the child_process in the first place). The commit message
is well-explained and the patch looks obviously correct.

-Peff

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

[Junio C Hamano]

Johannes Sixt  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 = >process;
>>
>
> Perhaps this should become
>
>   argv_array_push(>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 
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 
Signed-off-by: Johannes Sixt 
Signed-off-by: Junio C Hamano 
---
 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 = >process;
 
child_process_init(process);
-   process->argv = argv;
+   argv_array_push(>args, cmd);
process->use_shell = 1;
process->in = -1;
process->out = -1;
-- 
2.14.2-889-gd2948f6aa6

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

[Stefan Beller]

On Tue, Oct 3, 2017 at 12:57 PM, Thomas Gummerer  wrote:
> 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==
>
> Fix this by allocating the memory on properly on the heap.  This memory
> is allocated on the heap, and never free'd.  However the same seems to be
> true for struct child_process, so it should be fine to just let the
> memory be free'd when the process terminates.

Uh. :( The broken window theory at work.

The patch below seems correct, but as you eluded to, now we'd be
leaking memory. The run_command API has two fields 'char **argv'
and 'argv_array args'. The argv is kept around for historical reasons
as well as when the caller wants to be in control of the array (the caller
needs to free the memory, but could also just reuse it for a slightly
different invocation), whereas the args argument is owned by the child
process, such that the memory is freed by finish_command.

As we're doing a memory allocation now anyway, how about:

-   const char *argv[] = { cmd, NULL };
...
child_process_init(process);
+argv_array_push(process.args, cmd);

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

[Johannes Sixt]

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 = >process;



Perhaps this should become

argv_array_push(>args, cmd);

so that there is no new memory leak?

-- Hannes

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

[Thomas Gummerer]

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==

Fix this by allocating the memory on properly on the heap.  This memory
is allocated on the heap, and never free'd.  However the same seems to be
true for struct child_process, so it should be fine to just let the
memory be free'd when the process terminates.

Signed-off-by: Thomas Gummerer 
---
 sub-process.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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 = >process;
-- 
2.14.1.480.gb18f417b89