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

From: Jeff King <peff@peff.net>
Date: Jul 18, 2022, 15:45 UTC
Message-ID: <YtWAMP0ROFseFs6B@coredump.intra.peff.net>
In-Reply-To: <220718.86zgh6wiwa.gmgdl@evledraar.gmail.com>

On Mon, Jul 18, 2022 at 05:13:05PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 16 quoted lines

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

Hmm. I was thinking that we'd just have rev_info.we_own_our_argv, and then release it in release_revisions(). But actually, rev_info does not hold onto the argv at all! It's totally processed in setup_revisions().

And there it either:

  - becomes part of prune_data (in which case a copy is made)

  - is passed to handle_revisions_opt(), etc, in which case it is parsed
    but not held onto (it's possible that some string option holds onto
    a pointer, but the only one I found is --filter, which makes a
    copy).

  - is passed to handle_revision_arg(), but these days
    add_rev_cmdline(), etc, make copies. As they must, otherwise --stdin
    would be totally buggy.

So I actually wonder if the comment in bisect_rev_setup() is simply wrong. It was correct in 2011 when I wrote it, but things changed in df835d3a0c (add_rev_cmdline(): make a copy of the name argument, 2013-05-25), etc.

In that case, we could replace your patch 5 in favor of just calling strvec_clear() at the end of bisect_rev_setup(). It's possible there's a case I'm missing that makes this generally not-safe, but in the case of bisect_rev_setup() there's a very limited set of items in our argv in the first place. Doing so also passes the test suite with SANITIZE=address, though again, this is just exercising the very limited bisect options here.

-Peff