{"thread":{"id":"45617","subject":"[PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC 7.","startedAt":"2017-04-06T08:02:55Z","lastAt":"2017-04-06T10:24:25Z","messageCount":4,"participants":["Martin Liška","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"316259","messageId":"072afb58-6159-ddeb-b7dc-40a87e8c6ae7@suse.cz","threadId":"45617","inReplyTo":null,"subject":"[PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC 7.","fromName":"Martin Liška","fromEmail":"mliska@suse.cz","sentAt":"2017-04-06T08:02:45Z","receivedAt":"2017-04-06T08:02:55Z","isPatch":true,"sender":{"key":"mliska@suse.cz","avatar":null},"body":"Hello.\n\nFollowing patch fixes issues that can be seen with -fsanitize=address on GCC 7.\n\nPatch was tested with make test.\n\nThanks,\nMartin\n\n\nFrom 79dace4bdac4f571c14c7edb9b1007c155475c3f Mon Sep 17 00:00:00 2001\nFrom: marxin <mliska@suse.cz>\nDate: Wed, 5 Apr 2017 14:32:29 +0200\nSubject: [PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC\n 7.\n\nThe use-after-scope is triggered here:\nREAD of size 8 at 0x7ffc4f674e20 thread T0\n    #0 0x6f0b69 in finish_command /home/marxin/Programming/git/run-command.c:570\n    #1 0x5b6101 in kill_multi_file_filter /home/marxin/Programming/git/convert.c:570\n    #2 0x5b798a in kill_multi_file_filter /home/marxin/Programming/git/convert.c:770\n\nSigned-off-by: Martin Liska <mliska@suse.cz>\n---\n convert.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 8d652bf27..bf4eaf10e 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -568,6 +568,7 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *\n \tentry->process.clean_on_exit = 0;\n \tkill(entry->process.pid, SIGTERM);\n \tfinish_command(&entry->process);\n+\tfree(entry->process.argv);\n \n \thashmap_remove(hashmap, entry, NULL);\n \tfree(entry);\n@@ -582,6 +583,7 @@ static void stop_multi_file_filter(struct child_process *process)\n \tsigchain_pop(SIGPIPE);\n \t/* Finish command will wait until the shutdown is complete. */\n \tfinish_command(process);\n+\tfree(process->argv);\n }\n \n static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)\n@@ -589,7 +591,6 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n \tint err;\n \tstruct cmd2process *entry;\n \tstruct child_process *process;\n-\tconst char *argv[] = { cmd, NULL };\n \tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n \tchar *cap_buf;\n \tconst char *cap_name;\n@@ -600,7 +601,8 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n \tprocess = &entry->process;\n \n \tchild_process_init(process);\n-\tprocess->argv = argv;\n+\tprocess->argv = xcalloc(2, sizeof(const char *));\n+\tprocess->argv[0] = cmd;\n \tprocess->use_shell = 1;\n \tprocess->in = -1;\n \tprocess->out = -1;\n-- \n2.12.2\n\n"},{"id":"316261","messageId":"20170406083305.bs3hkxp7dagajg7m@sigill.intra.peff.net","threadId":"45617","inReplyTo":"072afb58-6159-ddeb-b7dc-40a87e8c6ae7@suse.cz","subject":"Re: [PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC 7.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-06T08:33:06Z","receivedAt":"2017-04-06T08:33:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 06, 2017 at 10:02:45AM +0200, Martin Liška wrote:\n\n> Subject: [PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC\n>  7.\n> \n> The use-after-scope is triggered here:\n> READ of size 8 at 0x7ffc4f674e20 thread T0\n>     #0 0x6f0b69 in finish_command /home/marxin/Programming/git/run-command.c:570\n>     #1 0x5b6101 in kill_multi_file_filter /home/marxin/Programming/git/convert.c:570\n>     #2 0x5b798a in kill_multi_file_filter /home/marxin/Programming/git/convert.c:770\n\nYeah, this is definitely a problem. Your fix works, but...\n\n> @@ -600,7 +601,8 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n>  \tprocess = &entry->process;\n>  \n>  \tchild_process_init(process);\n> -\tprocess->argv = argv;\n> +\tprocess->argv = xcalloc(2, sizeof(const char *));\n> +\tprocess->argv[0] = cmd;\n>  \tprocess->use_shell = 1;\n>  \tprocess->in = -1;\n>  \tprocess->out = -1;\n\nWe can just do:\n\n  argv_array_push(&process->args, cmd);\n\nhere. And then it is freed automatically when finish_command() is\ncalled (and also if start_command never starts the process, which I\nthink your patch misses).\n\n-Peff\n"},{"id":"316263","messageId":"alpine.DEB.2.20.1704061051130.4268@virtualbox","threadId":"45617","inReplyTo":"072afb58-6159-ddeb-b7dc-40a87e8c6ae7@suse.cz","subject":"Re: [PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC 7.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-06T08:55:17Z","receivedAt":"2017-04-06T08:56:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Martin,\n\nOn Thu, 6 Apr 2017, Martin Liška wrote:\n\n> Following patch fixes issues that can be seen with -fsanitize=address on\n> GCC 7.\n\nGood catch.\n\nHowever, it may make more sense to switch to using the \"args\" field\ninstead of the \"argv\" field: it is of type \"struct argv_array\" and is\nreleased automagically by finish_command().\n\nIn other words, you would use something like\n\n@@ -600,7 +601,8 @@ static struct cmd2process\n*start_multi_file_filter(struct hashmap *hashmap, cons\n        process = &entry->process;\n\n        child_process_init(process);\n-       process->argv = argv;\n+\targv_array_push(&process->args, cmd);\n\ninstead, making even for a nice LOC reduction.\n\nCiao,\nJohannes"},{"id":"316269","messageId":"28c22c5e-677b-1554-6156-4e31cc0f0913@suse.cz","threadId":"45617","inReplyTo":"alpine.DEB.2.20.1704061051130.4268@virtualbox","subject":"Re: [PATCH v2 2/2] Fix stack-use-after-scope error reported by ASAN by GCC 7.","fromName":"Martin Liška","fromEmail":"mliska@suse.cz","sentAt":"2017-04-06T10:23:53Z","receivedAt":"2017-04-06T10:24:25Z","isPatch":true,"sender":{"key":"mliska@suse.cz","avatar":null},"body":"On 04/06/2017 10:55 AM, Johannes Schindelin wrote:\n> Hi Martin,\n> \n> On Thu, 6 Apr 2017, Martin Liška wrote:\n> \n>> Following patch fixes issues that can be seen with -fsanitize=address on\n>> GCC 7.\n> \n> Good catch.\n\nYep, actually it was me who wrote the new use-after-scope support in GCC 7.\nAnd I was bit pessimistic about real examples of such errors, but one popped\nup very soon.\n\n> \n> However, it may make more sense to switch to using the \"args\" field\n> instead of the \"argv\" field: it is of type \"struct argv_array\" and is\n> released automagically by finish_command().\n> \n> In other words, you would use something like\n> \n> @@ -600,7 +601,8 @@ static struct cmd2process\n> *start_multi_file_filter(struct hashmap *hashmap, cons\n>         process = &entry->process;\n> \n>         child_process_init(process);\n> -       process->argv = argv;\n> +\targv_array_push(&process->args, cmd);\n> \n> instead, making even for a nice LOC reduction.\n\nDone that way, survives tests.\n\nMartin\n\n> \n> Ciao,\n> Johannes\n> \n\n\n\nFrom 4e80f9bddff3c29fefb3628e0e920ca265d55e65 Mon Sep 17 00:00:00 2001\nFrom: marxin <mliska@suse.cz>\nDate: Thu, 6 Apr 2017 11:40:24 +0200\nSubject: [PATCH 2/2] Fix stack-use-after-scope error reported by ASAN by GCC\n 7.\n\nThe use-after-scope is triggered here:\nREAD of size 8 at 0x7ffc4f674e20 thread T0\n    #0 0x6f0b69 in finish_command /home/marxin/Programming/git/run-command.c:570\n    #1 0x5b6101 in kill_multi_file_filter /home/marxin/Programming/git/convert.c:570\n    #2 0x5b798a in kill_multi_file_filter /home/marxin/Programming/git/convert.c:770\n---\n convert.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 8d652bf27..73eb28eb8 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -589,7 +589,6 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n \tint err;\n \tstruct cmd2process *entry;\n \tstruct child_process *process;\n-\tconst char *argv[] = { cmd, NULL };\n \tstruct string_list cap_list = STRING_LIST_INIT_NODUP;\n \tchar *cap_buf;\n \tconst char *cap_name;\n@@ -600,7 +599,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons\n \tprocess = &entry->process;\n \n \tchild_process_init(process);\n-\tprocess->argv = argv;\n+\targv_array_push(&process->args, cmd);\n \tprocess->use_shell = 1;\n \tprocess->in = -1;\n \tprocess->out = -1;\n-- \n2.12.2\n\n"}]}