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 <peff@xxxxxxxx>
---
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
