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

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

[Enzo Matsumiya]

When prepare_cmd() fails for, e.g., pager process setup,
child_process_clear() frees the memory in pager_process.args, but .argv
was pointed to pager_process.args.v earlier in start_command(), so it's
now a dangling pointer.

setup_pager() is then called a second time, from cmd_log_init_finish()
in this case, and any further operations using its .argv, e.g. strvec_*,
will use the dangling pointer and eventually crash. According to trivial
tests, setup_pager() is not called twice if the first call is
successful.

This patch makes sure that pager_process is properly initialized on
setup_pager().

Add a test to catch possible regressions.

Reproducer:
$ 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

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
Changes to v2:
 - move child_process_init() to setup_pager(), as other callers of
   child_process_clear() might've not expected .argv to change
 - add a test case with the reproducer of the original bug

Changes to v1:
 * Implement all of Jeff's suggestions:
   - remove double frees to .argv
   - discard the idea of falling back to DEFAULT_PAGER
   - replace memset() in child_process_clear() by child_process_init()
   - update/improve commit message

 pager.c          | 2 ++
 t/t7006-pager.sh | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/pager.c b/pager.c
index 52f27a6765c8..d93304527d62 100644
--- a/pager.c
+++ b/pager.c
@@ -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;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 0e7cf75435ec..0be9bcba49a8 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -786,4 +786,9 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
 	test_path_is_file pager-used
 '
 
+test_expect_success TTY 'handle attempt to run an invalid pager' '
+	test_config pager.show invalid-pager &&
+	test_terminal git show
+'
+
 test_done
-- 
2.33.1

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

[Eric Sunshine]

On Wed, Nov 24, 2021 at 9:54 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
> [...]
> Reproducer:
> $ 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
>
> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> ---
> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> @@ -786,4 +786,9 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
> +test_expect_success TTY 'handle attempt to run an invalid pager' '
> +       test_config pager.show invalid-pager &&
> +       test_terminal git show
> +'

This is a minor observation (so you decide what value it might have),
but the terminology "handle ... invalid pager" in the test title
doesn't convey very much information to some future reader of this
test, and that person will be forced to consult the commit message --
which does a good job of explaining the problem -- to really
understand what this test is checking. If you change the title to, for
instance:

    non-existent pager doesn't cause crash

then the reader will have an easier time understanding what this test
is about. It's true that the reader will still need to consult the
commit message for a detailed picture of the problem, but won't be
left head-scratching, as might be the case with the current test
title.

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

[Jeff King]

On Wed, Nov 24, 2021 at 11:54:09AM -0300, Enzo Matsumiya wrote:

> When prepare_cmd() fails for, e.g., pager process setup,
> child_process_clear() frees the memory in pager_process.args, but .argv
> was pointed to pager_process.args.v earlier in start_command(), so it's
> now a dangling pointer.
> 
> setup_pager() is then called a second time, from cmd_log_init_finish()
> in this case, and any further operations using its .argv, e.g. strvec_*,
> will use the dangling pointer and eventually crash. According to trivial
> tests, setup_pager() is not called twice if the first call is
> successful.
> 
> This patch makes sure that pager_process is properly initialized on
> setup_pager().
> 
> Add a test to catch possible regressions.

Oh, good. I just replied in the separate thread suggesting this
direction, but I see you had already sent this. :)

This patch looks good to me. Two small nits:

> diff --git a/pager.c b/pager.c
> index 52f27a6765c8..d93304527d62 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -124,6 +124,8 @@ void setup_pager(void)
>  
>  	setenv("GIT_PAGER_IN_USE", "true", 1);
>  
> +	child_process_init(&pager_process);
> +

You could drop the CHILD_PROCESS_INIT initializer in the declaration of
pager_process now. I'm OK with keeping it, though, as a
belt-and-suspenders thing. If we don't run setup_pager() at all I don't
think anybody should look at it (since we won't have installed our
signal/atexit cleanup handlers), but it doesn't hurt.

> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index 0e7cf75435ec..0be9bcba49a8 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -786,4 +786,9 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
>  	test_path_is_file pager-used
>  '
>  
> +test_expect_success TTY 'handle attempt to run an invalid pager' '
> +	test_config pager.show invalid-pager &&
> +	test_terminal git show
> +'

Yep, this should trigger the bug. I agree with Eric that "non-existent"
is probably more descriptive, as the key thing here is that
start_command() fails immediately, rather than us piping to the broken
pager.

-Peff