Re: [PATCH v2] pager: fix crash when pager program doesn't exist
- From
Jeff King <peff@peff.net>
- Date
- Nov 22, 2021, 16:30 UTC
- Message-ID
- <YZvFkwivicJ/tFAo@coredump.intra.peff.net>
- In-Reply-To
- <20211122153119.h2t2ti3lkiycd7pb@cyberdelia>
On Mon, Nov 22, 2021 at 12:31:19PM -0300, Enzo Matsumiya wrote:
Show 23 quoted lines
> On 11/21, Junio C Hamano wrote:
> > It is rather common for us to reuse "struct child_process" in a code
> > path, e.g. builtin/worktree.c::add_worktree() prepares a single
> > instance of such a struct, sets cp.git_cmd to true, and runs either
> > "update-ref" or "symbolic-ref" to update "HEAD". After successful
> > invocation of such a git subcommand, it then runs "git reset --hard",
> > with this piece of code:
>
> Do you agree that at least NULLing .argv and .env could be part of
> child_process_clear()?
>
> diff --git a/run-command.c b/run-command.c
> index a7bf81025afb..3839a26eff11 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -18,8 +18,9 @@ void child_process_init(struct child_process *child)
> void child_process_clear(struct child_process *child)
> {
> strvec_clear(&child->args);
> + child->argv = NULL;
> strvec_clear(&child->env_array);
> + child->env = NULL;
> }It's still potentially dangerous, as callers which do not use child->args could be pointing child->argv to to a more stable array, and expecting it to be reused. It's definitely _less_ dangerous, and maybe even OK, but you'd still have to look at the callers.
What would be safe is NULLing them if we know they point to the strvecs that are about to be cleared, like so:
diff --git a/run-command.c b/run-command.c index f40df01c77..60c7331213 100644 --- a/run-command.c +++ b/run-command.c @@ -19,6 +19,10 @@ void child_process_init(struct child_process *child) void child_process_clear(struct child_process *child) { + if (child->argv == child->args.v) + child->argv = NULL; + if (child->env == child->env_array.v) + child->env = NULL; strvec_clear(&child->args); strvec_clear(&child->env_array); } > With this change on main, all tests are successful. That gives us some confidence, but I don't fully trust our tests to cover every corner case (the bug you found being a good example :) ). -Peff