worktree: fix leak in check_clean_worktree()

Commit Message

Jeff King Aug. 27, 2020, 5:25 a.m. UTC

On Thu, Aug 27, 2020 at 01:03:43AM -0400, Eric Sunshine wrote:

> >   - the code right above the second hunk clears cp.args manually. That
> >     shouldn't be necessary because run_command() will leave it in a
> >     blank state (and we're already relying on that, since otherwise we'd
> >     be left with cruft in other fields from the previous run).
> 
> Right. I wonder why the author of 7f44e3d1de (worktree: make setup of
> new HEAD distinct from worktree population, 2015-07-17) chose to clear
> cp.args manually like that.

I wondered if we might not have cleared the array automatically back
then, but it looks like we did.

I do think this kind of child_process struct reuse is slightly sketchy
in general. Looking at child_process_clear(), we only free the memory,
but leave other fields set. And in fact we rely on that here; git_cmd
needs to remain set for both commands to work. But if the first command
had used, say, cp.in, then we'd be left with a bogus descriptor.

> >   - check_clean_worktree() only uses it once, and could drop the
> >     separate child_env (and in fact appears to leak it)
> 
> Perhaps this unnecessary 'child_env' strvec was a copy/paste from
> add_worktree()? But certainly cp.env_array would be simpler and avoid
> the leak.

Yeah, that was my guess, too.

Most of these issues are more complex and/or should be part of a larger
cleanup effort. But let's fix the leak while we're thinking about it.

-- >8 --
Subject: [PATCH] worktree: fix leak in check_clean_worktree()

We allocate a child_env strvec but never free its memory. Instead, let's
just use the strvec that our child_process struct provides, which is
cleaned up automatically when we run the command.

And while we're moving the initialization of the child_process around,
let's switch it to use the official init function (zero-initializing it
works OK, since strvec is happy enough with that, but it sets a bad
example).

Signed-off-by: Jeff King <[e-mail]>
---
 builtin/worktree.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Eric Sunshine Aug. 27, 2020, 5:56 a.m. UTC | #1

On Thu, Aug 27, 2020 at 1:25 AM Jeff King <[e-mail]> wrote:
> On Thu, Aug 27, 2020 at 01:03:43AM -0400, Eric Sunshine wrote:
> > Right. I wonder why the author of 7f44e3d1de (worktree: make setup of
> > new HEAD distinct from worktree population, 2015-07-17) chose to clear
> > cp.args manually like that.
>
> I wondered if we might not have cleared the array automatically back
> then, but it looks like we did.

I had the same thought and came to the same conclusion. It's possible
that the manual clearing of the array was leftover cruft as the
implementation matured before the patch was submitted. I have a vague
(perhaps false) recollection that a local argv_array was populated and
assigned to cp.argv originally, until cp.args was discovered as a
cleaner alternative and used instead. If that was the case, then the
local argv_array wouldn't have been cleared automatically by
run_command(), which would account for clearing it manually.

> I do think this kind of child_process struct reuse is slightly sketchy
> in general. Looking at child_process_clear(), we only free the memory,
> but leave other fields set. And in fact we rely on that here; git_cmd
> needs to remain set for both commands to work. But if the first command
> had used, say, cp.in, then we'd be left with a bogus descriptor.

Agreed. The current usage in worktree.c is a bit too familiar with the
current internal implementation of run_command(). Reinitializing the
child_process struct or using a separate one would be a good cleanup.

> -- >8 --
> Subject: [PATCH] worktree: fix leak in check_clean_worktree()
>
> We allocate a child_env strvec but never free its memory. Instead, let's
> just use the strvec that our child_process struct provides, which is
> cleaned up automatically when we run the command.
>
> And while we're moving the initialization of the child_process around,
> let's switch it to use the official init function (zero-initializing it
> works OK, since strvec is happy enough with that, but it sets a bad
> example).

The various memset()'s in worktree.c seem to have been inherited (and
multiplied) from Duy's original "git checkout --to" implementation
(which later became the basis for "git worktree add" after which it
mutated significantly), and "git checkout --to" predates introduction
of child_process_init().

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -924,7 +924,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)
> -       struct strvec child_env = STRVEC_INIT;
> @@ -935,15 +934,14 @@ static void check_clean_worktree(struct worktree *wt,
> -       strvec_pushf(&child_env, "%s=%s/.git",
> +       child_process_init(&cp);
> +       strvec_pushf(&cp.env_array, "%s=%s/.git",
>                      GIT_DIR_ENVIRONMENT, wt->path);
> -       strvec_pushf(&child_env, "%s=%s",
> +       strvec_pushf(&cp.env_array, "%s=%s",
>                      GIT_WORK_TREE_ENVIRONMENT, wt->path);
> -       memset(&cp, 0, sizeof(cp));
> -       cp.env = child_env.v;

Looks good to me. For what it's worth:

Reviewed-by: Eric Sunshine <[e-mail]>

Junio C Hamano Aug. 27, 2020, 3:31 p.m. UTC | #2

Eric Sunshine <[e-mail]> writes:

> Agreed. The current usage in worktree.c is a bit too familiar with the
> current internal implementation of run_command(). Reinitializing the
> child_process struct or using a separate one would be a good cleanup.
>
>> -- >8 --
>> Subject: [PATCH] worktree: fix leak in check_clean_worktree()
>>
>> We allocate a child_env strvec but never free its memory. Instead, let's
>> just use the strvec that our child_process struct provides, which is
>> cleaned up automatically when we run the command.
>>
>> And while we're moving the initialization of the child_process around,
>> let's switch it to use the official init function (zero-initializing it
>> works OK, since strvec is happy enough with that, but it sets a bad
>> example).
>
> The various memset()'s in worktree.c seem to have been inherited (and
> multiplied) from Duy's original "git checkout --to" implementation
> (which later became the basis for "git worktree add" after which it
> mutated significantly), and "git checkout --to" predates introduction
> of child_process_init().
>
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -924,7 +924,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>> -       struct strvec child_env = STRVEC_INIT;
>> @@ -935,15 +934,14 @@ static void check_clean_worktree(struct worktree *wt,
>> -       strvec_pushf(&child_env, "%s=%s/.git",
>> +       child_process_init(&cp);
>> +       strvec_pushf(&cp.env_array, "%s=%s/.git",
>>                      GIT_DIR_ENVIRONMENT, wt->path);
>> -       strvec_pushf(&child_env, "%s=%s",
>> +       strvec_pushf(&cp.env_array, "%s=%s",
>>                      GIT_WORK_TREE_ENVIRONMENT, wt->path);
>> -       memset(&cp, 0, sizeof(cp));
>> -       cp.env = child_env.v;
>
> Looks good to me. For what it's worth:
>
> Reviewed-by: Eric Sunshine <[e-mail]>

Thanks, both.  This looks good.

Patch

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 378f332b5d..df214697d2 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -924,7 +924,6 @@  static int move_worktree(int ac, const char **av, const char *prefix)
 static void check_clean_worktree(struct worktree *wt,
 				 const char *original_path)
 {
-	struct strvec child_env = STRVEC_INIT;
 	struct child_process cp;
 	char buf[1];
 	int ret;
@@ -935,15 +934,14 @@  static void check_clean_worktree(struct worktree *wt,
 	 */
 	validate_no_submodules(wt);
 
-	strvec_pushf(&child_env, "%s=%s/.git",
+	child_process_init(&cp);
+	strvec_pushf(&cp.env_array, "%s=%s/.git",
 		     GIT_DIR_ENVIRONMENT, wt->path);
-	strvec_pushf(&child_env, "%s=%s",
+	strvec_pushf(&cp.env_array, "%s=%s",
 		     GIT_WORK_TREE_ENVIRONMENT, wt->path);
-	memset(&cp, 0, sizeof(cp));
 	strvec_pushl(&cp.args, "status",
 		     "--porcelain", "--ignore-submodules=none",
 		     NULL);
-	cp.env = child_env.v;
 	cp.git_cmd = 1;
 	cp.dir = wt->path;
 	cp.out = -1;