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@xxxxxxx>
---
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
