[PATCH] pager: fix crash when pager program doesn't exist

[PATCH] pager: fix crash when pager program doesn't exist

[Enzo Matsumiya]

setup_pager() doesn't properly free pager_process.argv if
start_command() fails, nor finish_command() is ever called.

setup_pager() is called twice, once from commit_pager_choice(), and then
from cmd_log_init_finish(). On the first run, it runs fine because
start_command() assigns cmd->args.v to cmd->argv, and upon command
failure, child_process_clear() clears cmd->args.

On the second run, though, argv is no longer NULL, but .args has been
cleared, so any strvec_push() operation will crash.

This patch makes sure that freeing pager_process.argv is part of its
argument preparation, as well as zeroing the whole pager_process struct.

Reproduction:
$ git config pager.show INVALID_PAGER
$ git show $VALID_COMMIT
error: cannot run INVALID_PAGER: No such file or directory
[1]    3619 segmentation fault (core dumped)  git show $VALID_COMMIT

While at it, I implemented a fallback to the DEFAULT_PAGER, so it tries
at least one more time when the configured pager fails.

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
 pager.c       | 25 +++++++++++++++++++++----
 run-command.c |  1 +
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/pager.c b/pager.c
index 52f27a6765c8..79a47db55d63 100644
--- a/pager.c
+++ b/pager.c
@@ -97,6 +97,9 @@ static void setup_pager_env(struct strvec *env)
 
 void prepare_pager_args(struct child_process *pager_process, const char *pager)
 {
+	child_process_clear(pager_process);
+	if (pager_process->argv)
+		free(pager_process->argv);
 	strvec_push(&pager_process->args, pager);
 	pager_process->use_shell = 1;
 	setup_pager_env(&pager_process->env_array);
@@ -105,11 +108,14 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
 
 void setup_pager(void)
 {
-	const char *pager = git_pager(isatty(1));
+	const char *tmp_pager = git_pager(isatty(1));
+	char *pager;
 
-	if (!pager)
+	if (!tmp_pager)
 		return;
 
+	pager = xstrdup(tmp_pager);
+
 	/*
 	 * After we redirect standard output, we won't be able to use an ioctl
 	 * to get the terminal size. Let's grab it now, and then set $COLUMNS
@@ -124,12 +130,23 @@ void setup_pager(void)
 
 	setenv("GIT_PAGER_IN_USE", "true", 1);
 
+retry:
 	/* spawn the pager */
 	prepare_pager_args(&pager_process, pager);
 	pager_process.in = -1;
 	strvec_push(&pager_process.env_array, "GIT_PAGER_IN_USE");
-	if (start_command(&pager_process))
-		return;
+	if (start_command(&pager_process)) {
+		/* chosen pager failed, try again with the default pager */
+		if (strcmp(pager, DEFAULT_PAGER)) {
+			free(pager);
+			pager = xstrdup(DEFAULT_PAGER);
+			goto retry;
+		} else {
+			/* default pager failed, let caller handle it */
+			free(pager);
+			return;
+		}
+	}
 
 	/* original process continues, but writes to the pipe */
 	dup2(pager_process.in, 1);
diff --git a/run-command.c b/run-command.c
index f329391154ae..d2b7647afdd8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -19,6 +19,7 @@ void child_process_clear(struct child_process *child)
 {
 	strvec_clear(&child->args);
 	strvec_clear(&child->env_array);
+	memset(child, 0, sizeof(*child));
 }
 
 struct child_to_clean {
-- 
2.33.1

Re: [PATCH] pager: fix crash when pager program doesn't exist

[Jeff King]

On Fri, Nov 19, 2021 at 08:47:45PM -0300, Enzo Matsumiya wrote:

> setup_pager() doesn't properly free pager_process.argv if
> start_command() fails, nor finish_command() is ever called.

Hmm. It shouldn't need to. It sets up the args using
prepare_pager_args(), which pushes onto the pager_process->args strvec.
If start_command fails, then it takes care of freeing that strvec.

In fact, it would be wrong to free that pointer, because it is usually
just pointing to the strvec's "v" array, which will already be freed by
strvec_clear().

> setup_pager() is called twice, once from commit_pager_choice(), and then
> from cmd_log_init_finish(). On the first run, it runs fine because
> start_command() assigns cmd->args.v to cmd->argv, and upon command
> failure, child_process_clear() clears cmd->args.

When pager setup succeeds, the second run is a noop, because isatty(1)
is no longer true. But for the case you're interested in, the first one
fails, so we do try again. And I can reproduce your problem with:

 GIT_PAGER=no-such-command git -p log

I had to run it with ASan to trigger a failure, as use-after-free bugs
aren't always deterministic.

> On the second run, though, argv is no longer NULL, but .args has been
> cleared, so any strvec_push() operation will crash.

Right. So we want to make sure that argv is NULL, but we don't need to
free it. I think the bug is not in failing to clean up, though. It's in
assuming that the child_process has been properly initialized. The first
call works because of the initialization in the declaration, but the
second call can't rely on that.

So one solution is more like this:

diff --git a/pager.c b/pager.c
index 52f27a6765..27877f8ebb 100644
--- a/pager.c
+++ b/pager.c
@@ -8,7 +8,7 @@
 #define DEFAULT_PAGER "less"
 #endif
 
-static struct child_process pager_process = CHILD_PROCESS_INIT;
+static struct child_process pager_process;
 static const char *pager_program;
 
 /* Is the value coming back from term_columns() just a guess? */
@@ -124,6 +124,8 @@ void setup_pager(void)
 
 	setenv("GIT_PAGER_IN_USE", "true", 1);
 
+	child_process_init(&pager_process);
+
 	/* spawn the pager */
 	prepare_pager_args(&pager_process, pager);
 	pager_process.in = -1;

which is enough to fix the use-after-free.

> While at it, I implemented a fallback to the DEFAULT_PAGER, so it tries
> at least one more time when the configured pager fails.

I think this is probably a bad idea. If the user told us to use a
different pager, then we should not unexpectedly use the default one. At
any rate, it's unrelated to this patch; if we were to do it, it should
be separate from the bugfix.

> diff --git a/pager.c b/pager.c
> index 52f27a6765c8..79a47db55d63 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -97,6 +97,9 @@ static void setup_pager_env(struct strvec *env)
>  
>  void prepare_pager_args(struct child_process *pager_process, const char *pager)
>  {
> +	child_process_clear(pager_process);
> +	if (pager_process->argv)
> +		free(pager_process->argv);

As noted above, this would be double-freeing the strvec's array. Except
that your free() here never runs, because you added a memset() to
child_process_clear() which will always set pager_process->argv to NULL.

> @@ -105,11 +108,14 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
>  
>  void setup_pager(void)
>  {
> -	const char *pager = git_pager(isatty(1));
> +	const char *tmp_pager = git_pager(isatty(1));
> +	char *pager;
>  
> -	if (!pager)
> +	if (!tmp_pager)
>  		return;
>  
> +	pager = xstrdup(tmp_pager);
> +

Why are we making a copy of the pager string? When it's pushed into the
strvec, that will take ownership of it. Your patch creates a leak (in
the case that we start_command() doesn't fail, and we miss the new
free() calls you added).

> diff --git a/run-command.c b/run-command.c
> index f329391154ae..d2b7647afdd8 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -19,6 +19,7 @@ void child_process_clear(struct child_process *child)
>  {
>  	strvec_clear(&child->args);
>  	strvec_clear(&child->env_array);
> +	memset(child, 0, sizeof(*child));
>  }

This doesn't set up a child_process to be correctly reused, as the
initialized state for a strvec is not all-bits-zero (though it often
works in practice).

The patch I showed above fixes the use-after-free by making sure the
child_process struct is initialized before we use it. But we could also
change the semantics of _clear() to make sure it is ready for reuse. To
do that, you'd want to just call child_process_init() here.

There's some precedent in that with other APIs (e.g., strvec_clear()
makes the struct ready for reuse immediately). Plus it's harder for the
callers to get wrong (after adding such a line, setup_pager() does not
have to do anything else at all).

So this single-line change also fixes the use-after-free, without any of
the other stuff:

diff --git a/run-command.c b/run-command.c
index f40df01c77..92e00d9455 100644
--- a/run-command.c
+++ b/run-command.c
@@ -21,6 +21,7 @@ void child_process_clear(struct child_process *child)
 {
 	strvec_clear(&child->args);
 	strvec_clear(&child->env_array);
+	child_process_init(child);
 }
 
 struct child_to_clean {

-Peff

Re: [PATCH] pager: fix crash when pager program doesn't exist

[Enzo Matsumiya]

Hi,

Thanks for the detailed reply. Please see my replies below.

On 11/19, Jeff King wrote:
>On Fri, Nov 19, 2021 at 08:47:45PM -0300, Enzo Matsumiya wrote:
>
>> setup_pager() doesn't properly free pager_process.argv if
>> start_command() fails, nor finish_command() is ever called.
>
>Hmm. It shouldn't need to. It sets up the args using
>prepare_pager_args(), which pushes onto the pager_process->args strvec.
>If start_command fails, then it takes care of freeing that strvec.
>
>In fact, it would be wrong to free that pointer, because it is usually
>just pointing to the strvec's "v" array, which will already be freed by
>strvec_clear().

Agreed, the memory is freed, but argv still points to the old args.v
location.
You can easily check this by adding a breakpoint at start_command and
run my reproducer.

On the first run, argv is NULL, then assigned args.v location.
On the second run, argv points to the old args.v, and args.v has a new
location. That's when strvec_push() will try to use whatever garbage is
at argv.

>> setup_pager() is called twice, once from commit_pager_choice(), and then
>> from cmd_log_init_finish(). On the first run, it runs fine because
>> start_command() assigns cmd->args.v to cmd->argv, and upon command
>> failure, child_process_clear() clears cmd->args.
>
>When pager setup succeeds, the second run is a noop, because isatty(1)
>is no longer true. But for the case you're interested in, the first one
>fails, so we do try again. And I can reproduce your problem with:

No, it's not a noop such as that it's clear that things are different on
the second call.

> GIT_PAGER=no-such-command git -p log
>
>I had to run it with ASan to trigger a failure, as use-after-free bugs
>aren't always deterministic.

Please use my reproducer as it's 100% reliable and consistent (same
memory regions are affected).

I couldn't reproduce the issue with yours.

>> On the second run, though, argv is no longer NULL, but .args has been
>> cleared, so any strvec_push() operation will crash.
>
>Right. So we want to make sure that argv is NULL, but we don't need to
>free it. I think the bug is not in failing to clean up, though. It's in
>assuming that the child_process has been properly initialized. The first
>call works because of the initialization in the declaration, but the
>second call can't rely on that.
>
>So one solution is more like this:
>
>diff --git a/pager.c b/pager.c
>index 52f27a6765..27877f8ebb 100644
>--- a/pager.c
>+++ b/pager.c
>@@ -8,7 +8,7 @@
> #define DEFAULT_PAGER "less"
> #endif
>
>-static struct child_process pager_process = CHILD_PROCESS_INIT;
>+static struct child_process pager_process;
> static const char *pager_program;
>
> /* Is the value coming back from term_columns() just a guess? */
>@@ -124,6 +124,8 @@ void setup_pager(void)
>
> 	setenv("GIT_PAGER_IN_USE", "true", 1);
>
>+	child_process_init(&pager_process);
>+
> 	/* spawn the pager */
> 	prepare_pager_args(&pager_process, pager);
> 	pager_process.in = -1;
>
>which is enough to fix the use-after-free.

This worked for my case.

>> While at it, I implemented a fallback to the DEFAULT_PAGER, so it tries
>> at least one more time when the configured pager fails.
>
>I think this is probably a bad idea. If the user told us to use a
>different pager, then we should not unexpectedly use the default one. At
>any rate, it's unrelated to this patch; if we were to do it, it should
>be separate from the bugfix.

Got it. I was a bit skeptical about this one. Idea discarded.

>> diff --git a/pager.c b/pager.c
>> index 52f27a6765c8..79a47db55d63 100644
>> --- a/pager.c
>> +++ b/pager.c
>> @@ -97,6 +97,9 @@ static void setup_pager_env(struct strvec *env)
>>
>>  void prepare_pager_args(struct child_process *pager_process, const char *pager)
>>  {
>> +	child_process_clear(pager_process);
>> +	if (pager_process->argv)
>> +		free(pager_process->argv);
>
>As noted above, this would be double-freeing the strvec's array. Except
>that your free() here never runs, because you added a memset() to
>child_process_clear() which will always set pager_process->argv to NULL.

Ok.

>> @@ -105,11 +108,14 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
>>
>>  void setup_pager(void)
>>  {
>> -	const char *pager = git_pager(isatty(1));
>> +	const char *tmp_pager = git_pager(isatty(1));
>> +	char *pager;
>>
>> -	if (!pager)
>> +	if (!tmp_pager)
>>  		return;
>>
>> +	pager = xstrdup(tmp_pager);
>> +
>
>Why are we making a copy of the pager string? When it's pushed into the
>strvec, that will take ownership of it. Your patch creates a leak (in
>the case that we start_command() doesn't fail, and we miss the new
>free() calls you added).

Right, thanks.

>> diff --git a/run-command.c b/run-command.c
>> index f329391154ae..d2b7647afdd8 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -19,6 +19,7 @@ void child_process_clear(struct child_process *child)
>>  {
>>  	strvec_clear(&child->args);
>>  	strvec_clear(&child->env_array);
>> +	memset(child, 0, sizeof(*child));
>>  }
>
>This doesn't set up a child_process to be correctly reused, as the
>initialized state for a strvec is not all-bits-zero (though it often
>works in practice).

Noted, thanks.

>The patch I showed above fixes the use-after-free by making sure the
>child_process struct is initialized before we use it. But we could also
>change the semantics of _clear() to make sure it is ready for reuse. To
>do that, you'd want to just call child_process_init() here.
>
>There's some precedent in that with other APIs (e.g., strvec_clear()
>makes the struct ready for reuse immediately). Plus it's harder for the
>callers to get wrong (after adding such a line, setup_pager() does not
>have to do anything else at all).
>
>So this single-line change also fixes the use-after-free, without any of
>the other stuff:
>
>diff --git a/run-command.c b/run-command.c
>index f40df01c77..92e00d9455 100644
>--- a/run-command.c
>+++ b/run-command.c
>@@ -21,6 +21,7 @@ void child_process_clear(struct child_process *child)
> {
> 	strvec_clear(&child->args);
> 	strvec_clear(&child->env_array);
>+	child_process_init(child);
> }
>
> struct child_to_clean {

Of course this one works as well. And is more elegant IMHO.

Again, thanks a lot for the feedback.

Should I submit a v2 or will you?


>-Peff

Cheers,

Enzo

Re: [PATCH] pager: fix crash when pager program doesn't exist

[Enzo Matsumiya]

On 11/19, Enzo Matsumiya wrote:
>>When pager setup succeeds, the second run is a noop, because isatty(1)
>>is no longer true. But for the case you're interested in, the first one
>>fails, so we do try again. And I can reproduce your problem with:
>
>No, it's not a noop such as that it's clear that things are different on
>the second call.

Here I meant that setup_pager() is effectivelly called from 2 different places:

First backtrace:
setup_pager()
commit_pager_choice()
run_builtin()
handle_builtin()
run_argv()
cmd_main()

Second backtrace:
setup_pager()
cmd_log_init_finish()
cmd_log_init()
cmd_show()
run_builtin()
handle_builtin()
run_argv()
cmd_main()

Also, isatty(1) is not false in neither of the calls. Otherwise I
wouldn't hit this bug (pager would be NULL and setup_pager() a noop as
you said).


Cheers,

Enzo

Re: [PATCH] pager: fix crash when pager program doesn't exist

[Jeff King]

On Sat, Nov 20, 2021 at 12:06:21AM -0300, Enzo Matsumiya wrote:

> On 11/19, Enzo Matsumiya wrote:
> > > When pager setup succeeds, the second run is a noop, because isatty(1)
> > > is no longer true. But for the case you're interested in, the first one
> > > fails, so we do try again. And I can reproduce your problem with:
> > 
> > No, it's not a noop such as that it's clear that things are different on
> > the second call.
> 
> Here I meant that setup_pager() is effectivelly called from 2 different places:
> 
> First backtrace:
> setup_pager()
> commit_pager_choice()
> run_builtin()
> handle_builtin()
> run_argv()
> cmd_main()
> 
> Second backtrace:
> setup_pager()
> cmd_log_init_finish()
> cmd_log_init()
> cmd_show()
> run_builtin()
> handle_builtin()
> run_argv()
> cmd_main()
> 
> Also, isatty(1) is not false in neither of the calls. Otherwise I
> wouldn't hit this bug (pager would be NULL and setup_pager() a noop as
> you said).

Right, I mean in the "normal" case that the pager actually starts, the
second call hits isatty(1), then git_pager() returns NULL, and we return
from setup_pager() immediately.

It is only in the broken-pager case that the bug you found is triggered
(which is probably why nobody has really noticed it).

-Peff

Re: [PATCH] pager: fix crash when pager program doesn't exist

[Jeff King]

On Fri, Nov 19, 2021 at 11:32:46PM -0300, Enzo Matsumiya wrote:

> > GIT_PAGER=no-such-command git -p log
> > 
> > I had to run it with ASan to trigger a failure, as use-after-free bugs
> > aren't always deterministic.
> 
> Please use my reproducer as it's 100% reliable and consistent (same
> memory regions are affected).
> 
> I couldn't reproduce the issue with yours.

Our reproducers are triggering the same behavior. But it won't be 100%
reliable in the sense that the behavior is undefined. Depending upon
random details of the allocator, we may get a segfault, or see random
trash on the heap, or even see the old data. That's why I suggested
using ASan; it poisons the freed memory to reliably detect problems.

But at any rate, yes, it's clear that there is a bug here.

> > diff --git a/run-command.c b/run-command.c
> > index f40df01c77..92e00d9455 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -21,6 +21,7 @@ void child_process_clear(struct child_process *child)
> > {
> > 	strvec_clear(&child->args);
> > 	strvec_clear(&child->env_array);
> > +	child_process_init(child);
> > }
> > 
> > struct child_to_clean {
> 
> Of course this one works as well. And is more elegant IMHO.

Yeah, I think so, too.

> Should I submit a v2 or will you?

Why don't you put together a v2, and I can review it.

-Peff