{"thread":{"id":"45416","subject":"[PATCH] run-command: fix segfault when cleaning forked async process","startedAt":"2017-03-17T23:21:50Z","lastAt":"2017-03-17T23:46:28Z","messageCount":3,"participants":["Jeff King","Jonathan Nieder","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"314417","messageId":"20170317232004.xqs5f2q2swa3b5bq@sigill.intra.peff.net","threadId":"45416","inReplyTo":null,"subject":"[PATCH] run-command: fix segfault when cleaning forked async process","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-17T23:20:04Z","receivedAt":"2017-03-17T23:21:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Callers of the run-command API may mark a child as\n\"clean_on_exit\"; it gets added to a list and killed when the\nmain process dies. Since commit 46df6906f\n(execv_dashed_external: wait for child on signal death,\n2017-01-06), we respect an extra \"wait_after_clean\" flag,\nwhich we expect to find in the child_process struct.\n\nWhen Git is built with NO_PTHREADS, we start \"struct\nasync\" processes by forking rather than spawning a thread.\nThe resulting processes get added to the cleanup list but\nthey don't have a child_process struct, and the cleanup\nfunction ends up dereferencing NULL.\n\nWe should notice this case and assume that the processes do\nnot need to be waited for (i.e., the same behavior they had\nbefore 46df6906f).\n\nReported-by: Brandon Williams \nSigned-off-by: Jeff King \n---\nThis is a regression in v2.12.0, though there is no hurry to get it into\nv2.12.1 unless your grep patches go in, too. Without them you can't\nactually build with NO_PTHREADS anyway.\n\nHowever, applied directly on top of 46df6906f (which predates the build\nbreakage), you can easily see the test failures with NO_PTHREADS and\nthat this fixes them.\n\n run-command.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 5227f78ae..574b81d3e 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -48,7 +48,7 @@ static void cleanup_children(int sig, int in_signal)\n \n \t\tkill(p->pid, sig);\n \n-\t\tif (p->process->wait_after_clean) {\n+\t\tif (p->process && p->process->wait_after_clean) {\n \t\t\tp->next = children_to_wait_for;\n \t\t\tchildren_to_wait_for = p;\n \t\t} else {\n-- \n2.12.0.660.gf842b44fd\n"},{"id":"314423","messageId":"20170317234215.GT26789@aiede.mtv.corp.google.com","threadId":"45416","inReplyTo":"20170317232004.xqs5f2q2swa3b5bq@sigill.intra.peff.net","subject":"Re: [PATCH] run-command: fix segfault when cleaning forked async process","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-17T23:42:15Z","receivedAt":"2017-03-17T23:43:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Reported-by: Brandon Williams \n> Signed-off-by: Jeff King \n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"314425","messageId":"20170317233806.GG63813@google.com","threadId":"45416","inReplyTo":"20170317232004.xqs5f2q2swa3b5bq@sigill.intra.peff.net","subject":"Re: [PATCH] run-command: fix segfault when cleaning forked async process","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T23:38:06Z","receivedAt":"2017-03-17T23:46:28Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/17, Jeff King wrote:\n> Callers of the run-command API may mark a child as\n> \"clean_on_exit\"; it gets added to a list and killed when the\n> main process dies. Since commit 46df6906f\n> (execv_dashed_external: wait for child on signal death,\n> 2017-01-06), we respect an extra \"wait_after_clean\" flag,\n> which we expect to find in the child_process struct.\n> \n> When Git is built with NO_PTHREADS, we start \"struct\n> async\" processes by forking rather than spawning a thread.\n> The resulting processes get added to the cleanup list but\n> they don't have a child_process struct, and the cleanup\n> function ends up dereferencing NULL.\n> \n> We should notice this case and assume that the processes do\n> not need to be waited for (i.e., the same behavior they had\n> before 46df6906f).\n> \n> Reported-by: Brandon Williams \n> Signed-off-by: Jeff King \n> ---\n> This is a regression in v2.12.0, though there is no hurry to get it into\n> v2.12.1 unless your grep patches go in, too. Without them you can't\n> actually build with NO_PTHREADS anyway.\n> \n> However, applied directly on top of 46df6906f (which predates the build\n> breakage), you can easily see the test failures with NO_PTHREADS and\n> that this fixes them.\n> \n> run-command.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/run-command.c b/run-command.c\n> index 5227f78ae..574b81d3e 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -48,7 +48,7 @@ static void cleanup_children(int sig, int in_signal)\n> \n> \t\tkill(p->pid, sig);\n> \n> -\t\tif (p->process->wait_after_clean) {\n> +\t\tif (p->process && p->process->wait_after_clean) {\n> \t\t\tp->next = children_to_wait_for;\n> \t\t\tchildren_to_wait_for = p;\n> \t\t} else {\n\nLooks good to me! Thanks for tracking that down so quickly. I'm glad it\nwas a quick fix.\n\n-- \nBrandon Williams\n"}]}