{"thread":{"id":"56969","subject":"[PATCH v3] pager: fix crash when pager program doesn't exist","startedAt":"2021-11-24T14:54:26Z","lastAt":"2021-11-24T15:55:28Z","messageCount":3,"participants":["Enzo Matsumiya","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"442246","messageId":"20211124145409.8779-1-ematsumiya@suse.de","threadId":"56969","inReplyTo":null,"subject":"[PATCH v3] pager: fix crash when pager program doesn't exist","fromName":"Enzo Matsumiya","fromEmail":"ematsumiya@suse.de","sentAt":"2021-11-24T14:54:09Z","receivedAt":"2021-11-24T14:54:26Z","isPatch":true,"sender":{"key":"ematsumiya@suse.de","avatar":"https://avatars.githubusercontent.com/u/45605263?v=4"},"body":"When prepare_cmd() fails for, e.g., pager process setup,\nchild_process_clear() frees the memory in pager_process.args, but .argv\nwas pointed to pager_process.args.v earlier in start_command(), so it's\nnow a dangling pointer.\n\nsetup_pager() is then called a second time, from cmd_log_init_finish()\nin this case, and any further operations using its .argv, e.g. strvec_*,\nwill use the dangling pointer and eventually crash. According to trivial\ntests, setup_pager() is not called twice if the first call is\nsuccessful.\n\nThis patch makes sure that pager_process is properly initialized on\nsetup_pager().\n\nAdd a test to catch possible regressions.\n\nReproducer:\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\nSigned-off-by: Enzo Matsumiya <ematsumiya@suse.de>\n---\nChanges to v2:\n - move child_process_init() to setup_pager(), as other callers of\n   child_process_clear() might've not expected .argv to change\n - add a test case with the reproducer of the original bug\n\nChanges to v1:\n * Implement all of Jeff's suggestions:\n   - remove double frees to .argv\n   - discard the idea of falling back to DEFAULT_PAGER\n   - replace memset() in child_process_clear() by child_process_init()\n   - update/improve commit message\n\n pager.c          | 2 ++\n t/t7006-pager.sh | 5 +++++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/pager.c b/pager.c\nindex 52f27a6765c8..d93304527d62 100644\n--- a/pager.c\n+++ b/pager.c\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;\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 0e7cf75435ec..0be9bcba49a8 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -786,4 +786,9 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n \ttest_path_is_file pager-used\n '\n \n+test_expect_success TTY 'handle attempt to run an invalid pager' '\n+\ttest_config pager.show invalid-pager &&\n+\ttest_terminal git show\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"442249","messageId":"CAPig+cT0-bu2tcgJrRR+9J86bmGEOOpFZv0JygCp26gieP-2pg@mail.gmail.com","threadId":"56969","inReplyTo":"20211124145409.8779-1-ematsumiya@suse.de","subject":"Re: [PATCH v3] pager: fix crash when pager program doesn't exist","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-11-24T15:15:29Z","receivedAt":"2021-11-24T15:15:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 24, 2021 at 9:54 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:\n> [...]\n> Reproducer:\n> $ git config pager.show INVALID_PAGER\n> $ git show $VALID_COMMIT\n> error: cannot run INVALID_PAGER: No such file or directory\n> [1]    3619 segmentation fault (core dumped)  git show $VALID_COMMIT\n>\n> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>\n> ---\n> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n> @@ -786,4 +786,9 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n> +test_expect_success TTY 'handle attempt to run an invalid pager' '\n> +       test_config pager.show invalid-pager &&\n> +       test_terminal git show\n> +'\n\nThis is a minor observation (so you decide what value it might have),\nbut the terminology \"handle ... invalid pager\" in the test title\ndoesn't convey very much information to some future reader of this\ntest, and that person will be forced to consult the commit message --\nwhich does a good job of explaining the problem -- to really\nunderstand what this test is checking. If you change the title to, for\ninstance:\n\n    non-existent pager doesn't cause crash\n\nthen the reader will have an easier time understanding what this test\nis about. It's true that the reader will still need to consult the\ncommit message for a detailed picture of the problem, but won't be\nleft head-scratching, as might be the case with the current test\ntitle.\n"},{"id":"442251","messageId":"YZ5gao4sZF3R9mD8@coredump.intra.peff.net","threadId":"56969","inReplyTo":"20211124145409.8779-1-ematsumiya@suse.de","subject":"Re: [PATCH v3] pager: fix crash when pager program doesn't exist","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-24T15:55:22Z","receivedAt":"2021-11-24T15:55:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 24, 2021 at 11:54:09AM -0300, Enzo Matsumiya wrote:\n\n> When prepare_cmd() fails for, e.g., pager process setup,\n> child_process_clear() frees the memory in pager_process.args, but .argv\n> was pointed to pager_process.args.v earlier in start_command(), so it's\n> now a dangling pointer.\n> \n> setup_pager() is then called a second time, from cmd_log_init_finish()\n> in this case, and any further operations using its .argv, e.g. strvec_*,\n> will use the dangling pointer and eventually crash. According to trivial\n> tests, setup_pager() is not called twice if the first call is\n> successful.\n> \n> This patch makes sure that pager_process is properly initialized on\n> setup_pager().\n> \n> Add a test to catch possible regressions.\n\nOh, good. I just replied in the separate thread suggesting this\ndirection, but I see you had already sent this. :)\n\nThis patch looks good to me. Two small nits:\n\n> diff --git a/pager.c b/pager.c\n> index 52f27a6765c8..d93304527d62 100644\n> --- a/pager.c\n> +++ b/pager.c\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\nYou could drop the CHILD_PROCESS_INIT initializer in the declaration of\npager_process now. I'm OK with keeping it, though, as a\nbelt-and-suspenders thing. If we don't run setup_pager() at all I don't\nthink anybody should look at it (since we won't have installed our\nsignal/atexit cleanup handlers), but it doesn't hurt.\n\n> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\n> index 0e7cf75435ec..0be9bcba49a8 100755\n> --- a/t/t7006-pager.sh\n> +++ b/t/t7006-pager.sh\n> @@ -786,4 +786,9 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '\n>  \ttest_path_is_file pager-used\n>  '\n>  \n> +test_expect_success TTY 'handle attempt to run an invalid pager' '\n> +\ttest_config pager.show invalid-pager &&\n> +\ttest_terminal git show\n> +'\n\nYep, this should trigger the bug. I agree with Eric that \"non-existent\"\nis probably more descriptive, as the key thing here is that\nstart_command() fails immediately, rather than us piping to the broken\npager.\n\n-Peff\n"}]}