Re: [PATCH 5/6] bisect.c: partially fix bisect_rev_setup() memory leak

[Jeff King <peff@peff.net>]

On Wed, Jul 13, 2022 at 03:10:34PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Partially fix the memory leak noted in in 8a534b61241 (bisect: use
> argv_array API, 2011-09-13), which added the "XXX" comment seen in the
> context. We can partially fix it by having the bisect_rev_setup()
> function take a "struct strvec", rather than constructing it.
> 
> As the comment notes we need to keep the construct "rev_argv" around
> while the "struct rev_info" is around, which as seen in the newly
> added "strvec_clear()" calls here we do after "release_revisions()".

This seems like not too bad a solution on its face. The caller is
responsible for keeping rev_argv around as long as rev is valid.

This would be simpler if setup_revisions() made its own copy of the argv
strings and dropped them via release_revisions(). That has a small cost,
but I suspect it doesn't matter much in practice.

I thought that would be too much of a pain to add, but it kind of looks
like you went there anyway in the next patch. I'll comment further
there.

> This "partially" fixes the memory leak because we're leaking the "--"
> added to the "rev_argv" here still, which will be addressed in a
> subsequent commit.

This part confused me, because the "--" is in the strvec which gets
cleared. I'd guess that the revisions code just overwrites it with NULL,
and we lose access to it. But it should become clear in the next commit.

> -	setup_revisions(rev_argv.nr, rev_argv.v, revs, NULL);
> +	setup_revisions(rev_argv->nr, rev_argv->v, revs, NULL);
>  	/* XXX leak rev_argv, as "revs" may still be pointing to it */

I wouldn't really call this a "leak" anymore; the caller now owns it.
Maybe nit-picking, since this comment goes away later.

-Peff