On Wed, Oct 27, 2021 at 04:44:48PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
---
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
