threads / patch / 45617

patch, 2 partsFix stack-use-after-scope error reported by ASAN by GCC 7.

Subject: [PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC 7.

## tl;dr

4 messages between Apr 6, 2017 and Apr 6, 2017. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Martin Liška· Apr 6, 2017, 08:02 UTC · lore

Hello.

Following patch fixes issues that can be seen with -fsanitize=address on GCC 7.

Patch was tested with make test.

Thanks, Martin

From 79dace4bdac4f571c14c7edb9b1007c155475c3f Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 5 Apr 2017 14:32:29 +0200
Subject: [PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC
 7.

The use-after-scope is triggered here:
READ of size 8 at 0x7ffc4f674e20 thread T0
    #0 0x6f0b69 in finish_command /home/marxin/Programming/git/run-command.c:570
    #1 0x5b6101 in kill_multi_file_filter /home/marxin/Programming/git/convert.c:570
    #2 0x5b798a in kill_multi_file_filter /home/marxin/Programming/git/convert.c:770

Signed-off-by: Martin Liska <mliska@suse.cz>
---
 convert.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Show changes to convert.c +4 −2

diff --git a/convert.c b/convert.c
index 8d652bf27..bf4eaf10e 100644
--- a/convert.c
+++ b/convert.c
@@ -568,6 +568,7 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *
 	entry->process.clean_on_exit = 0;
 	kill(entry->process.pid, SIGTERM);
 	finish_command(&entry->process);
+	free(entry->process.argv);
 
 	hashmap_remove(hashmap, entry, NULL);
 	free(entry);
@@ -582,6 +583,7 @@ static void stop_multi_file_filter(struct child_process *process)
 	sigchain_pop(SIGPIPE);
 	/* Finish command will wait until the shutdown is complete. */
 	finish_command(process);
+	free(process->argv);
 }
 
 static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)
@@ -589,7 +591,6 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
 	int err;
 	struct cmd2process *entry;
 	struct child_process *process;
-	const char *argv[] = { cmd, NULL };
 	struct string_list cap_list = STRING_LIST_INIT_NODUP;
 	char *cap_buf;
 	const char *cap_name;
@@ -600,7 +601,8 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
 	process = &entry->process;
 
 	child_process_init(process);
-	process->argv = argv;
+	process->argv = xcalloc(2, sizeof(const char *));
+	process->argv[0] = cmd;
 	process->use_shell = 1;
 	process->in = -1;
 	process->out = -1;

-- 
2.12.2

Jeff King· Apr 6, 2017, 08:33 UTC · re: Martin Liška · lore

Re: [PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC 7.

On Thu, Apr 06, 2017 at 10:02:45AM +0200, Martin Liška wrote:

Show 8 quoted lines

> Subject: [PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC
>  7.
> 
> The use-after-scope is triggered here:
> READ of size 8 at 0x7ffc4f674e20 thread T0
>     #0 0x6f0b69 in finish_command /home/marxin/Programming/git/run-command.c:570
>     #1 0x5b6101 in kill_multi_file_filter /home/marxin/Programming/git/convert.c:570
>     #2 0x5b798a in kill_multi_file_filter /home/marxin/Programming/git/convert.c:770

Yeah, this is definitely a problem. Your fix works, but...

Show 10 quoted lines

> @@ -600,7 +601,8 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
>  	process = &entry->process;
>  
>  	child_process_init(process);
> -	process->argv = argv;
> +	process->argv = xcalloc(2, sizeof(const char *));
> +	process->argv[0] = cmd;
>  	process->use_shell = 1;
>  	process->in = -1;
>  	process->out = -1;

We can just do:

  argv_array_push(&process->args, cmd);

here. And then it is freed automatically when finish_command() is called (and also if start_command never starts the process, which I think your patch misses).

-Peff

Johannes Schindelin· Apr 6, 2017, 08:55 UTC · re: Martin Liška · lore

Re: [PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC 7.

Hi Martin,

On Thu, 6 Apr 2017, Martin Liška wrote:

> Following patch fixes issues that can be seen with -fsanitize=address on
> GCC 7.

Good catch.

However, it may make more sense to switch to using the "args" field instead of the "argv" field: it is of type "struct argv_array" and is released automagically by finish_command().

In other words, you would use something like

Show changes to diff +1 −1

@@ -600,7 +601,8 @@ static struct cmd2process
*start_multi_file_filter(struct hashmap *hashmap, cons
        process = &entry->process;

        child_process_init(process);
-       process->argv = argv;
+	argv_array_push(&process->args, cmd);

instead, making even for a nice LOC reduction.

Ciao,
Johannes

Martin Liška· Apr 6, 2017, 10:23 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 2/2] Fix stack-use-after-scope error reported by ASAN by GCC 7.

On 04/06/2017 10:55 AM, Johannes Schindelin wrote:

Show 8 quoted lines

> Hi Martin,
> 
> On Thu, 6 Apr 2017, Martin Liška wrote:
> 
>> Following patch fixes issues that can be seen with -fsanitize=address on
>> GCC 7.
> 
> Good catch.

Yep, actually it was me who wrote the new use-after-scope support in GCC 7. And I was bit pessimistic about real examples of such errors, but one popped up very soon.

Show 16 quoted lines

> 
> However, it may make more sense to switch to using the "args" field
> instead of the "argv" field: it is of type "struct argv_array" and is
> released automagically by finish_command().
> 
> In other words, you would use something like
> 
> @@ -600,7 +601,8 @@ static struct cmd2process
> *start_multi_file_filter(struct hashmap *hashmap, cons
>         process = &entry->process;
> 
>         child_process_init(process);
> -       process->argv = argv;
> +	argv_array_push(&process->args, cmd);
> 
> instead, making even for a nice LOC reduction.

Done that way, survives tests.

Martin

> 
> Ciao,
> Johannes
>

From 4e80f9bddff3c29fefb3628e0e920ca265d55e65 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 6 Apr 2017 11:40:24 +0200
Subject: [PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC
 7.

The use-after-scope is triggered here:
READ of size 8 at 0x7ffc4f674e20 thread T0
    #0 0x6f0b69 in finish_command /home/marxin/Programming/git/run-command.c:570
    #1 0x5b6101 in kill_multi_file_filter /home/marxin/Programming/git/convert.c:570
    #2 0x5b798a in kill_multi_file_filter /home/marxin/Programming/git/convert.c:770
---
 convert.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Show changes to convert.c +1 −2

diff --git a/convert.c b/convert.c
index 8d652bf27..73eb28eb8 100644
--- a/convert.c
+++ b/convert.c
@@ -589,7 +589,6 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
 	int err;
 	struct cmd2process *entry;
 	struct child_process *process;
-	const char *argv[] = { cmd, NULL };
 	struct string_list cap_list = STRING_LIST_INIT_NODUP;
 	char *cap_buf;
 	const char *cap_name;
@@ -600,7 +599,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
 	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.12.2