subject:"\[PATCH 1\/6\] run_external_diff\: use an argv

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

2014-04-19 Thread 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.in...@gmail.com
Signed-off-by: Jeff King p...@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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-04-19 Thread 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 p...@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.in...@gmail.com
 Signed-off-by: Jeff King p...@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


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-04-19 Thread Eric Sunshine

On Sat, Apr 19, 2014 at 3:17 PM, Jeff King p...@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.in...@gmail.com
 Signed-off-by: Jeff King p...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-04-19 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information