# [PATCH] run-command: do not pass child process data into callbacks 6 messages from 2016-02-29 to 2016-03-01. Participants: Stefan Beller, Junio C Hamano, Johannes Sixt. Thread: https://gitlist.dev/t/41572 ## Stefan Beller, 2016-02-29 21:57 Subject: [PATCH] run-command: do not pass child process data into callbacks Message-ID: <1456783026-3328-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1456783026-3328-1-git-send-email-sbeller%40google.com ``` 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 --- This is the proper fix to not access memory after freeing. run-command.c | 24 +++--------------------- run-command.h | 4 +--- submodule.c | 7 +++---- test-run-command.c | 1 - 4 files changed, 7 insertions(+), 29 deletions(-) diff --git a/run-command.c b/run-command.c index 863dad5..c726010 100644 --- 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 index 42917e8..c6a3e42 100644 --- a/run-command.h +++ b/run-command.h @@ -159,8 +159,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); @@ -179,7 +178,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); diff --git a/submodule.c b/submodule.c index 24fb81a..62c4356 100644 --- 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 index fbe0a27..30a64a9 100644 --- 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) -- 2.8.0.rc0.1.g68b4e3f ``` ## Stefan Beller, 2016-02-29 21:58 Subject: Re: [PATCH] run-command: do not pass child process data into callbacks Message-ID: URL: https://gitlist.dev/e/CAGZ79kb2h%3D4qeGbQ%2B42Zkq5wZ6eVu1rNQn%3Dd1R6Rz4YA-q0xwg%40mail.gmail.com In-Reply-To: <1456783026-3328-1-git-send-email-sbeller@google.com> ``` It applies on 2.8.0-rc0. (I tried backporting it to 2.7, but realized it is not an issue there) ``` ## Junio C Hamano, 2016-02-29 23:53 Subject: Re: [PATCH] run-command: do not pass child process data into callbacks Message-ID: URL: https://gitlist.dev/e/xmqqoaaz845n.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Stefan Beller writes: > It applies on 2.8.0-rc0. > > (I tried backporting it to 2.7, but realized it is not an issue there) Thanks. I tried to be nice to rebase the parallel-update stuff on top of this myself, but stopped it as I won't have enough time to push the result out for the day anyway. submodule-parallel-update, submodule-init and refs-backend-lmdb topics will be missing from tonight's 'pu'. ``` ## Johannes Sixt, 2016-03-01 07:13 Subject: Re: [PATCH] run-command: do not pass child process data into callbacks Message-ID: <56D54129.4090007@kdbg.org> URL: https://gitlist.dev/e/56D54129.4090007%40kdbg.org In-Reply-To: <1456783026-3328-1-git-send-email-sbeller@google.com> ``` Am 29.02.2016 um 22:57 schrieb Stefan Beller: > 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. Thanks. The code changes match what I had prototyped. But please squash in this documentation change: diff --git a/run-command.h b/run-command.h index c6a3e42..3d1e59e 100644 --- a/run-command.h +++ b/run-command.h @@ -191,9 +191,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, ``` ## Junio C Hamano, 2016-03-01 17:43 Subject: Re: [PATCH] run-command: do not pass child process data into callbacks Message-ID: URL: https://gitlist.dev/e/xmqqd1re856r.fsf%40gitster.mtv.corp.google.com In-Reply-To: <56D54129.4090007@kdbg.org> ``` Johannes Sixt writes: > Am 29.02.2016 um 22:57 schrieb Stefan Beller: >> 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. > > Thanks. The code changes match what I had prototyped. But please squash > in this documentation change: > > diff --git a/run-command.h b/run-command.h > index c6a3e42..3d1e59e 100644 > --- a/run-command.h > +++ b/run-command.h > @@ -191,9 +191,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, Thanks for careful reading. ``` ## Stefan Beller, 2016-03-01 17:55 Subject: [PATCHv2] run-command: do not pass child process data into callbacks Message-ID: <1456854926-13838-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1456854926-13838-1-git-send-email-sbeller%40google.com In-Reply-To: ``` 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 --- Thanks Johannes for reviewing and double checking. I squashed in your proposed documentation change. This applies on 2.8.0-rc0. Thanks, Stefan run-command.c | 24 +++--------------------- run-command.h | 9 +++------ submodule.c | 7 +++---- test-run-command.c | 1 - 4 files changed, 9 insertions(+), 32 deletions(-) diff --git a/run-command.c b/run-command.c index 51fd72c..8e3ad07 100644 --- 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 index d5a57f9..6e17894 100644 --- 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 index b83939c..916fc84 100644 --- 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 index fbe0a27..30a64a9 100644 --- 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) -- 2.8.0.rc0.1.g68b4e3f ```