git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH v3] pager: fix crash when pager program doesn't exist
@ 2021-11-24 14:54 Enzo Matsumiya
  2021-11-24 15:15 ` Eric Sunshine
  2021-11-24 15:55 ` Jeff King
  0 siblings, 2 replies; 3+ messages in thread
From: Enzo Matsumiya @ 2021-11-24 14:54 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] pager: fix crash when pager program doesn't exist
  2021-11-24 14:54 [PATCH v3] pager: fix crash when pager program doesn't exist Enzo Matsumiya
@ 2021-11-24 15:15 ` Eric Sunshine
  2021-11-24 15:55 ` Jeff King
  1 sibling, 0 replies; 3+ messages in thread
From: Eric Sunshine @ 2021-11-24 15:15 UTC (permalink / raw)
  To: Enzo Matsumiya; +Cc: Git List

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.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] pager: fix crash when pager program doesn't exist
  2021-11-24 14:54 [PATCH v3] pager: fix crash when pager program doesn't exist Enzo Matsumiya
  2021-11-24 15:15 ` Eric Sunshine
@ 2021-11-24 15:55 ` Jeff King
  1 sibling, 0 replies; 3+ messages in thread
From: Jeff King @ 2021-11-24 15:55 UTC (permalink / raw)
  To: Enzo Matsumiya; +Cc: git

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-11-24 15:55 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-24 14:54 [PATCH v3] pager: fix crash when pager program doesn't exist Enzo Matsumiya
2021-11-24 15:15 ` Eric Sunshine
2021-11-24 15:55 ` Jeff King

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).