{"thread":{"id":"41572","subject":"[PATCH] run-command: do not pass child process data into callbacks","startedAt":"2016-02-29T21:57:06Z","lastAt":"2016-03-01T17:55:26Z","messageCount":6,"participants":["Stefan Beller","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"279855","messageId":"1456783026-3328-1-git-send-email-sbeller@google.com","threadId":"41572","inReplyTo":null,"subject":"[PATCH] run-command: do not pass child process data into callbacks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-29T21:57:06Z","receivedAt":"2016-02-29T21:57:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The expected way to pass data into the callback is to pass them via\nthe customizable callback pointer. The error reporting in\ndefault_{start_failure, task_finished} is not user friendly enough, that\nwe want to encourage using the child data for such purposes.\n\nFurthermore the struct child data is cleaned by the run-command API,\nbefore we access them in the callbacks, leading to use-after-free\nsituations.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n This is the proper fix to not access memory after freeing.\n\n run-command.c      | 24 +++---------------------\n run-command.h      |  4 +---\n submodule.c        |  7 +++----\n test-run-command.c |  1 -\n 4 files changed, 7 insertions(+), 29 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 863dad5..c726010 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -902,35 +902,18 @@ struct parallel_processes {\n \tstruct strbuf buffered_output; /* of finished children */\n };\n \n-static int default_start_failure(struct child_process *cp,\n-\t\t\t\t struct strbuf *err,\n+static int default_start_failure(struct strbuf *err,\n \t\t\t\t void *pp_cb,\n \t\t\t\t void *pp_task_cb)\n {\n-\tint i;\n-\n-\tstrbuf_addstr(err, \"Starting a child failed:\");\n-\tfor (i = 0; cp->argv[i]; i++)\n-\t\tstrbuf_addf(err, \" %s\", cp->argv[i]);\n-\n \treturn 0;\n }\n \n static int default_task_finished(int result,\n-\t\t\t\t struct child_process *cp,\n \t\t\t\t struct strbuf *err,\n \t\t\t\t void *pp_cb,\n \t\t\t\t void *pp_task_cb)\n {\n-\tint i;\n-\n-\tif (!result)\n-\t\treturn 0;\n-\n-\tstrbuf_addf(err, \"A child failed with return code %d:\", result);\n-\tfor (i = 0; cp->argv[i]; i++)\n-\t\tstrbuf_addf(err, \" %s\", cp->argv[i]);\n-\n \treturn 0;\n }\n \n@@ -1048,8 +1031,7 @@ static int pp_start_one(struct parallel_processes *pp)\n \tpp->children[i].process.no_stdin = 1;\n \n \tif (start_command(&pp->children[i].process)) {\n-\t\tcode = pp->start_failure(&pp->children[i].process,\n-\t\t\t\t\t &pp->children[i].err,\n+\t\tcode = pp->start_failure(&pp->children[i].err,\n \t\t\t\t\t pp->data,\n \t\t\t\t\t &pp->children[i].data);\n \t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n@@ -1117,7 +1099,7 @@ static int pp_collect_finished(struct parallel_processes *pp)\n \n \t\tcode = finish_command(&pp->children[i].process);\n \n-\t\tcode = pp->task_finished(code, &pp->children[i].process,\n+\t\tcode = pp->task_finished(code,\n \t\t\t\t\t &pp->children[i].err, pp->data,\n \t\t\t\t\t &pp->children[i].data);\n \ndiff --git a/run-command.h b/run-command.h\nindex 42917e8..c6a3e42 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -159,8 +159,7 @@ typedef int (*get_next_task_fn)(struct child_process *cp,\n  * To send a signal to other child processes for abortion, return\n  * the negative signal number.\n  */\n-typedef int (*start_failure_fn)(struct child_process *cp,\n-\t\t\t\tstruct strbuf *err,\n+typedef int (*start_failure_fn)(struct strbuf *err,\n \t\t\t\tvoid *pp_cb,\n \t\t\t\tvoid *pp_task_cb);\n \n@@ -179,7 +178,6 @@ typedef int (*start_failure_fn)(struct child_process *cp,\n  * the negative signal number.\n  */\n typedef int (*task_finished_fn)(int result,\n-\t\t\t\tstruct child_process *cp,\n \t\t\t\tstruct strbuf *err,\n \t\t\t\tvoid *pp_cb,\n \t\t\t\tvoid *pp_task_cb);\ndiff --git a/submodule.c b/submodule.c\nindex 24fb81a..62c4356 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -705,8 +705,7 @@ static int get_next_submodule(struct child_process *cp,\n \treturn 0;\n }\n \n-static int fetch_start_failure(struct child_process *cp,\n-\t\t\t       struct strbuf *err,\n+static int fetch_start_failure(struct strbuf *err,\n \t\t\t       void *cb, void *task_cb)\n {\n \tstruct submodule_parallel_fetch *spf = cb;\n@@ -716,8 +715,8 @@ static int fetch_start_failure(struct child_process *cp,\n \treturn 0;\n }\n \n-static int fetch_finish(int retvalue, struct child_process *cp,\n-\t\t\tstruct strbuf *err, void *cb, void *task_cb)\n+static int fetch_finish(int retvalue, struct strbuf *err,\n+\t\t\tvoid *cb, void *task_cb)\n {\n \tstruct submodule_parallel_fetch *spf = cb;\n \ndiff --git a/test-run-command.c b/test-run-command.c\nindex fbe0a27..30a64a9 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -41,7 +41,6 @@ static int no_job(struct child_process *cp,\n }\n \n static int task_finished(int result,\n-\t\t\t struct child_process *cp,\n \t\t\t struct strbuf *err,\n \t\t\t void *pp_cb,\n \t\t\t void *pp_task_cb)\n-- \n2.8.0.rc0.1.g68b4e3f\n"},{"id":"279858","messageId":"CAGZ79kb2h=4qeGbQ+42Zkq5wZ6eVu1rNQn=d1R6Rz4YA-q0xwg@mail.gmail.com","threadId":"41572","inReplyTo":"1456783026-3328-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] run-command: do not pass child process data into callbacks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-29T21:58:41Z","receivedAt":"2016-02-29T21:58:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"It applies on 2.8.0-rc0.\n\n(I tried backporting it to 2.7, but realized it is not an issue there)\n"},{"id":"279883","messageId":"xmqqoaaz845n.fsf@gitster.mtv.corp.google.com","threadId":"41572","inReplyTo":"CAGZ79kb2h=4qeGbQ+42Zkq5wZ6eVu1rNQn=d1R6Rz4YA-q0xwg@mail.gmail.com","subject":"Re: [PATCH] run-command: do not pass child process data into callbacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-29T23:53:24Z","receivedAt":"2016-02-29T23:53:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> It applies on 2.8.0-rc0.\n>\n> (I tried backporting it to 2.7, but realized it is not an issue there)\n\nThanks.  I tried to be nice to rebase the parallel-update stuff on\ntop of this myself, but stopped it as I won't have enough time to\npush the result out for the day anyway.\n\nsubmodule-parallel-update, submodule-init and refs-backend-lmdb\ntopics will be missing from tonight's 'pu'.\n"},{"id":"279940","messageId":"56D54129.4090007@kdbg.org","threadId":"41572","inReplyTo":"1456783026-3328-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] run-command: do not pass child process data into callbacks","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-03-01T07:13:45Z","receivedAt":"2016-03-01T07:13:45Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 29.02.2016 um 22:57 schrieb Stefan Beller:\n> The expected way to pass data into the callback is to pass them via\n> the customizable callback pointer. The error reporting in\n> default_{start_failure, task_finished} is not user friendly enough, that\n> we want to encourage using the child data for such purposes.\n> \n> Furthermore the struct child data is cleaned by the run-command API,\n> before we access them in the callbacks, leading to use-after-free\n> situations.\n\nThanks. The code changes match what I had prototyped. But please squash\nin this documentation change:\n\ndiff --git a/run-command.h b/run-command.h\nindex c6a3e42..3d1e59e 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -191,9 +191,8 @@ typedef int (*task_finished_fn)(int result,\n  * (both stdout and stderr) is routed to stderr in a manner that output\n  * from different tasks does not interleave.\n  *\n- * If start_failure_fn or task_finished_fn are NULL, default handlers\n- * will be used. The default handlers will print an error message on\n- * error without issuing an emergency stop.\n+ * start_failure_fn and task_finished_fn can be NULL to omit any\n+ * special handling.\n  */\n int run_processes_parallel(int n,\n \t\t\t   get_next_task_fn,\n"},{"id":"279984","messageId":"xmqqd1re856r.fsf@gitster.mtv.corp.google.com","threadId":"41572","inReplyTo":"56D54129.4090007@kdbg.org","subject":"Re: [PATCH] run-command: do not pass child process data into callbacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-01T17:43:24Z","receivedAt":"2016-03-01T17:43:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Am 29.02.2016 um 22:57 schrieb Stefan Beller:\n>> The expected way to pass data into the callback is to pass them via\n>> the customizable callback pointer. The error reporting in\n>> default_{start_failure, task_finished} is not user friendly enough, that\n>> we want to encourage using the child data for such purposes.\n>> \n>> Furthermore the struct child data is cleaned by the run-command API,\n>> before we access them in the callbacks, leading to use-after-free\n>> situations.\n>\n> Thanks. The code changes match what I had prototyped. But please squash\n> in this documentation change:\n>\n> diff --git a/run-command.h b/run-command.h\n> index c6a3e42..3d1e59e 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -191,9 +191,8 @@ typedef int (*task_finished_fn)(int result,\n>   * (both stdout and stderr) is routed to stderr in a manner that output\n>   * from different tasks does not interleave.\n>   *\n> - * If start_failure_fn or task_finished_fn are NULL, default handlers\n> - * will be used. The default handlers will print an error message on\n> - * error without issuing an emergency stop.\n> + * start_failure_fn and task_finished_fn can be NULL to omit any\n> + * special handling.\n>   */\n>  int run_processes_parallel(int n,\n>  \t\t\t   get_next_task_fn,\n\nThanks for careful reading.\n"},{"id":"279986","messageId":"1456854926-13838-1-git-send-email-sbeller@google.com","threadId":"41572","inReplyTo":"xmqqd1re856r.fsf@gitster.mtv.corp.google.com","subject":"[PATCHv2] run-command: do not pass child process data into callbacks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-01T17:55:26Z","receivedAt":"2016-03-01T17:55:26Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The expected way to pass data into the callback is to pass them via\nthe customizable callback pointer. The error reporting in\ndefault_{start_failure, task_finished} is not user friendly enough, that\nwe want to encourage using the child data for such purposes.\n\nFurthermore the struct child data is cleaned by the run-command API,\nbefore we access them in the callbacks, leading to use-after-free\nsituations.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n Thanks Johannes for reviewing and double checking. I squashed in your\n proposed documentation change.\n \n This applies on 2.8.0-rc0.\n \n Thanks,\n Stefan\n\n run-command.c      | 24 +++---------------------\n run-command.h      |  9 +++------\n submodule.c        |  7 +++----\n test-run-command.c |  1 -\n 4 files changed, 9 insertions(+), 32 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 51fd72c..8e3ad07 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -902,35 +902,18 @@ struct parallel_processes {\n \tstruct strbuf buffered_output; /* of finished children */\n };\n \n-static int default_start_failure(struct child_process *cp,\n-\t\t\t\t struct strbuf *err,\n+static int default_start_failure(struct strbuf *err,\n \t\t\t\t void *pp_cb,\n \t\t\t\t void *pp_task_cb)\n {\n-\tint i;\n-\n-\tstrbuf_addstr(err, \"Starting a child failed:\");\n-\tfor (i = 0; cp->argv[i]; i++)\n-\t\tstrbuf_addf(err, \" %s\", cp->argv[i]);\n-\n \treturn 0;\n }\n \n static int default_task_finished(int result,\n-\t\t\t\t struct child_process *cp,\n \t\t\t\t struct strbuf *err,\n \t\t\t\t void *pp_cb,\n \t\t\t\t void *pp_task_cb)\n {\n-\tint i;\n-\n-\tif (!result)\n-\t\treturn 0;\n-\n-\tstrbuf_addf(err, \"A child failed with return code %d:\", result);\n-\tfor (i = 0; cp->argv[i]; i++)\n-\t\tstrbuf_addf(err, \" %s\", cp->argv[i]);\n-\n \treturn 0;\n }\n \n@@ -1048,8 +1031,7 @@ static int pp_start_one(struct parallel_processes *pp)\n \tpp->children[i].process.no_stdin = 1;\n \n \tif (start_command(&pp->children[i].process)) {\n-\t\tcode = pp->start_failure(&pp->children[i].process,\n-\t\t\t\t\t &pp->children[i].err,\n+\t\tcode = pp->start_failure(&pp->children[i].err,\n \t\t\t\t\t pp->data,\n \t\t\t\t\t &pp->children[i].data);\n \t\tstrbuf_addbuf(&pp->buffered_output, &pp->children[i].err);\n@@ -1117,7 +1099,7 @@ static int pp_collect_finished(struct parallel_processes *pp)\n \n \t\tcode = finish_command(&pp->children[i].process);\n \n-\t\tcode = pp->task_finished(code, &pp->children[i].process,\n+\t\tcode = pp->task_finished(code,\n \t\t\t\t\t &pp->children[i].err, pp->data,\n \t\t\t\t\t &pp->children[i].data);\n \ndiff --git a/run-command.h b/run-command.h\nindex d5a57f9..6e17894 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -158,8 +158,7 @@ typedef int (*get_next_task_fn)(struct child_process *cp,\n  * To send a signal to other child processes for abortion, return\n  * the negative signal number.\n  */\n-typedef int (*start_failure_fn)(struct child_process *cp,\n-\t\t\t\tstruct strbuf *err,\n+typedef int (*start_failure_fn)(struct strbuf *err,\n \t\t\t\tvoid *pp_cb,\n \t\t\t\tvoid *pp_task_cb);\n \n@@ -178,7 +177,6 @@ typedef int (*start_failure_fn)(struct child_process *cp,\n  * the negative signal number.\n  */\n typedef int (*task_finished_fn)(int result,\n-\t\t\t\tstruct child_process *cp,\n \t\t\t\tstruct strbuf *err,\n \t\t\t\tvoid *pp_cb,\n \t\t\t\tvoid *pp_task_cb);\n@@ -192,9 +190,8 @@ typedef int (*task_finished_fn)(int result,\n  * (both stdout and stderr) is routed to stderr in a manner that output\n  * from different tasks does not interleave.\n  *\n- * If start_failure_fn or task_finished_fn are NULL, default handlers\n- * will be used. The default handlers will print an error message on\n- * error without issuing an emergency stop.\n+ * start_failure_fn and task_finished_fn can be NULL to omit any\n+ * special handling.\n  */\n int run_processes_parallel(int n,\n \t\t\t   get_next_task_fn,\ndiff --git a/submodule.c b/submodule.c\nindex b83939c..916fc84 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -705,8 +705,7 @@ static int get_next_submodule(struct child_process *cp,\n \treturn 0;\n }\n \n-static int fetch_start_failure(struct child_process *cp,\n-\t\t\t       struct strbuf *err,\n+static int fetch_start_failure(struct strbuf *err,\n \t\t\t       void *cb, void *task_cb)\n {\n \tstruct submodule_parallel_fetch *spf = cb;\n@@ -716,8 +715,8 @@ static int fetch_start_failure(struct child_process *cp,\n \treturn 0;\n }\n \n-static int fetch_finish(int retvalue, struct child_process *cp,\n-\t\t\tstruct strbuf *err, void *cb, void *task_cb)\n+static int fetch_finish(int retvalue, struct strbuf *err,\n+\t\t\tvoid *cb, void *task_cb)\n {\n \tstruct submodule_parallel_fetch *spf = cb;\n \ndiff --git a/test-run-command.c b/test-run-command.c\nindex fbe0a27..30a64a9 100644\n--- a/test-run-command.c\n+++ b/test-run-command.c\n@@ -41,7 +41,6 @@ static int no_job(struct child_process *cp,\n }\n \n static int task_finished(int result,\n-\t\t\t struct child_process *cp,\n \t\t\t struct strbuf *err,\n \t\t\t void *pp_cb,\n \t\t\t void *pp_task_cb)\n-- \n2.8.0.rc0.1.g68b4e3f\n"}]}