{"thread":{"id":"56946","subject":"[PATCH] pager: fix crash when pager program doesn't exist","startedAt":"2021-11-19T23:48:08Z","lastAt":"2021-11-20T03:42:47Z","messageCount":6,"participants":["Enzo Matsumiya","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"441822","messageId":"20211119234745.26605-1-ematsumiya@suse.de","threadId":"56946","inReplyTo":null,"subject":"[PATCH] pager: fix crash when pager program doesn't exist","fromName":"Enzo Matsumiya","fromEmail":"ematsumiya@suse.de","sentAt":"2021-11-19T23:47:45Z","receivedAt":"2021-11-19T23:48:08Z","isPatch":true,"sender":{"key":"ematsumiya@suse.de","avatar":"https://avatars.githubusercontent.com/u/45605263?v=4"},"body":"setup_pager() doesn't properly free pager_process.argv if\nstart_command() fails, nor finish_command() is ever called.\n\nsetup_pager() is called twice, once from commit_pager_choice(), and then\nfrom cmd_log_init_finish(). On the first run, it runs fine because\nstart_command() assigns cmd->args.v to cmd->argv, and upon command\nfailure, child_process_clear() clears cmd->args.\n\nOn the second run, though, argv is no longer NULL, but .args has been\ncleared, so any strvec_push() operation will crash.\n\nThis patch makes sure that freeing pager_process.argv is part of its\nargument preparation, as well as zeroing the whole pager_process struct.\n\nReproduction:\n$ git config pager.show INVALID_PAGER\n$ git show $VALID_COMMIT\nerror: cannot run INVALID_PAGER: No such file or directory\n[1]    3619 segmentation fault (core dumped)  git show $VALID_COMMIT\n\nWhile at it, I implemented a fallback to the DEFAULT_PAGER, so it tries\nat least one more time when the configured pager fails.\n\nSigned-off-by: Enzo Matsumiya <ematsumiya@suse.de>\n---\n pager.c       | 25 +++++++++++++++++++++----\n run-command.c |  1 +\n 2 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 52f27a6765c8..79a47db55d63 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -97,6 +97,9 @@ static void setup_pager_env(struct strvec *env)\n \n void prepare_pager_args(struct child_process *pager_process, const char *pager)\n {\n+\tchild_process_clear(pager_process);\n+\tif (pager_process->argv)\n+\t\tfree(pager_process->argv);\n \tstrvec_push(&pager_process->args, pager);\n \tpager_process->use_shell = 1;\n \tsetup_pager_env(&pager_process->env_array);\n@@ -105,11 +108,14 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)\n \n void setup_pager(void)\n {\n-\tconst char *pager = git_pager(isatty(1));\n+\tconst char *tmp_pager = git_pager(isatty(1));\n+\tchar *pager;\n \n-\tif (!pager)\n+\tif (!tmp_pager)\n \t\treturn;\n \n+\tpager = xstrdup(tmp_pager);\n+\n \t/*\n \t * After we redirect standard output, we won't be able to use an ioctl\n \t * to get the terminal size. Let's grab it now, and then set $COLUMNS\n@@ -124,12 +130,23 @@ void setup_pager(void)\n \n \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n \n+retry:\n \t/* spawn the pager */\n \tprepare_pager_args(&pager_process, pager);\n \tpager_process.in = -1;\n \tstrvec_push(&pager_process.env_array, \"GIT_PAGER_IN_USE\");\n-\tif (start_command(&pager_process))\n-\t\treturn;\n+\tif (start_command(&pager_process)) {\n+\t\t/* chosen pager failed, try again with the default pager */\n+\t\tif (strcmp(pager, DEFAULT_PAGER)) {\n+\t\t\tfree(pager);\n+\t\t\tpager = xstrdup(DEFAULT_PAGER);\n+\t\t\tgoto retry;\n+\t\t} else {\n+\t\t\t/* default pager failed, let caller handle it */\n+\t\t\tfree(pager);\n+\t\t\treturn;\n+\t\t}\n+\t}\n \n \t/* original process continues, but writes to the pipe */\n \tdup2(pager_process.in, 1);\ndiff --git a/run-command.c b/run-command.c\nindex f329391154ae..d2b7647afdd8 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -19,6 +19,7 @@ void child_process_clear(struct child_process *child)\n {\n \tstrvec_clear(&child->args);\n \tstrvec_clear(&child->env_array);\n+\tmemset(child, 0, sizeof(*child));\n }\n \n struct child_to_clean {\n-- \n2.33.1\n\n"},{"id":"441825","messageId":"YZhVA8DOjHu90gzs@coredump.intra.peff.net","threadId":"56946","inReplyTo":"20211119234745.26605-1-ematsumiya@suse.de","subject":"Re: [PATCH] pager: fix crash when pager program doesn't exist","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-20T01:53:07Z","receivedAt":"2021-11-20T01:53:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 19, 2021 at 08:47:45PM -0300, Enzo Matsumiya wrote:\n\n> setup_pager() doesn't properly free pager_process.argv if\n> start_command() fails, nor finish_command() is ever called.\n\nHmm. It shouldn't need to. It sets up the args using\nprepare_pager_args(), which pushes onto the pager_process->args strvec.\nIf start_command fails, then it takes care of freeing that strvec.\n\nIn fact, it would be wrong to free that pointer, because it is usually\njust pointing to the strvec's \"v\" array, which will already be freed by\nstrvec_clear().\n\n> setup_pager() is called twice, once from commit_pager_choice(), and then\n> from cmd_log_init_finish(). On the first run, it runs fine because\n> start_command() assigns cmd->args.v to cmd->argv, and upon command\n> failure, child_process_clear() clears cmd->args.\n\nWhen pager setup succeeds, the second run is a noop, because isatty(1)\nis no longer true. But for the case you're interested in, the first one\nfails, so we do try again. And I can reproduce your problem with:\n\n GIT_PAGER=no-such-command git -p log\n\nI had to run it with ASan to trigger a failure, as use-after-free bugs\naren't always deterministic.\n\n> On the second run, though, argv is no longer NULL, but .args has been\n> cleared, so any strvec_push() operation will crash.\n\nRight. So we want to make sure that argv is NULL, but we don't need to\nfree it. I think the bug is not in failing to clean up, though. It's in\nassuming that the child_process has been properly initialized. The first\ncall works because of the initialization in the declaration, but the\nsecond call can't rely on that.\n\nSo one solution is more like this:\n\ndiff --git a/pager.c b/pager.c\nindex 52f27a6765..27877f8ebb 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -8,7 +8,7 @@\n #define DEFAULT_PAGER \"less\"\n #endif\n \n-static struct child_process pager_process = CHILD_PROCESS_INIT;\n+static struct child_process pager_process;\n static const char *pager_program;\n \n /* Is the value coming back from term_columns() just a guess? */\n@@ -124,6 +124,8 @@ void setup_pager(void)\n \n \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n \n+\tchild_process_init(&pager_process);\n+\n \t/* spawn the pager */\n \tprepare_pager_args(&pager_process, pager);\n \tpager_process.in = -1;\n\nwhich is enough to fix the use-after-free.\n\n> While at it, I implemented a fallback to the DEFAULT_PAGER, so it tries\n> at least one more time when the configured pager fails.\n\nI think this is probably a bad idea. If the user told us to use a\ndifferent pager, then we should not unexpectedly use the default one. At\nany rate, it's unrelated to this patch; if we were to do it, it should\nbe separate from the bugfix.\n\n> diff --git a/pager.c b/pager.c\n> index 52f27a6765c8..79a47db55d63 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -97,6 +97,9 @@ static void setup_pager_env(struct strvec *env)\n>  \n>  void prepare_pager_args(struct child_process *pager_process, const char *pager)\n>  {\n> +\tchild_process_clear(pager_process);\n> +\tif (pager_process->argv)\n> +\t\tfree(pager_process->argv);\n\nAs noted above, this would be double-freeing the strvec's array. Except\nthat your free() here never runs, because you added a memset() to\nchild_process_clear() which will always set pager_process->argv to NULL.\n\n> @@ -105,11 +108,14 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)\n>  \n>  void setup_pager(void)\n>  {\n> -\tconst char *pager = git_pager(isatty(1));\n> +\tconst char *tmp_pager = git_pager(isatty(1));\n> +\tchar *pager;\n>  \n> -\tif (!pager)\n> +\tif (!tmp_pager)\n>  \t\treturn;\n>  \n> +\tpager = xstrdup(tmp_pager);\n> +\n\nWhy are we making a copy of the pager string? When it's pushed into the\nstrvec, that will take ownership of it. Your patch creates a leak (in\nthe case that we start_command() doesn't fail, and we miss the new\nfree() calls you added).\n\n> diff --git a/run-command.c b/run-command.c\n> index f329391154ae..d2b7647afdd8 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -19,6 +19,7 @@ void child_process_clear(struct child_process *child)\n>  {\n>  \tstrvec_clear(&child->args);\n>  \tstrvec_clear(&child->env_array);\n> +\tmemset(child, 0, sizeof(*child));\n>  }\n\nThis doesn't set up a child_process to be correctly reused, as the\ninitialized state for a strvec is not all-bits-zero (though it often\nworks in practice).\n\nThe patch I showed above fixes the use-after-free by making sure the\nchild_process struct is initialized before we use it. But we could also\nchange the semantics of _clear() to make sure it is ready for reuse. To\ndo that, you'd want to just call child_process_init() here.\n\nThere's some precedent in that with other APIs (e.g., strvec_clear()\nmakes the struct ready for reuse immediately). Plus it's harder for the\ncallers to get wrong (after adding such a line, setup_pager() does not\nhave to do anything else at all).\n\nSo this single-line change also fixes the use-after-free, without any of\nthe other stuff:\n\ndiff --git a/run-command.c b/run-command.c\nindex f40df01c77..92e00d9455 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -21,6 +21,7 @@ void child_process_clear(struct child_process *child)\n {\n \tstrvec_clear(&child->args);\n \tstrvec_clear(&child->env_array);\n+\tchild_process_init(child);\n }\n \n struct child_to_clean {\n\n-Peff\n"},{"id":"441826","messageId":"20211120023246.7ynehp5v3iypfr6w@cyberdelia","threadId":"56946","inReplyTo":"YZhVA8DOjHu90gzs@coredump.intra.peff.net","subject":"Re: [PATCH] pager: fix crash when pager program doesn't exist","fromName":"Enzo Matsumiya","fromEmail":"ematsumiya@suse.de","sentAt":"2021-11-20T02:32:46Z","receivedAt":"2021-11-20T02:32:53Z","isPatch":true,"sender":{"key":"ematsumiya@suse.de","avatar":"https://avatars.githubusercontent.com/u/45605263?v=4"},"body":"Hi,\n\nThanks for the detailed reply. Please see my replies below.\n\nOn 11/19, Jeff King wrote:\n>On Fri, Nov 19, 2021 at 08:47:45PM -0300, Enzo Matsumiya wrote:\n>\n>> setup_pager() doesn't properly free pager_process.argv if\n>> start_command() fails, nor finish_command() is ever called.\n>\n>Hmm. It shouldn't need to. It sets up the args using\n>prepare_pager_args(), which pushes onto the pager_process->args strvec.\n>If start_command fails, then it takes care of freeing that strvec.\n>\n>In fact, it would be wrong to free that pointer, because it is usually\n>just pointing to the strvec's \"v\" array, which will already be freed by\n>strvec_clear().\n\nAgreed, the memory is freed, but argv still points to the old args.v\nlocation.\nYou can easily check this by adding a breakpoint at start_command and\nrun my reproducer.\n\nOn the first run, argv is NULL, then assigned args.v location.\nOn the second run, argv points to the old args.v, and args.v has a new\nlocation. That's when strvec_push() will try to use whatever garbage is\nat argv.\n\n>> setup_pager() is called twice, once from commit_pager_choice(), and then\n>> from cmd_log_init_finish(). On the first run, it runs fine because\n>> start_command() assigns cmd->args.v to cmd->argv, and upon command\n>> failure, child_process_clear() clears cmd->args.\n>\n>When pager setup succeeds, the second run is a noop, because isatty(1)\n>is no longer true. But for the case you're interested in, the first one\n>fails, so we do try again. And I can reproduce your problem with:\n\nNo, it's not a noop such as that it's clear that things are different on\nthe second call.\n\n> GIT_PAGER=no-such-command git -p log\n>\n>I had to run it with ASan to trigger a failure, as use-after-free bugs\n>aren't always deterministic.\n\nPlease use my reproducer as it's 100% reliable and consistent (same\nmemory regions are affected).\n\nI couldn't reproduce the issue with yours.\n\n>> On the second run, though, argv is no longer NULL, but .args has been\n>> cleared, so any strvec_push() operation will crash.\n>\n>Right. So we want to make sure that argv is NULL, but we don't need to\n>free it. I think the bug is not in failing to clean up, though. It's in\n>assuming that the child_process has been properly initialized. The first\n>call works because of the initialization in the declaration, but the\n>second call can't rely on that.\n>\n>So one solution is more like this:\n>\n>diff --git a/pager.c b/pager.c\n>index 52f27a6765..27877f8ebb 100644\n>--- a/pager.c\n>+++ b/pager.c\n>@@ -8,7 +8,7 @@\n> #define DEFAULT_PAGER \"less\"\n> #endif\n>\n>-static struct child_process pager_process = CHILD_PROCESS_INIT;\n>+static struct child_process pager_process;\n> static const char *pager_program;\n>\n> /* Is the value coming back from term_columns() just a guess? */\n>@@ -124,6 +124,8 @@ void setup_pager(void)\n>\n> \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n>\n>+\tchild_process_init(&pager_process);\n>+\n> \t/* spawn the pager */\n> \tprepare_pager_args(&pager_process, pager);\n> \tpager_process.in = -1;\n>\n>which is enough to fix the use-after-free.\n\nThis worked for my case.\n\n>> While at it, I implemented a fallback to the DEFAULT_PAGER, so it tries\n>> at least one more time when the configured pager fails.\n>\n>I think this is probably a bad idea. If the user told us to use a\n>different pager, then we should not unexpectedly use the default one. At\n>any rate, it's unrelated to this patch; if we were to do it, it should\n>be separate from the bugfix.\n\nGot it. I was a bit skeptical about this one. Idea discarded.\n\n>> diff --git a/pager.c b/pager.c\n>> index 52f27a6765c8..79a47db55d63 100644\n>> --- a/pager.c\n>> +++ b/pager.c\n>> @@ -97,6 +97,9 @@ static void setup_pager_env(struct strvec *env)\n>>\n>>  void prepare_pager_args(struct child_process *pager_process, const char *pager)\n>>  {\n>> +\tchild_process_clear(pager_process);\n>> +\tif (pager_process->argv)\n>> +\t\tfree(pager_process->argv);\n>\n>As noted above, this would be double-freeing the strvec's array. Except\n>that your free() here never runs, because you added a memset() to\n>child_process_clear() which will always set pager_process->argv to NULL.\n\nOk.\n\n>> @@ -105,11 +108,14 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)\n>>\n>>  void setup_pager(void)\n>>  {\n>> -\tconst char *pager = git_pager(isatty(1));\n>> +\tconst char *tmp_pager = git_pager(isatty(1));\n>> +\tchar *pager;\n>>\n>> -\tif (!pager)\n>> +\tif (!tmp_pager)\n>>  \t\treturn;\n>>\n>> +\tpager = xstrdup(tmp_pager);\n>> +\n>\n>Why are we making a copy of the pager string? When it's pushed into the\n>strvec, that will take ownership of it. Your patch creates a leak (in\n>the case that we start_command() doesn't fail, and we miss the new\n>free() calls you added).\n\nRight, thanks.\n\n>> diff --git a/run-command.c b/run-command.c\n>> index f329391154ae..d2b7647afdd8 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -19,6 +19,7 @@ void child_process_clear(struct child_process *child)\n>>  {\n>>  \tstrvec_clear(&child->args);\n>>  \tstrvec_clear(&child->env_array);\n>> +\tmemset(child, 0, sizeof(*child));\n>>  }\n>\n>This doesn't set up a child_process to be correctly reused, as the\n>initialized state for a strvec is not all-bits-zero (though it often\n>works in practice).\n\nNoted, thanks.\n\n>The patch I showed above fixes the use-after-free by making sure the\n>child_process struct is initialized before we use it. But we could also\n>change the semantics of _clear() to make sure it is ready for reuse. To\n>do that, you'd want to just call child_process_init() here.\n>\n>There's some precedent in that with other APIs (e.g., strvec_clear()\n>makes the struct ready for reuse immediately). Plus it's harder for the\n>callers to get wrong (after adding such a line, setup_pager() does not\n>have to do anything else at all).\n>\n>So this single-line change also fixes the use-after-free, without any of\n>the other stuff:\n>\n>diff --git a/run-command.c b/run-command.c\n>index f40df01c77..92e00d9455 100644\n>--- a/run-command.c\n>+++ b/run-command.c\n>@@ -21,6 +21,7 @@ void child_process_clear(struct child_process *child)\n> {\n> \tstrvec_clear(&child->args);\n> \tstrvec_clear(&child->env_array);\n>+\tchild_process_init(child);\n> }\n>\n> struct child_to_clean {\n\nOf course this one works as well. And is more elegant IMHO.\n\nAgain, thanks a lot for the feedback.\n\nShould I submit a v2 or will you?\n\n\n>-Peff\n\nCheers,\n\nEnzo\n"},{"id":"441827","messageId":"20211120030621.2xbcz5usaqkh2ald@cyberdelia","threadId":"56946","inReplyTo":"20211120023246.7ynehp5v3iypfr6w@cyberdelia","subject":"Re: [PATCH] pager: fix crash when pager program doesn't exist","fromName":"Enzo Matsumiya","fromEmail":"ematsumiya@suse.de","sentAt":"2021-11-20T03:06:21Z","receivedAt":"2021-11-20T03:06:29Z","isPatch":true,"sender":{"key":"ematsumiya@suse.de","avatar":"https://avatars.githubusercontent.com/u/45605263?v=4"},"body":"On 11/19, Enzo Matsumiya wrote:\n>>When pager setup succeeds, the second run is a noop, because isatty(1)\n>>is no longer true. But for the case you're interested in, the first one\n>>fails, so we do try again. And I can reproduce your problem with:\n>\n>No, it's not a noop such as that it's clear that things are different on\n>the second call.\n\nHere I meant that setup_pager() is effectivelly called from 2 different places:\n\nFirst backtrace:\nsetup_pager()\ncommit_pager_choice()\nrun_builtin()\nhandle_builtin()\nrun_argv()\ncmd_main()\n\nSecond backtrace:\nsetup_pager()\ncmd_log_init_finish()\ncmd_log_init()\ncmd_show()\nrun_builtin()\nhandle_builtin()\nrun_argv()\ncmd_main()\n\nAlso, isatty(1) is not false in neither of the calls. Otherwise I\nwouldn't hit this bug (pager would be NULL and setup_pager() a noop as\nyou said).\n\n\nCheers,\n\nEnzo\n"},{"id":"441836","messageId":"YZhtplvcVkY0Yzmt@coredump.intra.peff.net","threadId":"56946","inReplyTo":"20211120030621.2xbcz5usaqkh2ald@cyberdelia","subject":"Re: [PATCH] pager: fix crash when pager program doesn't exist","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-20T03:38:14Z","receivedAt":"2021-11-20T03:38:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 20, 2021 at 12:06:21AM -0300, Enzo Matsumiya wrote:\n\n> On 11/19, Enzo Matsumiya wrote:\n> > > When pager setup succeeds, the second run is a noop, because isatty(1)\n> > > is no longer true. But for the case you're interested in, the first one\n> > > fails, so we do try again. And I can reproduce your problem with:\n> > \n> > No, it's not a noop such as that it's clear that things are different on\n> > the second call.\n> \n> Here I meant that setup_pager() is effectivelly called from 2 different places:\n> \n> First backtrace:\n> setup_pager()\n> commit_pager_choice()\n> run_builtin()\n> handle_builtin()\n> run_argv()\n> cmd_main()\n> \n> Second backtrace:\n> setup_pager()\n> cmd_log_init_finish()\n> cmd_log_init()\n> cmd_show()\n> run_builtin()\n> handle_builtin()\n> run_argv()\n> cmd_main()\n> \n> Also, isatty(1) is not false in neither of the calls. Otherwise I\n> wouldn't hit this bug (pager would be NULL and setup_pager() a noop as\n> you said).\n\nRight, I mean in the \"normal\" case that the pager actually starts, the\nsecond call hits isatty(1), then git_pager() returns NULL, and we return\nfrom setup_pager() immediately.\n\nIt is only in the broken-pager case that the bug you found is triggered\n(which is probably why nobody has really noticed it).\n\n-Peff\n"},{"id":"441837","messageId":"YZhurgbXq7vhDTXC@coredump.intra.peff.net","threadId":"56946","inReplyTo":"20211120023246.7ynehp5v3iypfr6w@cyberdelia","subject":"Re: [PATCH] pager: fix crash when pager program doesn't exist","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-20T03:42:38Z","receivedAt":"2021-11-20T03:42:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 19, 2021 at 11:32:46PM -0300, Enzo Matsumiya wrote:\n\n> > GIT_PAGER=no-such-command git -p log\n> > \n> > I had to run it with ASan to trigger a failure, as use-after-free bugs\n> > aren't always deterministic.\n> \n> Please use my reproducer as it's 100% reliable and consistent (same\n> memory regions are affected).\n> \n> I couldn't reproduce the issue with yours.\n\nOur reproducers are triggering the same behavior. But it won't be 100%\nreliable in the sense that the behavior is undefined. Depending upon\nrandom details of the allocator, we may get a segfault, or see random\ntrash on the heap, or even see the old data. That's why I suggested\nusing ASan; it poisons the freed memory to reliably detect problems.\n\nBut at any rate, yes, it's clear that there is a bug here.\n\n> > diff --git a/run-command.c b/run-command.c\n> > index f40df01c77..92e00d9455 100644\n> > --- a/run-command.c\n> > +++ b/run-command.c\n> > @@ -21,6 +21,7 @@ void child_process_clear(struct child_process *child)\n> > {\n> > \tstrvec_clear(&child->args);\n> > \tstrvec_clear(&child->env_array);\n> > +\tchild_process_init(child);\n> > }\n> > \n> > struct child_to_clean {\n> \n> Of course this one works as well. And is more elegant IMHO.\n\nYeah, I think so, too.\n\n> Should I submit a v2 or will you?\n\nWhy don't you put together a v2, and I can review it.\n\n-Peff\n"}]}