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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jul 18 2022, Jeff King wrote:

> 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...

>> 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.