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

stefanbeller · gitster · commit 2a73b3dad09e · 2016-03-01T09:42:01.000-08:00
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 &lt;sbeller@google.com&gt;
Signed-off-by: Junio C Hamano &lt;gitster@pobox.com&gt;
diff --git a/run-command.c b/run-command.c
@@ -902,35 +902,18 @@ struct parallel_processes {
 	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)
 {
-	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,
-				 struct child_process *cp,
 				 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;
 }
 
@@ -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)) {
-		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);
@@ -1117,7 +1099,7 @@ static int pp_collect_finished(struct parallel_processes *pp)
 
 		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);
 
diff --git 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.
  */
-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);
 
@@ -178,7 +177,6 @@ typedef int (*start_failure_fn)(struct child_process *cp,
  * 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);
@@ -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.
  *
- * 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,
diff --git a/submodule.c b/submodule.c
@@ -705,8 +705,7 @@ static int get_next_submodule(struct child_process *cp,
 	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;
@@ -716,8 +715,8 @@ static int fetch_start_failure(struct child_process *cp,
 	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;
 
diff --git 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,
-			 struct child_process *cp,
 			 struct strbuf *err,
 			 void *pp_cb,
 			 void *pp_task_cb)

Original file line number	Diff line number	Diff line change
`@@ -705,8 +705,7 @@ static int get_next_submodule(struct child_process *cp,`
`705`	`705`	`return 0;`
`706`	`706`	`}`
`707`	`707`
`708`		`-static int fetch_start_failure(struct child_process *cp,`
`709`		`- struct strbuf *err,`
	`708`	`+static int fetch_start_failure(struct strbuf *err,`
`710`	`709`	`void cb, void task_cb)`
`711`	`710`	`{`
`712`	`711`	`struct submodule_parallel_fetch *spf = cb;`
`@@ -716,8 +715,8 @@ static int fetch_start_failure(struct child_process *cp,`
`716`	`715`	`return 0;`
`717`	`716`	`}`
`718`	`717`
`719`		`-static int fetch_finish(int retvalue, struct child_process *cp,`
`720`		`- struct strbuf err, void cb, void *task_cb)`
	`718`	`+static int fetch_finish(int retvalue, struct strbuf *err,`
	`719`	`+ void cb, void task_cb)`
`721`	`720`	`{`
`722`	`721`	`struct submodule_parallel_fetch *spf = cb;`
`723`	`722`
Original file line number	Diff line number	Diff line change
`@@ -41,7 +41,6 @@ static int no_job(struct child_process *cp,`
`41`	`41`	`}`
`42`	`42`
`43`	`43`	`static int task_finished(int result,`
`44`		`- struct child_process *cp,`
`45`	`44`	`struct strbuf *err,`
`46`	`45`	`void *pp_cb,`
`47`	`46`	`void *pp_task_cb)`