On Mon, Nov 22, 2021 at 12:31:19PM -0300, Enzo Matsumiya wrote:
> 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
