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

From: Taylor Blau <[email-masked]>
Date: Thu, 21 Oct 2021 23:21:53 -0400

On Thu, Oct 21, 2021 at 09:37:07AM -0700, Junio C Hamano wrote:
> Taylor Blau <[email-masked]> writes:
>
> > `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.
> >
> > In none of these cases do we bother to call child_process_clear(), which
> > frees the memory associated with each child's arguments and environment.
> >
> > In order to do so, tweak each function that spawns a child process to
> > have a `cleanup` label that we always visit before returning from each
> > function. Then, make sure that we call child_process_clear() as a part
> > of that label.
>
> I have a slight aversion against the fact that this patch makes us
> call child_process_clear(), when we know we may have called
> finish_command().  Callers of finish_command() in other codepaths
> rely on the fact that finish_command() calls child_process_clear(),
> which means that because of this change, we are now constrained to
> keep child_process_clear() idempotent?

Good point. In functions besides cmd_repack(), it's easy enough to just
call child_process_clear() once before returning instead of using a
label. That has the added benefit of side-stepping the variable name
question that you and Stolee raised (but I think whether we call it
"result", "res", or "ret" is besides the point).

In cmd_repack(), I'd like to wrap the other cleanup-related calls
underneath a 'cleanup' label. So there we want to track whether or not
we have called finish_command(), so that we only call
child_process_clear() when we *haven't* already called it via
finish_command().

Thanks,
Taylor