[PATCH] pager: fix crash when pager program doesn't exist
- From
Enzo Matsumiya <ematsumiya@suse.de>
- Date
- Nov 19, 2021, 23:47 UTC
- Message-ID
- <20211119234745.26605-1-ematsumiya@suse.de>
setup_pager() doesn't properly free pager_process.argv if start_command() fails, nor finish_command() is ever called.
setup_pager() is called twice, once from commit_pager_choice(), and then from cmd_log_init_finish(). On the first run, it runs fine because start_command() assigns cmd->args.v to cmd->argv, and upon command failure, child_process_clear() clears cmd->args.
On the second run, though, argv is no longer NULL, but .args has been cleared, so any strvec_push() operation will crash.
This patch makes sure that freeing pager_process.argv is part of its argument preparation, as well as zeroing the whole pager_process struct.
Reproduction: $ 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
While at it, I implemented a fallback to the DEFAULT_PAGER, so it tries at least one more time when the configured pager fails.
Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> --- pager.c | 25 +++++++++++++++++++++---- run-command.c | 1 + 2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/pager.c b/pager.c index 52f27a6765c8..79a47db55d63 100644 --- a/pager.c +++ b/pager.c @@ -97,6 +97,9 @@ static void setup_pager_env(struct strvec *env) void prepare_pager_args(struct child_process *pager_process, const char *pager) { + child_process_clear(pager_process); + if (pager_process->argv) + free(pager_process->argv); strvec_push(&pager_process->args, pager); pager_process->use_shell = 1; setup_pager_env(&pager_process->env_array); @@ -105,11 +108,14 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager) void setup_pager(void) { - const char *pager = git_pager(isatty(1)); + const char *tmp_pager = git_pager(isatty(1)); + char *pager; - if (!pager) + if (!tmp_pager) return; + pager = xstrdup(tmp_pager); + /* * After we redirect standard output, we won't be able to use an ioctl * to get the terminal size. Let's grab it now, and then set $COLUMNS @@ -124,12 +130,23 @@ void setup_pager(void) setenv("GIT_PAGER_IN_USE", "true", 1); +retry: /* spawn the pager */ prepare_pager_args(&pager_process, pager); pager_process.in = -1; strvec_push(&pager_process.env_array, "GIT_PAGER_IN_USE"); - if (start_command(&pager_process)) - return; + if (start_command(&pager_process)) { + /* chosen pager failed, try again with the default pager */ + if (strcmp(pager, DEFAULT_PAGER)) { + free(pager); + pager = xstrdup(DEFAULT_PAGER); + goto retry; + } else { + /* default pager failed, let caller handle it */ + free(pager); + return; + } + } /* original process continues, but writes to the pipe */ dup2(pager_process.in, 1); diff --git a/run-command.c b/run-command.c index f329391154ae..d2b7647afdd8 100644 --- a/run-command.c +++ b/run-command.c @@ -19,6 +19,7 @@ void child_process_clear(struct child_process *child) { strvec_clear(&child->args); strvec_clear(&child->env_array); + memset(child, 0, sizeof(*child)); } struct child_to_clean {
-- 2.33.1