[PATCH 0/6] fixing double-frees and leaks via setup_revisions()

From: Jeff King <peff@peff.net>
Date: Sep 19, 2025, 22:33 UTC
Message-ID: <20250919223351.GA3906184@coredump.intra.peff.net>
In-Reply-To: <20250919195626.GA2750626@coredump.intra.peff.net>

On Fri, Sep 19, 2025 at 03:56:26PM -0400, Jeff King wrote:

Show 20 quoted lines

> > > But really, I do not know that we need to NULL the whole thing. We have
> > > given the caller the reduced argc. The only argv invariant we are
> > > violating is that argv[argc] should be NULL (or in this case,
> > > argv[left]). Anything after argv+left should be considered
> > > uninitialized. So just:
> > >
> > >   argv[left] = NULL;
> > >
> > > would be enough, I'd think.
> > 
> > Even when strvec was passed and more than one element was eaten
> > after parsing?  strvec_clear() goes by .nr not stopping at the first
> > NULL IIRC.
> 
> Yes, there is a big can of worms here. ;) It turns out that many spots
> with strvecs were relying on leaving these entries untouched, and so
> setting any of them to NULL causes leaks.
> 
> I think I've got it mostly worked out, but that's why I haven't sent
> patches yet. Stay tuned.

OK, here's what I've come up with. It passes the regular test suite, as well as with LSan, ASan, and UBSan. There were definitely some ugly gotchas in there.

It _almost_ makes me think that setup_revisions() should never munge argv at all, and should just produce a separate strvec of unknown options. That keeps the concerns separate, and the extra allocation cast is trivial. But we'd have to modify every caller to handle the new world order.

So anyway, here it is:

  [1/6]: stash: tell setup_revisions() to free our allocated strings

    This fixes a real (albeit tiny) leak.

  [2/6]: revision: manage memory ownership of argv in setup_revisions()

    This fixes the double-free that started us off, but also addresses
    the related leak issues that our earlier discussion had.

  [3/6]: revision: add wrapper to setup_revisions() from a strvec
  [4/6]: treewide: use setup_revisions_from_strvec() when we have a strvec
  [5/6]: treewide: pass strvecs around for setup_revisions_from_strvec()

    The rest of this is optional. These three introduce a convenience
    wrapper, which I do think makes the code a little shorter and
    eliminates a possible footgun. But AFAIK it's not actually fixing
    any bugs.

  [6/6]: revision: retain argv NULL invariant in setup_revisions()

    This one does the NULL thing we discussed earlier for all callers.
    It turned up some interesting gotchas! One is that without the
    wrapper from patches 3-5, it introduces leaks for strvec users which
    assume (correctly) that setup_revisions() will never munge their
    argv or need to free anything. And two, lots of callers pass a NULL
    argv to setup_revisions. Yikes.

 bisect.c                    |  5 +---
 builtin/describe.c          |  3 ++-
 builtin/pack-objects.c      |  6 ++---
 builtin/rebase.c            |  3 +--
 builtin/stash.c             |  4 +--
 builtin/submodule--helper.c | 10 ++------
 http-push.c                 |  2 +-
 remote.c                    |  5 +---
 revision.c                  | 49 ++++++++++++++++++++++++++++++++++---
 revision.h                  |  2 ++
 sequencer.c                 |  7 +++---
 sequencer.h                 |  4 +--
 shallow.c                   |  4 +--
 shallow.h                   |  4 +--
 submodule.c                 |  2 +-
 t/t3903-stash.sh            | 11 +++++++++
 upload-pack.c               |  7 +++---
 17 files changed, 86 insertions(+), 42 deletions(-)

-Peff