* [PATCH] run-command: do not pass child process data into callbacks
@ 2016-02-29 21:57 Stefan Beller
2016-02-29 21:58 ` Stefan Beller
2016-03-01 7:13 ` Johannes Sixt
0 siblings, 2 replies; 6+ messages in thread
From: Stefan Beller @ 2016-02-29 21:57 UTC (permalink / raw)
To: gitster; +Cc: git, j6t, 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.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] run-command: do not pass child process data into callbacks
2016-02-29 21:57 [PATCH] run-command: do not pass child process data into callbacks Stefan Beller
@ 2016-02-29 21:58 ` Stefan Beller
2016-02-29 23:53 ` Junio C Hamano
2016-03-01 7:13 ` Johannes Sixt
1 sibling, 1 reply; 6+ messages in thread
From: Stefan Beller @ 2016-02-29 21:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Johannes Sixt, Stefan Beller
It applies on 2.8.0-rc0.
(I tried backporting it to 2.7, but realized it is not an issue there)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] run-command: do not pass child process data into callbacks
2016-02-29 21:58 ` Stefan Beller
@ 2016-02-29 23:53 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2016-02-29 23:53 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Johannes Sixt
Stefan Beller <sbeller@google.com> 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'.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] run-command: do not pass child process data into callbacks
2016-02-29 21:57 [PATCH] run-command: do not pass child process data into callbacks Stefan Beller
2016-02-29 21:58 ` Stefan Beller
@ 2016-03-01 7:13 ` Johannes Sixt
2016-03-01 17:43 ` Junio C Hamano
1 sibling, 1 reply; 6+ messages in thread
From: Johannes Sixt @ 2016-03-01 7:13 UTC (permalink / raw)
To: Stefan Beller, gitster; +Cc: git
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,
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] run-command: do not pass child process data into callbacks
2016-03-01 7:13 ` Johannes Sixt
@ 2016-03-01 17:43 ` Junio C Hamano
2016-03-01 17:55 ` [PATCHv2] " Stefan Beller
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2016-03-01 17:43 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Stefan Beller, git
Johannes Sixt <j6t@kdbg.org> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCHv2] run-command: do not pass child process data into callbacks
2016-03-01 17:43 ` Junio C Hamano
@ 2016-03-01 17:55 ` Stefan Beller
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Beller @ 2016-03-01 17:55 UTC (permalink / raw)
To: j6t, git, gitster; +Cc: 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.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-03-01 17:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-29 21:57 [PATCH] run-command: do not pass child process data into callbacks Stefan Beller
2016-02-29 21:58 ` Stefan Beller
2016-02-29 23:53 ` Junio C Hamano
2016-03-01 7:13 ` Johannes Sixt
2016-03-01 17:43 ` Junio C Hamano
2016-03-01 17:55 ` [PATCHv2] " Stefan Beller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).