run-command: do not pass child process data into callbacks

The expected way to pass data into the callback is to pass them via
the customizable callback pointer. The error reporting in
default_{start_failure, task_finished} is not user friendly enough, that
we want to encourage using the child data for such purposes.

Furthermore the struct child data is cleaned by the run-command API,
before we access them in the callbacks, leading to use-after-free
situations.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/run-command.c b/run-command.c
index 51fd72c4273c3ee59246285e40c9a5f9bd0e2f98..8e3ad07ffe823f9eb29eaf2794ab0549b4afd1ea 100644 (file)
--- a/run-command.c
+++ b/run-command.c
@@ -902,35 +902,18 @@ struct parallel_processes {
        struct strbuf buffered_output; /* of finished children */
 };
 
        struct strbuf buffered_output; /* of finished children */
 };
 

-static int default_start_failure(struct child_process *cp,
-                                struct strbuf *err,
+static int default_start_failure(struct strbuf *err,

                                 void *pp_cb,
                                 void *pp_task_cb)
 {
                                 void *pp_cb,
                                 void *pp_task_cb)
 {

-       int i;
-
-       strbuf_addstr(err, "Starting a child failed:");
-       for (i = 0; cp->argv[i]; i++)
-               strbuf_addf(err, " %s", cp->argv[i]);
-

        return 0;
 }
 
 static int default_task_finished(int result,
        return 0;
 }
 
 static int default_task_finished(int result,

-                                struct child_process *cp,

                                 struct strbuf *err,
                                 void *pp_cb,
                                 void *pp_task_cb)
 {
                                 struct strbuf *err,
                                 void *pp_cb,
                                 void *pp_task_cb)
 {

-       int i;
-
-       if (!result)
-               return 0;
-
-       strbuf_addf(err, "A child failed with return code %d:", result);
-       for (i = 0; cp->argv[i]; i++)
-               strbuf_addf(err, " %s", cp->argv[i]);
-

        return 0;
 }
 
        return 0;
 }
 


@@ -1048,8 +1031,7 @@ static int pp_start_one(struct parallel_processes *pp)
        pp->children[i].process.no_stdin = 1;
 
        if (start_command(&pp->children[i].process)) {
        pp->children[i].process.no_stdin = 1;
 
        if (start_command(&pp->children[i].process)) {

-               code = pp->start_failure(&pp->children[i].process,
-                                        &pp->children[i].err,
+               code = pp->start_failure(&pp->children[i].err,

                                         pp->data,
                                         &pp->children[i].data);
                strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
                                         pp->data,
                                         &pp->children[i].data);
                strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);


@@ -1117,7 +1099,7 @@ static int pp_collect_finished(struct parallel_processes *pp)
 
                code = finish_command(&pp->children[i].process);
 
 
                code = finish_command(&pp->children[i].process);
 

-               code = pp->task_finished(code, &pp->children[i].process,
+               code = pp->task_finished(code,

                                         &pp->children[i].err, pp->data,
                                         &pp->children[i].data);
 
                                         &pp->children[i].err, pp->data,
                                         &pp->children[i].data);
 

diff --git a/run-command.h b/run-command.h
index d5a57f922781de57d8ee6ae56f90af1456ccd5d5..6e17894f010fb59fb5d89ac5f4be5273fbd89162 100644 (file)
--- a/run-command.h
+++ b/run-command.h
@@ -158,8 +158,7 @@ typedef int (*get_next_task_fn)(struct child_process *cp,
  * To send a signal to other child processes for abortion, return
  * the negative signal number.
  */
  * To send a signal to other child processes for abortion, return
  * the negative signal number.
  */

-typedef int (*start_failure_fn)(struct child_process *cp,
-                               struct strbuf *err,
+typedef int (*start_failure_fn)(struct strbuf *err,

                                void *pp_cb,
                                void *pp_task_cb);
 
                                void *pp_cb,
                                void *pp_task_cb);
 


@@ -178,7 +177,6 @@ typedef int (*start_failure_fn)(struct child_process *cp,
  * the negative signal number.
  */
 typedef int (*task_finished_fn)(int result,
  * the negative signal number.
  */
 typedef int (*task_finished_fn)(int result,

-                               struct child_process *cp,

                                struct strbuf *err,
                                void *pp_cb,
                                void *pp_task_cb);
                                struct strbuf *err,
                                void *pp_cb,
                                void *pp_task_cb);


@@ -192,9 +190,8 @@ typedef int (*task_finished_fn)(int result,
  * (both stdout and stderr) is routed to stderr in a manner that output
  * from different tasks does not interleave.
  *
  * (both stdout and stderr) is routed to stderr in a manner that output
  * from different tasks does not interleave.
  *

- * If start_failure_fn or task_finished_fn are NULL, default handlers
- * will be used. The default handlers will print an error message on
- * error without issuing an emergency stop.
+ * start_failure_fn and task_finished_fn can be NULL to omit any
+ * special handling.

  */
 int run_processes_parallel(int n,
                           get_next_task_fn,
  */
 int run_processes_parallel(int n,
                           get_next_task_fn,

diff --git a/submodule.c b/submodule.c
index b83939c294782ac59e5c78c8882064a1e272e309..916fc84aaabf7cc4d2974d6b930c40fbcbfb8bb1 100644 (file)
--- a/submodule.c
+++ b/submodule.c
@@ -705,8 +705,7 @@ static int get_next_submodule(struct child_process *cp,
        return 0;
 }
 
        return 0;
 }
 

-static int fetch_start_failure(struct child_process *cp,
-                              struct strbuf *err,
+static int fetch_start_failure(struct strbuf *err,

                               void *cb, void *task_cb)
 {
        struct submodule_parallel_fetch *spf = cb;
                               void *cb, void *task_cb)
 {
        struct submodule_parallel_fetch *spf = cb;


@@ -716,8 +715,8 @@ static int fetch_start_failure(struct child_process *cp,
        return 0;
 }
 
        return 0;
 }
 

-static int fetch_finish(int retvalue, struct child_process *cp,
-                       struct strbuf *err, void *cb, void *task_cb)
+static int fetch_finish(int retvalue, struct strbuf *err,
+                       void *cb, void *task_cb)

 {
        struct submodule_parallel_fetch *spf = cb;
 
 {
        struct submodule_parallel_fetch *spf = cb;
 

diff --git a/test-run-command.c b/test-run-command.c
index fbe0a27ef3295fe3312fb72a478571bfb450cb25..30a64a98dc8b53a3a2d7edfaa57a8ee51d5d63e7 100644 (file)
--- a/test-run-command.c
+++ b/test-run-command.c
@@ -41,7 +41,6 @@ static int no_job(struct child_process *cp,
 }
 
 static int task_finished(int result,
 }
 
 static int task_finished(int result,

-                        struct child_process *cp,

                         struct strbuf *err,
                         void *pp_cb,
                         void *pp_task_cb)
                         struct strbuf *err,
                         void *pp_cb,
                         void *pp_task_cb)