| Message ID | 20211124145409.8779-1-ematsumiya@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] pager: fix crash when pager program doesn't exist | expand |
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.
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
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
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(+)