{"thread":{"id":"56972","subject":"[PATCH v4] pager: fix crash when pager program doesn't exist","startedAt":"2021-11-25T00:02:49Z","lastAt":"2021-11-26T06:25:08Z","messageCount":2,"participants":["Enzo Matsumiya","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"442278","messageId":"20211125000239.2336-1-ematsumiya@suse.de","threadId":"56972","inReplyTo":null,"subject":"[PATCH v4] pager: fix crash when pager program doesn't exist","fromName":"Enzo Matsumiya","fromEmail":"ematsumiya@suse.de","sentAt":"2021-11-25T00:02:39Z","receivedAt":"2021-11-25T00:02:49Z","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(). Drop CHILD_PROCESS_INIT from its declaration since it's\nno longer really necessary.\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 v3:\n - change test description, as per Eric's suggestion\n - drop CHILD_PROCESS_INIT, as per Jeff's suggestion\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          | 4 +++-\n t/t7006-pager.sh | 5 +++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/pager.c b/pager.c\nindex 52f27a6765c8..27877f8ebbc1 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -8,7 +8,7 @@\n #define DEFAULT_PAGER \"less\"\n #endif\n \n-static struct child_process pager_process = CHILD_PROCESS_INIT;\n+static struct child_process pager_process;\n static const char *pager_program;\n \n /* Is the value coming back from term_columns() just a guess? */\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..eed680a2b1ee 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 'non-existent pager doesnt cause crash' '\n+\ttest_config pager.show invalid-pager &&\n+\ttest_terminal git show\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"442341","messageId":"xmqqbl27a116.fsf@gitster.g","threadId":"56972","inReplyTo":"20211125000239.2336-1-ematsumiya@suse.de","subject":"Re: [PATCH v4] pager: fix crash when pager program doesn't exist","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-26T06:23:01Z","receivedAt":"2021-11-26T06:25:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Enzo Matsumiya <ematsumiya@suse.de> writes:\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(). Drop CHILD_PROCESS_INIT from its declaration since it's\n> no longer really necessary.\n>\n> Add a test to catch possible regressions.\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> Changes to v3:\n>  - change test description, as per Eric's suggestion\n>  - drop CHILD_PROCESS_INIT, as per Jeff's suggestion\n\nThanks.  Everything looks good.  Will queue.\n"}]}