{"thread":{"id":"36447","subject":"[PATCH 0/6] run_external_diff cleanups","startedAt":"2014-04-19T19:11:29Z","lastAt":"2014-04-20T01:35:00Z","messageCount":10,"participants":["Jeff King","Max L","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"239125","messageId":"20140419191128.GA9266@sigill.intra.peff.net","threadId":"36447","inReplyTo":null,"subject":"[PATCH 0/6] run_external_diff cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-19T19:11:29Z","receivedAt":"2014-04-19T19:11:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's possible to overflow an array in run_external_diff and write a\nsingle NULL onto the stack. The first patch below fixes that. The rest\nare cleanups and modernizations I noticed while in the area. It's\npossible that patch 3 is also a bug fix, depending on your\ninterpretation.\n\n  [1/6]: run_external_diff: use an argv_array for the command line\n  [2/6]: run_external_diff: use an argv_array for the environment\n  [3/6]: run_external_diff: clean up error handling\n  [4/6]: run_external_diff: drop fflush(NULL)\n  [5/6]: run_external_diff: hoist common bits out of conditional\n  [6/6]: run_external_diff: refactor cmdline setup logic\n\n-Peff\n"},{"id":"239128","messageId":"20140419191706.GA8981@sigill.intra.peff.net","threadId":"36447","inReplyTo":"20140419191128.GA9266@sigill.intra.peff.net","subject":"[PATCH 1/6] run_external_diff: use an argv_array for the command line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-19T19:17:06Z","receivedAt":"2014-04-19T19:17:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently generate the command-line for the external\ncommand using a fixed-length array of size 10. But if there\nis a rename, we actually need 11 elements (10 items, plus a\nNULL), and end up writing a random NULL onto the stack.\n\nRather than bump the limit, let's just an argv_array, which\nmakes this sort of error impossible.\n\nNoticed-by: Max L <infthi.inbox@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis was actually noticed by a GitHub user, who proposed bumping\nthe array size to 11:\n\n  https://github.com/git/git/pull/92\n\nEven though this fix is a bigger change, I'd rather do it this way, as\nit is more obviously correct to a reader (and it solves the problem\nforever). I pulled the name/email from that commit, but please let me\nknow if you'd prefer to be credited differently.\n\n diff.c | 32 ++++++++++++++++----------------\n 1 file changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 539997f..b154284 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -16,6 +16,7 @@\n #include \"submodule.h\"\n #include \"ll-merge.h\"\n #include \"string-list.h\"\n+#include \"argv-array.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -2902,9 +2903,8 @@ static void run_external_diff(const char *pgm,\n \t\t\t      int complete_rewrite,\n \t\t\t      struct diff_options *o)\n {\n-\tconst char *spawn_arg[10];\n+\tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tint retval;\n-\tconst char **arg = &spawn_arg[0];\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tconst char *env[3] = { NULL };\n \tchar env_counter[50];\n@@ -2915,23 +2915,22 @@ static void run_external_diff(const char *pgm,\n \t\tconst char *othername = (other ? other : name);\n \t\ttemp_one = prepare_temp_file(name, one);\n \t\ttemp_two = prepare_temp_file(othername, two);\n-\t\t*arg++ = pgm;\n-\t\t*arg++ = name;\n-\t\t*arg++ = temp_one->name;\n-\t\t*arg++ = temp_one->hex;\n-\t\t*arg++ = temp_one->mode;\n-\t\t*arg++ = temp_two->name;\n-\t\t*arg++ = temp_two->hex;\n-\t\t*arg++ = temp_two->mode;\n+\t\targv_array_push(&argv, pgm);\n+\t\targv_array_push(&argv, name);\n+\t\targv_array_push(&argv, temp_one->name);\n+\t\targv_array_push(&argv, temp_one->hex);\n+\t\targv_array_push(&argv, temp_one->mode);\n+\t\targv_array_push(&argv, temp_two->name);\n+\t\targv_array_push(&argv, temp_two->hex);\n+\t\targv_array_push(&argv, temp_two->mode);\n \t\tif (other) {\n-\t\t\t*arg++ = other;\n-\t\t\t*arg++ = xfrm_msg;\n+\t\t\targv_array_push(&argv, other);\n+\t\t\targv_array_push(&argv, xfrm_msg);\n \t\t}\n \t} else {\n-\t\t*arg++ = pgm;\n-\t\t*arg++ = name;\n+\t\targv_array_push(&argv, pgm);\n+\t\targv_array_push(&argv, name);\n \t}\n-\t*arg = NULL;\n \tfflush(NULL);\n \n \tenv[0] = env_counter;\n@@ -2940,8 +2939,9 @@ static void run_external_diff(const char *pgm,\n \tenv[1] = env_total;\n \tsnprintf(env_total, sizeof(env_total), \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n \n-\tretval = run_command_v_opt_cd_env(spawn_arg, RUN_USING_SHELL, NULL, env);\n+\tretval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env);\n \tremove_tempfile();\n+\targv_array_clear(&argv);\n \tif (retval) {\n \t\tfprintf(stderr, \"external diff died, stopping at %s.\\n\", name);\n \t\texit(1);\n-- \n1.9.1.656.ge8a0637\n"},{"id":"239129","messageId":"20140419191725.GB8981@sigill.intra.peff.net","threadId":"36447","inReplyTo":"20140419191128.GA9266@sigill.intra.peff.net","subject":"[PATCH 2/6] run_external_diff: use an argv_array for the environment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-19T19:17:25Z","receivedAt":"2014-04-19T19:17:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently use static buffers and a static array for\nformatting the environment passed to the external diff.\nThere's nothing wrong in the code, but it is much easier to\nverify that it is correct if we use a dynamic argv_array.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c | 14 +++++---------\n 1 file changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex b154284..760fc96 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2904,11 +2904,9 @@ static void run_external_diff(const char *pgm,\n \t\t\t      struct diff_options *o)\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n+\tstruct argv_array env = ARGV_ARRAY_INIT;\n \tint retval;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tconst char *env[3] = { NULL };\n-\tchar env_counter[50];\n-\tchar env_total[50];\n \n \tif (one && two) {\n \t\tstruct diff_tempfile *temp_one, *temp_two;\n@@ -2933,15 +2931,13 @@ static void run_external_diff(const char *pgm,\n \t}\n \tfflush(NULL);\n \n-\tenv[0] = env_counter;\n-\tsnprintf(env_counter, sizeof(env_counter), \"GIT_DIFF_PATH_COUNTER=%d\",\n-\t\t ++o->diff_path_counter);\n-\tenv[1] = env_total;\n-\tsnprintf(env_total, sizeof(env_total), \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n+\targv_array_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n+\targv_array_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n \n-\tretval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env);\n+\tretval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv);\n \tremove_tempfile();\n \targv_array_clear(&argv);\n+\targv_array_clear(&env);\n \tif (retval) {\n \t\tfprintf(stderr, \"external diff died, stopping at %s.\\n\", name);\n \t\texit(1);\n-- \n1.9.1.656.ge8a0637\n"},{"id":"239130","messageId":"20140419191919.GC8981@sigill.intra.peff.net","threadId":"36447","inReplyTo":"20140419191128.GA9266@sigill.intra.peff.net","subject":"[PATCH 3/6] run_external_diff: clean up error handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-19T19:19:19Z","receivedAt":"2014-04-19T19:19:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the external diff reports an error, we try to clean up\nand die. However, we can make this process a bit simpler:\n\n  1. We do not need to bother freeing memory, since we are\n     about to exit.  Nor do we need to clean up our\n     tempfiles, since the atexit() handler will do it for\n     us. So we can die as soon as we see the error.\n\n  3. We can just call die() rather than fprintf/exit. This\n     does technically change our exit code, but the exit\n     code of \"1\" is not meaningful here. In fact, it is\n     probably wrong, since \"1\" from diff usually means\n     \"completed successfully, but there were differences\".\n\nAnd while we're there, we can mark the error message for\ntranslation, and drop the full stop at the end to make it\nmore like our other messages.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNote that we do have to update one test, which was expecting difftool to\nexit with 1 (and difftool just propagates diff's exit status in this\ncase). I couldn't find any reasoning in the history for this exit(1),\nand it dates all the way back to May 2005. So I do not think it had any\nparticular purpose, and for the reasons above, I do not think anyone\nwould be sane to be relying on it.\n\n diff.c              | 9 +++------\n t/t7800-difftool.sh | 2 +-\n 2 files changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 760fc96..b517d01 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2905,7 +2905,6 @@ static void run_external_diff(const char *pgm,\n {\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tstruct argv_array env = ARGV_ARRAY_INIT;\n-\tint retval;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \n \tif (one && two) {\n@@ -2934,14 +2933,12 @@ static void run_external_diff(const char *pgm,\n \targv_array_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n \targv_array_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n \n-\tretval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv);\n+\tif (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv))\n+\t\tdie(_(\"external diff died, stopping at %s\"), name);\n+\n \tremove_tempfile();\n \targv_array_clear(&argv);\n \targv_array_clear(&env);\n-\tif (retval) {\n-\t\tfprintf(stderr, \"external diff died, stopping at %s.\\n\", name);\n-\t\texit(1);\n-\t}\n }\n \n static int similarity_index(struct diff_filepair *p)\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 5a193c5..dc30a51 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -58,7 +58,7 @@ test_expect_success PERL 'custom tool commands override built-ins' '\n \n test_expect_success PERL 'difftool ignores bad --tool values' '\n \t: >expect &&\n-\ttest_expect_code 1 \\\n+\ttest_must_fail \\\n \t\tgit difftool --no-prompt --tool=bad-tool branch >actual &&\n \ttest_cmp expect actual\n '\n-- \n1.9.1.656.ge8a0637\n"},{"id":"239131","messageId":"20140419191930.GD8981@sigill.intra.peff.net","threadId":"36447","inReplyTo":"20140419191128.GA9266@sigill.intra.peff.net","subject":"[PATCH 4/6] run_external_diff: drop fflush(NULL)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-19T19:19:30Z","receivedAt":"2014-04-19T19:19:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This fflush was added in d5535ec (Use run_command() to spawn\nexternal diff programs instead of fork/exec., 2007-10-19),\nbecause flushing buffers before forking is a good habit.\n\nBut later, 7d0b18a (Add output flushing before fork(),\n2008-08-04) added it to the generic run-command interface,\nmeaning that our flush here is redundant.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex b517d01..fdb7f6c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2928,7 +2928,6 @@ static void run_external_diff(const char *pgm,\n \t\targv_array_push(&argv, pgm);\n \t\targv_array_push(&argv, name);\n \t}\n-\tfflush(NULL);\n \n \targv_array_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n \targv_array_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n-- \n1.9.1.656.ge8a0637\n"},{"id":"239132","messageId":"20140419192045.GA9189@sigill.intra.peff.net","threadId":"36447","inReplyTo":"20140419191128.GA9266@sigill.intra.peff.net","subject":"[PATCH 5/6] run_external_diff: hoist common bits out of conditional","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-19T19:20:46Z","receivedAt":"2014-04-19T19:20:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Whether we have diff_filespecs to give to the diff command\nor not, we always are going to run the program and pass it\nthe pathname. Let's pull that duplicated part out of the\nconditional to make it more obvious.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex fdb7f6c..173b657 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2907,26 +2907,24 @@ static void run_external_diff(const char *pgm,\n \tstruct argv_array env = ARGV_ARRAY_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \n+\targv_array_push(&argv, pgm);\n+\targv_array_push(&argv, name);\n+\n \tif (one && two) {\n \t\tstruct diff_tempfile *temp_one, *temp_two;\n \t\tconst char *othername = (other ? other : name);\n \t\ttemp_one = prepare_temp_file(name, one);\n \t\ttemp_two = prepare_temp_file(othername, two);\n-\t\targv_array_push(&argv, pgm);\n-\t\targv_array_push(&argv, name);\n \t\targv_array_push(&argv, temp_one->name);\n \t\targv_array_push(&argv, temp_one->hex);\n \t\targv_array_push(&argv, temp_one->mode);\n \t\targv_array_push(&argv, temp_two->name);\n \t\targv_array_push(&argv, temp_two->hex);\n \t\targv_array_push(&argv, temp_two->mode);\n \t\tif (other) {\n \t\t\targv_array_push(&argv, other);\n \t\t\targv_array_push(&argv, xfrm_msg);\n \t\t}\n-\t} else {\n-\t\targv_array_push(&argv, pgm);\n-\t\targv_array_push(&argv, name);\n \t}\n \n \targv_array_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n-- \n1.9.1.656.ge8a0637\n"},{"id":"239133","messageId":"20140419192225.GB9189@sigill.intra.peff.net","threadId":"36447","inReplyTo":"20140419191128.GA9266@sigill.intra.peff.net","subject":"[PATCH 6/6] run_external_diff: refactor cmdline setup logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-19T19:22:25Z","receivedAt":"2014-04-19T19:22:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The current logic makes it hard to see what gets put onto\nthe command line in which cases. Pulling out a helper\nfunction lets us see that we have two sets of file data, and\nthe second set either uses the original name, or the \"other\"\nrenamed/copy name.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe last patch and this one are getting a little bit into code churn,\nperhaps. I think the prior one is hands-down more readable. This one, I\nam on the fence on whether it is a true improvement or simply \"this is\nhow _I_ would have written it\". I won't be offended if we drop it.\n\n diff.c | 26 +++++++++++++++-----------\n 1 file changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 173b657..4b8bfc6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2888,6 +2888,16 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \treturn temp;\n }\n \n+static void add_external_diff_name(struct argv_array *argv,\n+\t\t\t\t   const char *name,\n+\t\t\t\t   struct diff_filespec *df)\n+{\n+\tstruct diff_tempfile *temp = prepare_temp_file(name, df);\n+\targv_array_push(argv, temp->name);\n+\targv_array_push(argv, temp->hex);\n+\targv_array_push(argv, temp->mode);\n+}\n+\n /* An external diff command takes:\n  *\n  * diff-cmd name infile1 infile1-sha1 infile1-mode \\\n@@ -2911,17 +2921,11 @@ static void run_external_diff(const char *pgm,\n \targv_array_push(&argv, name);\n \n \tif (one && two) {\n-\t\tstruct diff_tempfile *temp_one, *temp_two;\n-\t\tconst char *othername = (other ? other : name);\n-\t\ttemp_one = prepare_temp_file(name, one);\n-\t\ttemp_two = prepare_temp_file(othername, two);\n-\t\targv_array_push(&argv, temp_one->name);\n-\t\targv_array_push(&argv, temp_one->hex);\n-\t\targv_array_push(&argv, temp_one->mode);\n-\t\targv_array_push(&argv, temp_two->name);\n-\t\targv_array_push(&argv, temp_two->hex);\n-\t\targv_array_push(&argv, temp_two->mode);\n-\t\tif (other) {\n+\t\tadd_external_diff_name(&argv, name, one);\n+\t\tif (!other)\n+\t\t\tadd_external_diff_name(&argv, name, two);\n+\t\telse {\n+\t\t\tadd_external_diff_name(&argv, other, two);\n \t\t\targv_array_push(&argv, other);\n \t\t\targv_array_push(&argv, xfrm_msg);\n \t\t}\n-- \n1.9.1.656.ge8a0637\n"},{"id":"239140","messageId":"CAA6TxYu=AX_RWQq4gr7cv6ZppPerRw30NY_Wbi1r8qEBma7Ctg@mail.gmail.com","threadId":"36447","inReplyTo":"20140419191706.GA8981@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] run_external_diff: use an argv_array for the command line","fromName":"Max L","fromEmail":"infthi.inbox@gmail.com","sentAt":"2014-04-19T22:09:49Z","receivedAt":"2014-04-19T22:09:49Z","isPatch":true,"sender":{"key":"infthi.inbox@gmail.com","avatar":null},"body":"One more note: at this moment the problem is slightly deeper. This\narray is next passed to the execvp function, which now falls with\nEFAULT on two my machines (both faced this problem after upgrading to\nubuntu 14.04, everything 'worked' fine before, looks like now execvp\nchecks input more strictly). This leads to non-working 'git difftool'.\n\n2014-04-19 23:17 GMT+04:00, Jeff King <peff@peff.net>:\n> We currently generate the command-line for the external\n> command using a fixed-length array of size 10. But if there\n> is a rename, we actually need 11 elements (10 items, plus a\n> NULL), and end up writing a random NULL onto the stack.\n>\n> Rather than bump the limit, let's just an argv_array, which\n> makes this sort of error impossible.\n>\n> Noticed-by: Max L <infthi.inbox@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> This was actually noticed by a GitHub user, who proposed bumping\n> the array size to 11:\n>\n>   https://github.com/git/git/pull/92\n>\n> Even though this fix is a bigger change, I'd rather do it this way, as\n> it is more obviously correct to a reader (and it solves the problem\n> forever). I pulled the name/email from that commit, but please let me\n> know if you'd prefer to be credited differently.\n>\n>  diff.c | 32 ++++++++++++++++----------------\n>  1 file changed, 16 insertions(+), 16 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 539997f..b154284 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -16,6 +16,7 @@\n>  #include \"submodule.h\"\n>  #include \"ll-merge.h\"\n>  #include \"string-list.h\"\n> +#include \"argv-array.h\"\n>\n>  #ifdef NO_FAST_WORKING_DIRECTORY\n>  #define FAST_WORKING_DIRECTORY 0\n> @@ -2902,9 +2903,8 @@ static void run_external_diff(const char *pgm,\n>  \t\t\t      int complete_rewrite,\n>  \t\t\t      struct diff_options *o)\n>  {\n> -\tconst char *spawn_arg[10];\n> +\tstruct argv_array argv = ARGV_ARRAY_INIT;\n>  \tint retval;\n> -\tconst char **arg = &spawn_arg[0];\n>  \tstruct diff_queue_struct *q = &diff_queued_diff;\n>  \tconst char *env[3] = { NULL };\n>  \tchar env_counter[50];\n> @@ -2915,23 +2915,22 @@ static void run_external_diff(const char *pgm,\n>  \t\tconst char *othername = (other ? other : name);\n>  \t\ttemp_one = prepare_temp_file(name, one);\n>  \t\ttemp_two = prepare_temp_file(othername, two);\n> -\t\t*arg++ = pgm;\n> -\t\t*arg++ = name;\n> -\t\t*arg++ = temp_one->name;\n> -\t\t*arg++ = temp_one->hex;\n> -\t\t*arg++ = temp_one->mode;\n> -\t\t*arg++ = temp_two->name;\n> -\t\t*arg++ = temp_two->hex;\n> -\t\t*arg++ = temp_two->mode;\n> +\t\targv_array_push(&argv, pgm);\n> +\t\targv_array_push(&argv, name);\n> +\t\targv_array_push(&argv, temp_one->name);\n> +\t\targv_array_push(&argv, temp_one->hex);\n> +\t\targv_array_push(&argv, temp_one->mode);\n> +\t\targv_array_push(&argv, temp_two->name);\n> +\t\targv_array_push(&argv, temp_two->hex);\n> +\t\targv_array_push(&argv, temp_two->mode);\n>  \t\tif (other) {\n> -\t\t\t*arg++ = other;\n> -\t\t\t*arg++ = xfrm_msg;\n> +\t\t\targv_array_push(&argv, other);\n> +\t\t\targv_array_push(&argv, xfrm_msg);\n>  \t\t}\n>  \t} else {\n> -\t\t*arg++ = pgm;\n> -\t\t*arg++ = name;\n> +\t\targv_array_push(&argv, pgm);\n> +\t\targv_array_push(&argv, name);\n>  \t}\n> -\t*arg = NULL;\n>  \tfflush(NULL);\n>\n>  \tenv[0] = env_counter;\n> @@ -2940,8 +2939,9 @@ static void run_external_diff(const char *pgm,\n>  \tenv[1] = env_total;\n>  \tsnprintf(env_total, sizeof(env_total), \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n>\n> -\tretval = run_command_v_opt_cd_env(spawn_arg, RUN_USING_SHELL, NULL, env);\n> +\tretval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env);\n>  \tremove_tempfile();\n> +\targv_array_clear(&argv);\n>  \tif (retval) {\n>  \t\tfprintf(stderr, \"external diff died, stopping at %s.\\n\", name);\n>  \t\texit(1);\n> --\n> 1.9.1.656.ge8a0637\n>\n>\n"},{"id":"239141","messageId":"CAPig+cR3AeuZ1ZnkKSocje0Yr2Eb37R2rgM=igEpdf6C5GEsvQ@mail.gmail.com","threadId":"36447","inReplyTo":"20140419191706.GA8981@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] run_external_diff: use an argv_array for the command line","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-04-20T01:20:52Z","receivedAt":"2014-04-20T01:20:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 19, 2014 at 3:17 PM, Jeff King <peff@peff.net> wrote:\n> We currently generate the command-line for the external\n> command using a fixed-length array of size 10. But if there\n> is a rename, we actually need 11 elements (10 items, plus a\n> NULL), and end up writing a random NULL onto the stack.\n>\n> Rather than bump the limit, let's just an argv_array, which\n\ns/just/just use/\n\n> makes this sort of error impossible.\n>\n> Noticed-by: Max L <infthi.inbox@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> This was actually noticed by a GitHub user, who proposed bumping\n> the array size to 11:\n>\n>   https://github.com/git/git/pull/92\n>\n> Even though this fix is a bigger change, I'd rather do it this way, as\n> it is more obviously correct to a reader (and it solves the problem\n> forever). I pulled the name/email from that commit, but please let me\n> know if you'd prefer to be credited differently.\n>\n>  diff.c | 32 ++++++++++++++++----------------\n>  1 file changed, 16 insertions(+), 16 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 539997f..b154284 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -16,6 +16,7 @@\n>  #include \"submodule.h\"\n>  #include \"ll-merge.h\"\n>  #include \"string-list.h\"\n> +#include \"argv-array.h\"\n>\n>  #ifdef NO_FAST_WORKING_DIRECTORY\n>  #define FAST_WORKING_DIRECTORY 0\n> @@ -2902,9 +2903,8 @@ static void run_external_diff(const char *pgm,\n>                               int complete_rewrite,\n>                               struct diff_options *o)\n>  {\n> -       const char *spawn_arg[10];\n> +       struct argv_array argv = ARGV_ARRAY_INIT;\n>         int retval;\n> -       const char **arg = &spawn_arg[0];\n>         struct diff_queue_struct *q = &diff_queued_diff;\n>         const char *env[3] = { NULL };\n>         char env_counter[50];\n> @@ -2915,23 +2915,22 @@ static void run_external_diff(const char *pgm,\n>                 const char *othername = (other ? other : name);\n>                 temp_one = prepare_temp_file(name, one);\n>                 temp_two = prepare_temp_file(othername, two);\n> -               *arg++ = pgm;\n> -               *arg++ = name;\n> -               *arg++ = temp_one->name;\n> -               *arg++ = temp_one->hex;\n> -               *arg++ = temp_one->mode;\n> -               *arg++ = temp_two->name;\n> -               *arg++ = temp_two->hex;\n> -               *arg++ = temp_two->mode;\n> +               argv_array_push(&argv, pgm);\n> +               argv_array_push(&argv, name);\n> +               argv_array_push(&argv, temp_one->name);\n> +               argv_array_push(&argv, temp_one->hex);\n> +               argv_array_push(&argv, temp_one->mode);\n> +               argv_array_push(&argv, temp_two->name);\n> +               argv_array_push(&argv, temp_two->hex);\n> +               argv_array_push(&argv, temp_two->mode);\n>                 if (other) {\n> -                       *arg++ = other;\n> -                       *arg++ = xfrm_msg;\n> +                       argv_array_push(&argv, other);\n> +                       argv_array_push(&argv, xfrm_msg);\n>                 }\n>         } else {\n> -               *arg++ = pgm;\n> -               *arg++ = name;\n> +               argv_array_push(&argv, pgm);\n> +               argv_array_push(&argv, name);\n>         }\n> -       *arg = NULL;\n>         fflush(NULL);\n>\n>         env[0] = env_counter;\n> @@ -2940,8 +2939,9 @@ static void run_external_diff(const char *pgm,\n>         env[1] = env_total;\n>         snprintf(env_total, sizeof(env_total), \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n>\n> -       retval = run_command_v_opt_cd_env(spawn_arg, RUN_USING_SHELL, NULL, env);\n> +       retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env);\n>         remove_tempfile();\n> +       argv_array_clear(&argv);\n>         if (retval) {\n>                 fprintf(stderr, \"external diff died, stopping at %s.\\n\", name);\n>                 exit(1);\n> --\n> 1.9.1.656.ge8a0637\n"},{"id":"239142","messageId":"20140420013500.GB10531@sigill.intra.peff.net","threadId":"36447","inReplyTo":"CAA6TxYu=AX_RWQq4gr7cv6ZppPerRw30NY_Wbi1r8qEBma7Ctg@mail.gmail.com","subject":"Re: [PATCH 1/6] run_external_diff: use an argv_array for the command line","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-20T01:35:00Z","receivedAt":"2014-04-20T01:35:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 20, 2014 at 02:09:49AM +0400, Max L wrote:\n\n> One more note: at this moment the problem is slightly deeper. This\n> array is next passed to the execvp function, which now falls with\n> EFAULT on two my machines (both faced this problem after upgrading to\n> ubuntu 14.04, everything 'worked' fine before, looks like now execvp\n> checks input more strictly). This leads to non-working 'git difftool'.\n\nInteresting. We're overwriting whatever is after spawn_arg on the stack,\nso I'd expect the fork/exec to work, but the function to complain while\npopping the stack frame (though I couldn't get it to do so). I wonder if\nsome kind of stack protection is kicking in, and the NULL doesn't get\nwritten or something. Either way, we should definitely address it.\n\n-Peff\n"}]}