[PATCH 0/6] run_external_diff cleanups

[PATCH 0/6] run_external_diff cleanups

[Jeff King]

It's possible to overflow an array in run_external_diff and write a
single NULL onto the stack. The first patch below fixes that. The rest
are cleanups and modernizations I noticed while in the area. It's
possible that patch 3 is also a bug fix, depending on your
interpretation.

  [1/6]: run_external_diff: use an argv_array for the command line
  [2/6]: run_external_diff: use an argv_array for the environment
  [3/6]: run_external_diff: clean up error handling
  [4/6]: run_external_diff: drop fflush(NULL)
  [5/6]: run_external_diff: hoist common bits out of conditional
  [6/6]: run_external_diff: refactor cmdline setup logic

-Peff

[PATCH 1/6] run_external_diff: use an argv_array for the command line

[Jeff King]

We currently generate the command-line for the external
command using a fixed-length array of size 10. But if there
is a rename, we actually need 11 elements (10 items, plus a
NULL), and end up writing a random NULL onto the stack.

Rather than bump the limit, let's just an argv_array, which
makes this sort of error impossible.

Noticed-by: Max L <infthi.inbox@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
This was actually noticed by a GitHub user, who proposed bumping
the array size to 11:

  https://github.com/git/git/pull/92

Even though this fix is a bigger change, I'd rather do it this way, as
it is more obviously correct to a reader (and it solves the problem
forever). I pulled the name/email from that commit, but please let me
know if you'd prefer to be credited differently.

 diff.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/diff.c b/diff.c
index 539997f..b154284 100644
--- a/diff.c
+++ b/diff.c
@@ -16,6 +16,7 @@
 #include "submodule.h"
 #include "ll-merge.h"
 #include "string-list.h"
+#include "argv-array.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -2902,9 +2903,8 @@ static void run_external_diff(const char *pgm,
 			      int complete_rewrite,
 			      struct diff_options *o)
 {
-	const char *spawn_arg[10];
+	struct argv_array argv = ARGV_ARRAY_INIT;
 	int retval;
-	const char **arg = &spawn_arg[0];
 	struct diff_queue_struct *q = &diff_queued_diff;
 	const char *env[3] = { NULL };
 	char env_counter[50];
@@ -2915,23 +2915,22 @@ static void run_external_diff(const char *pgm,
 		const char *othername = (other ? other : name);
 		temp_one = prepare_temp_file(name, one);
 		temp_two = prepare_temp_file(othername, two);
-		*arg++ = pgm;
-		*arg++ = name;
-		*arg++ = temp_one->name;
-		*arg++ = temp_one->hex;
-		*arg++ = temp_one->mode;
-		*arg++ = temp_two->name;
-		*arg++ = temp_two->hex;
-		*arg++ = temp_two->mode;
+		argv_array_push(&argv, pgm);
+		argv_array_push(&argv, name);
+		argv_array_push(&argv, temp_one->name);
+		argv_array_push(&argv, temp_one->hex);
+		argv_array_push(&argv, temp_one->mode);
+		argv_array_push(&argv, temp_two->name);
+		argv_array_push(&argv, temp_two->hex);
+		argv_array_push(&argv, temp_two->mode);
 		if (other) {
-			*arg++ = other;
-			*arg++ = xfrm_msg;
+			argv_array_push(&argv, other);
+			argv_array_push(&argv, xfrm_msg);
 		}
 	} else {
-		*arg++ = pgm;
-		*arg++ = name;
+		argv_array_push(&argv, pgm);
+		argv_array_push(&argv, name);
 	}
-	*arg = NULL;
 	fflush(NULL);
 
 	env[0] = env_counter;
@@ -2940,8 +2939,9 @@ static void run_external_diff(const char *pgm,
 	env[1] = env_total;
 	snprintf(env_total, sizeof(env_total), "GIT_DIFF_PATH_TOTAL=%d", q->nr);
 
-	retval = run_command_v_opt_cd_env(spawn_arg, RUN_USING_SHELL, NULL, env);
+	retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env);
 	remove_tempfile();
+	argv_array_clear(&argv);
 	if (retval) {
 		fprintf(stderr, "external diff died, stopping at %s.\n", name);
 		exit(1);
-- 
1.9.1.656.ge8a0637

[PATCH 2/6] run_external_diff: use an argv_array for the environment

[Jeff King]

We currently use static buffers and a static array for
formatting the environment passed to the external diff.
There's nothing wrong in the code, but it is much easier to
verify that it is correct if we use a dynamic argv_array.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/diff.c b/diff.c
index b154284..760fc96 100644
--- a/diff.c
+++ b/diff.c
@@ -2904,11 +2904,9 @@ static void run_external_diff(const char *pgm,
 			      struct diff_options *o)
 {
 	struct argv_array argv = ARGV_ARRAY_INIT;
+	struct argv_array env = ARGV_ARRAY_INIT;
 	int retval;
 	struct diff_queue_struct *q = &diff_queued_diff;
-	const char *env[3] = { NULL };
-	char env_counter[50];
-	char env_total[50];
 
 	if (one && two) {
 		struct diff_tempfile *temp_one, *temp_two;
@@ -2933,15 +2931,13 @@ static void run_external_diff(const char *pgm,
 	}
 	fflush(NULL);
 
-	env[0] = env_counter;
-	snprintf(env_counter, sizeof(env_counter), "GIT_DIFF_PATH_COUNTER=%d",
-		 ++o->diff_path_counter);
-	env[1] = env_total;
-	snprintf(env_total, sizeof(env_total), "GIT_DIFF_PATH_TOTAL=%d", q->nr);
+	argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
+	argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
 
-	retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env);
+	retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv);
 	remove_tempfile();
 	argv_array_clear(&argv);
+	argv_array_clear(&env);
 	if (retval) {
 		fprintf(stderr, "external diff died, stopping at %s.\n", name);
 		exit(1);
-- 
1.9.1.656.ge8a0637

[PATCH 3/6] run_external_diff: clean up error handling

[Jeff King]

When the external diff reports an error, we try to clean up
and die. However, we can make this process a bit simpler:

  1. We do not need to bother freeing memory, since we are
     about to exit.  Nor do we need to clean up our
     tempfiles, since the atexit() handler will do it for
     us. So we can die as soon as we see the error.

  3. We can just call die() rather than fprintf/exit. This
     does technically change our exit code, but the exit
     code of "1" is not meaningful here. In fact, it is
     probably wrong, since "1" from diff usually means
     "completed successfully, but there were differences".

And while we're there, we can mark the error message for
translation, and drop the full stop at the end to make it
more like our other messages.

Signed-off-by: Jeff King <peff@peff.net>
---
Note that we do have to update one test, which was expecting difftool to
exit with 1 (and difftool just propagates diff's exit status in this
case). I couldn't find any reasoning in the history for this exit(1),
and it dates all the way back to May 2005. So I do not think it had any
particular purpose, and for the reasons above, I do not think anyone
would be sane to be relying on it.

 diff.c              | 9 +++------
 t/t7800-difftool.sh | 2 +-
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/diff.c b/diff.c
index 760fc96..b517d01 100644
--- a/diff.c
+++ b/diff.c
@@ -2905,7 +2905,6 @@ static void run_external_diff(const char *pgm,
 {
 	struct argv_array argv = ARGV_ARRAY_INIT;
 	struct argv_array env = ARGV_ARRAY_INIT;
-	int retval;
 	struct diff_queue_struct *q = &diff_queued_diff;
 
 	if (one && two) {
@@ -2934,14 +2933,12 @@ static void run_external_diff(const char *pgm,
 	argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
 	argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
 
-	retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv);
+	if (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv))
+		die(_("external diff died, stopping at %s"), name);
+
 	remove_tempfile();
 	argv_array_clear(&argv);
 	argv_array_clear(&env);
-	if (retval) {
-		fprintf(stderr, "external diff died, stopping at %s.\n", name);
-		exit(1);
-	}
 }
 
 static int similarity_index(struct diff_filepair *p)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 5a193c5..dc30a51 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -58,7 +58,7 @@ test_expect_success PERL 'custom tool commands override built-ins' '
 
 test_expect_success PERL 'difftool ignores bad --tool values' '
 	: >expect &&
-	test_expect_code 1 \
+	test_must_fail \
 		git difftool --no-prompt --tool=bad-tool branch >actual &&
 	test_cmp expect actual
 '
-- 
1.9.1.656.ge8a0637

[PATCH 4/6] run_external_diff: drop fflush(NULL)

[Jeff King]

This fflush was added in d5535ec (Use run_command() to spawn
external diff programs instead of fork/exec., 2007-10-19),
because flushing buffers before forking is a good habit.

But later, 7d0b18a (Add output flushing before fork(),
2008-08-04) added it to the generic run-command interface,
meaning that our flush here is redundant.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/diff.c b/diff.c
index b517d01..fdb7f6c 100644
--- a/diff.c
+++ b/diff.c
@@ -2928,7 +2928,6 @@ static void run_external_diff(const char *pgm,
 		argv_array_push(&argv, pgm);
 		argv_array_push(&argv, name);
 	}
-	fflush(NULL);
 
 	argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
 	argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
-- 
1.9.1.656.ge8a0637

[PATCH 5/6] run_external_diff: hoist common bits out of conditional

[Jeff King]

Whether we have diff_filespecs to give to the diff command
or not, we always are going to run the program and pass it
the pathname. Let's pull that duplicated part out of the
conditional to make it more obvious.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/diff.c b/diff.c
index fdb7f6c..173b657 100644
--- a/diff.c
+++ b/diff.c
@@ -2907,26 +2907,24 @@ static void run_external_diff(const char *pgm,
 	struct argv_array env = ARGV_ARRAY_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;
 
+	argv_array_push(&argv, pgm);
+	argv_array_push(&argv, name);
+
 	if (one && two) {
 		struct diff_tempfile *temp_one, *temp_two;
 		const char *othername = (other ? other : name);
 		temp_one = prepare_temp_file(name, one);
 		temp_two = prepare_temp_file(othername, two);
-		argv_array_push(&argv, pgm);
-		argv_array_push(&argv, name);
 		argv_array_push(&argv, temp_one->name);
 		argv_array_push(&argv, temp_one->hex);
 		argv_array_push(&argv, temp_one->mode);
 		argv_array_push(&argv, temp_two->name);
 		argv_array_push(&argv, temp_two->hex);
 		argv_array_push(&argv, temp_two->mode);
 		if (other) {
 			argv_array_push(&argv, other);
 			argv_array_push(&argv, xfrm_msg);
 		}
-	} else {
-		argv_array_push(&argv, pgm);
-		argv_array_push(&argv, name);
 	}
 
 	argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
-- 
1.9.1.656.ge8a0637

[PATCH 6/6] run_external_diff: refactor cmdline setup logic

[Jeff King]

The current logic makes it hard to see what gets put onto
the command line in which cases. Pulling out a helper
function lets us see that we have two sets of file data, and
the second set either uses the original name, or the "other"
renamed/copy name.

Signed-off-by: Jeff King <peff@peff.net>
---
The last patch and this one are getting a little bit into code churn,
perhaps. I think the prior one is hands-down more readable. This one, I
am on the fence on whether it is a true improvement or simply "this is
how _I_ would have written it". I won't be offended if we drop it.

 diff.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/diff.c b/diff.c
index 173b657..4b8bfc6 100644
--- a/diff.c
+++ b/diff.c
@@ -2888,6 +2888,16 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
 	return temp;
 }
 
+static void add_external_diff_name(struct argv_array *argv,
+				   const char *name,
+				   struct diff_filespec *df)
+{
+	struct diff_tempfile *temp = prepare_temp_file(name, df);
+	argv_array_push(argv, temp->name);
+	argv_array_push(argv, temp->hex);
+	argv_array_push(argv, temp->mode);
+}
+
 /* An external diff command takes:
  *
  * diff-cmd name infile1 infile1-sha1 infile1-mode \
@@ -2911,17 +2921,11 @@ static void run_external_diff(const char *pgm,
 	argv_array_push(&argv, name);
 
 	if (one && two) {
-		struct diff_tempfile *temp_one, *temp_two;
-		const char *othername = (other ? other : name);
-		temp_one = prepare_temp_file(name, one);
-		temp_two = prepare_temp_file(othername, two);
-		argv_array_push(&argv, temp_one->name);
-		argv_array_push(&argv, temp_one->hex);
-		argv_array_push(&argv, temp_one->mode);
-		argv_array_push(&argv, temp_two->name);
-		argv_array_push(&argv, temp_two->hex);
-		argv_array_push(&argv, temp_two->mode);
-		if (other) {
+		add_external_diff_name(&argv, name, one);
+		if (!other)
+			add_external_diff_name(&argv, name, two);
+		else {
+			add_external_diff_name(&argv, other, two);
 			argv_array_push(&argv, other);
 			argv_array_push(&argv, xfrm_msg);
 		}
-- 
1.9.1.656.ge8a0637

Re: [PATCH 1/6] run_external_diff: use an argv_array for the command line

[Max L]

One more note: at this moment the problem is slightly deeper. This
array is next passed to the execvp function, which now falls with
EFAULT on two my machines (both faced this problem after upgrading to
ubuntu 14.04, everything 'worked' fine before, looks like now execvp
checks input more strictly). This leads to non-working 'git difftool'.

2014-04-19 23:17 GMT+04:00, Jeff King <peff@peff.net>:
> We currently generate the command-line for the external
> command using a fixed-length array of size 10. But if there
> is a rename, we actually need 11 elements (10 items, plus a
> NULL), and end up writing a random NULL onto the stack.
>
> Rather than bump the limit, let's just an argv_array, which
> makes this sort of error impossible.
>
> Noticed-by: Max L <infthi.inbox@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This was actually noticed by a GitHub user, who proposed bumping
> the array size to 11:
>
>   https://github.com/git/git/pull/92
>
> Even though this fix is a bigger change, I'd rather do it this way, as
> it is more obviously correct to a reader (and it solves the problem
> forever). I pulled the name/email from that commit, but please let me
> know if you'd prefer to be credited differently.
>
>  diff.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 539997f..b154284 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -16,6 +16,7 @@
>  #include "submodule.h"
>  #include "ll-merge.h"
>  #include "string-list.h"
> +#include "argv-array.h"
>
>  #ifdef NO_FAST_WORKING_DIRECTORY
>  #define FAST_WORKING_DIRECTORY 0
> @@ -2902,9 +2903,8 @@ static void run_external_diff(const char *pgm,
>  			      int complete_rewrite,
>  			      struct diff_options *o)
>  {
> -	const char *spawn_arg[10];
> +	struct argv_array argv = ARGV_ARRAY_INIT;
>  	int retval;
> -	const char **arg = &spawn_arg[0];
>  	struct diff_queue_struct *q = &diff_queued_diff;
>  	const char *env[3] = { NULL };
>  	char env_counter[50];
> @@ -2915,23 +2915,22 @@ static void run_external_diff(const char *pgm,
>  		const char *othername = (other ? other : name);
>  		temp_one = prepare_temp_file(name, one);
>  		temp_two = prepare_temp_file(othername, two);
> -		*arg++ = pgm;
> -		*arg++ = name;
> -		*arg++ = temp_one->name;
> -		*arg++ = temp_one->hex;
> -		*arg++ = temp_one->mode;
> -		*arg++ = temp_two->name;
> -		*arg++ = temp_two->hex;
> -		*arg++ = temp_two->mode;
> +		argv_array_push(&argv, pgm);
> +		argv_array_push(&argv, name);
> +		argv_array_push(&argv, temp_one->name);
> +		argv_array_push(&argv, temp_one->hex);
> +		argv_array_push(&argv, temp_one->mode);
> +		argv_array_push(&argv, temp_two->name);
> +		argv_array_push(&argv, temp_two->hex);
> +		argv_array_push(&argv, temp_two->mode);
>  		if (other) {
> -			*arg++ = other;
> -			*arg++ = xfrm_msg;
> +			argv_array_push(&argv, other);
> +			argv_array_push(&argv, xfrm_msg);
>  		}
>  	} else {
> -		*arg++ = pgm;
> -		*arg++ = name;
> +		argv_array_push(&argv, pgm);
> +		argv_array_push(&argv, name);
>  	}
> -	*arg = NULL;
>  	fflush(NULL);
>
>  	env[0] = env_counter;
> @@ -2940,8 +2939,9 @@ static void run_external_diff(const char *pgm,
>  	env[1] = env_total;
>  	snprintf(env_total, sizeof(env_total), "GIT_DIFF_PATH_TOTAL=%d", q->nr);
>
> -	retval = run_command_v_opt_cd_env(spawn_arg, RUN_USING_SHELL, NULL, env);
> +	retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env);
>  	remove_tempfile();
> +	argv_array_clear(&argv);
>  	if (retval) {
>  		fprintf(stderr, "external diff died, stopping at %s.\n", name);
>  		exit(1);
> --
> 1.9.1.656.ge8a0637
>
>

Re: [PATCH 1/6] run_external_diff: use an argv_array for the command line

[Eric Sunshine]

On Sat, Apr 19, 2014 at 3:17 PM, Jeff King <peff@peff.net> wrote:
> We currently generate the command-line for the external
> command using a fixed-length array of size 10. But if there
> is a rename, we actually need 11 elements (10 items, plus a
> NULL), and end up writing a random NULL onto the stack.
>
> Rather than bump the limit, let's just an argv_array, which

s/just/just use/

> makes this sort of error impossible.
>
> Noticed-by: Max L <infthi.inbox@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This was actually noticed by a GitHub user, who proposed bumping
> the array size to 11:
>
>   https://github.com/git/git/pull/92
>
> Even though this fix is a bigger change, I'd rather do it this way, as
> it is more obviously correct to a reader (and it solves the problem
> forever). I pulled the name/email from that commit, but please let me
> know if you'd prefer to be credited differently.
>
>  diff.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 539997f..b154284 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -16,6 +16,7 @@
>  #include "submodule.h"
>  #include "ll-merge.h"
>  #include "string-list.h"
> +#include "argv-array.h"
>
>  #ifdef NO_FAST_WORKING_DIRECTORY
>  #define FAST_WORKING_DIRECTORY 0
> @@ -2902,9 +2903,8 @@ static void run_external_diff(const char *pgm,
>                               int complete_rewrite,
>                               struct diff_options *o)
>  {
> -       const char *spawn_arg[10];
> +       struct argv_array argv = ARGV_ARRAY_INIT;
>         int retval;
> -       const char **arg = &spawn_arg[0];
>         struct diff_queue_struct *q = &diff_queued_diff;
>         const char *env[3] = { NULL };
>         char env_counter[50];
> @@ -2915,23 +2915,22 @@ static void run_external_diff(const char *pgm,
>                 const char *othername = (other ? other : name);
>                 temp_one = prepare_temp_file(name, one);
>                 temp_two = prepare_temp_file(othername, two);
> -               *arg++ = pgm;
> -               *arg++ = name;
> -               *arg++ = temp_one->name;
> -               *arg++ = temp_one->hex;
> -               *arg++ = temp_one->mode;
> -               *arg++ = temp_two->name;
> -               *arg++ = temp_two->hex;
> -               *arg++ = temp_two->mode;
> +               argv_array_push(&argv, pgm);
> +               argv_array_push(&argv, name);
> +               argv_array_push(&argv, temp_one->name);
> +               argv_array_push(&argv, temp_one->hex);
> +               argv_array_push(&argv, temp_one->mode);
> +               argv_array_push(&argv, temp_two->name);
> +               argv_array_push(&argv, temp_two->hex);
> +               argv_array_push(&argv, temp_two->mode);
>                 if (other) {
> -                       *arg++ = other;
> -                       *arg++ = xfrm_msg;
> +                       argv_array_push(&argv, other);
> +                       argv_array_push(&argv, xfrm_msg);
>                 }
>         } else {
> -               *arg++ = pgm;
> -               *arg++ = name;
> +               argv_array_push(&argv, pgm);
> +               argv_array_push(&argv, name);
>         }
> -       *arg = NULL;
>         fflush(NULL);
>
>         env[0] = env_counter;
> @@ -2940,8 +2939,9 @@ static void run_external_diff(const char *pgm,
>         env[1] = env_total;
>         snprintf(env_total, sizeof(env_total), "GIT_DIFF_PATH_TOTAL=%d", q->nr);
>
> -       retval = run_command_v_opt_cd_env(spawn_arg, RUN_USING_SHELL, NULL, env);
> +       retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env);
>         remove_tempfile();
> +       argv_array_clear(&argv);
>         if (retval) {
>                 fprintf(stderr, "external diff died, stopping at %s.\n", name);
>                 exit(1);
> --
> 1.9.1.656.ge8a0637

Re: [PATCH 1/6] run_external_diff: use an argv_array for the command line

[Jeff King]

On Sun, Apr 20, 2014 at 02:09:49AM +0400, Max L wrote:

> One more note: at this moment the problem is slightly deeper. This
> array is next passed to the execvp function, which now falls with
> EFAULT on two my machines (both faced this problem after upgrading to
> ubuntu 14.04, everything 'worked' fine before, looks like now execvp
> checks input more strictly). This leads to non-working 'git difftool'.

Interesting. We're overwriting whatever is after spawn_arg on the stack,
so I'd expect the fork/exec to work, but the function to complain while
popping the stack frame (though I couldn't get it to do so). I wonder if
some kind of stack protection is kicking in, and the NULL doesn't get
written or something. Either way, we should definitely address it.

-Peff