[PATCH] worktree: fix leak in check_clean_worktree()

From: Jeff King <peff@peff.net>
Date: Aug 27, 2020, 05:25 UTC
Message-ID: <20200827052504.GA3360984@coredump.intra.peff.net>
In-Reply-To: <CAPig+cSA56xgNN0WP4t+YoyNU8fGf5eaz__=4Vh+s=He-tG=DA@mail.gmail.com>

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

Show 8 quoted lines

> >   - 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.

Show 6 quoted lines

> >   - 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 <peff@peff.net>
---
 builtin/worktree.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

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;

-- 
2.28.0.751.g0834cceced