Re: [PATCH v2 5/9] builtin/repack.c: avoid leaking child arguments

[Taylor Blau <me@ttaylorr.com>]

On Wed, Oct 27, 2021 at 04:44:48PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > @@ -586,8 +588,10 @@ static int write_midx_included_packs(struct string_list *include,
> >  		strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);
> >
> >  	ret = start_command(&cmd);
> > -	if (ret)
> > +	if (ret) {
> > +		child_process_clear(&cmd);
> >  		return ret;
> > +	}
>
> This happens only when start_command() returns an error.  But the
> function always calls child_process_clear() before doing so.
>
> So I am not sure if this hunk is needed.  It didn't exist in v1, if
> I recall correctly.  Am I missing something obvious?

No, it was the person replying to you missing something obvious ;).

Any hunks like this that call child_process_clear() after
start_command() returns a non-zero value are unnecessary. But the one in
repack_promisor_objects() is good, and does prevent the leak that had
led me in this direction in the first place.

Here is a suitable replacement for this patch (I believe that everything
else in this version is fine as-is):

--- >8 ---

Subject: [PATCH] builtin/repack.c: avoid leaking child arguments

`git repack` invokes a handful of child processes: one to write the
actual pack, and optionally ones to repack promisor objects and update
the MIDX.

Most of these are freed automatically by calling `start_command()` (which
invokes it on error) and `finish_command()` which calls it
automatically.

But repack_promisor_objects() can initialize a child_process, populate
its array of arguments, and then return from the function before even
calling start_command().

Make sure that the prepared list of arguments is freed by calling
child_process_clear() ourselves to avoid leaking memory along this path.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/repack.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 0b2d1e5d82..9b74e0d468 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -258,9 +258,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 	for_each_packed_object(write_oid, &cmd,
 			       FOR_EACH_OBJECT_PROMISOR_ONLY);

-	if (cmd.in == -1)
+	if (cmd.in == -1) {
 		/* No packed objects; cmd was never started */
+		child_process_clear(&cmd);
 		return;
+	}

 	close(cmd.in);

--
2.33.0.96.g73915697e6