Re: [PATCH v2 5/9] builtin/repack.c: avoid leaking child arguments
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Oct 28, 2021, 20:25 UTC
- Message-ID
- <YXsHTKGBYYeDsdhh@nand.local>
- In-Reply-To
- <xmqqzgqut4lr.fsf@gitster.g>
On Wed, Oct 27, 2021 at 04:44:48PM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> 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