Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 18, 2022, 15:13 UTC
Message-ID: <220718.86zgh6wiwa.gmgdl@evledraar.gmail.com>
In-Reply-To: <YtV4KmrTBkmcx6m3@coredump.intra.peff.net>

On Mon, Jul 18 2022, Jeff King wrote:

Show 10 quoted lines

> On Wed, Jul 13, 2022 at 03:10:35PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> There's several potential ways to fix those sort of leaks, we could
>> add a "nodup" mode to "struct strvec", which would work for the cases
>> where we push constant strings to it. But that wouldn't work as soon
>> as we used strvec_pushf(), or otherwise needed to duplicate or create
>> a string for that "struct strvec".
>
> Right, I think this falls down when you have mixed const/non-const
> entries. You have to know which is which for strvec_clear().

Yes, that aspect of it sucks, such a caller will need to do its own bespoke free-ing.

In practice I haven't really found such callers, API users tend to use one or the other. Either they're using strvec (needs free-ing) or main()'s argv (no free-ing).

To the extent that that wasn't the case I figured the easiest fix for those is probably to fully convert them to strvec, the copies being trivial in the larger context...

Show 17 quoted lines

>> Let's instead make it the responsibility of the revisions API. If it's
>> going to clobber elements of argv it can also free() them, which it
>> will now do if instructed to do so via "free_removed_argv_elements".
>
> OK, I think this is a reasonable and minimal fix for the "--" problem on
> its own.
>
> But if you went just a little further and made the option "rev should
> own its own argv", then I think you can simplify life for callers even
> more. They could construct a strvec themselves and then hand it off to
> the rev_info, to be cleaned up when release_revisions() is called (and
> of course freeing the "--" when we overwrite it in the interim, as you
> do here).
>
> Then all of the bisect callers from the previous patch could avoid
> having to deal with the strvec at all. They'd call bisect_rev_setup(),
> which would internally attach the memory to rev_info.

Yes, I experimented with this, and it's a solid approach.

But it's a much larger change, particularly since we'd also want to update the API itself to take take "const" in the appropriate places to do it properly.