Re: [PATCH] revision: hang on to "freed" argv elements

[Jeff King <peff@peff.net>]

On Tue, Sep 01, 2026 at 10:51:26AM +0200, Patrick Steinhardt wrote:

> > This fixes the prefix case above (which is now tested in t3903), and
> > should fix any other stray cases. Though I could not find any; we use
> > OPT_STRING only in the prefix diff options, and very few revision opts
> > store strings. Those that do (like --format and --encoding) already make
> > a copy of the string. They do not need for us to hold on to the memory
> > longer, but it does not hurt them if we do.
> 
> So the fix could've been as trivial as you mention above, where we
> simply perform a copy of the string for "--src-prefix", and everything
> else works just fine?

Well, and --dst-prefix, and also any other cases that get added later.
And keep in mind that --src-prefix and --dst-prefix are not even in the
revision code, but in the diff code. So we'd be creating a very subtle
requirement for somewhat far-away code to adhere to.

> In any case though, your approach is more defensive and makes it way
> harder for such use-after-free bugs to be introduced going forward, so
> I'm in line with the proposed patch.

Yeah, defensive is exactly what I was going for.

> > +static void mark_argv_for_free(struct rev_info *revs, const char *str)
> > +{
> > +	if (!str)
> > +		return;
> > +	strvec_push_nodup(&revs->argv_to_free, (char *)str);
> > +}
> 
> Hm. Doesn't this mean that we take ownership of the string and then
> eventually try to release it when releasing the vector? I wonder whether
> this could introduce subtle lifetime issues where the caller passes a
> non-heap-allocated string.

Yes, that's exactly the point. We are replacing a call to free() with
one that passes ownership to a strvec which later frees it. If somebody
is passing a non-heap string along with free_removed_argv_elements, then
everything was already broken.

> I don't think it's that bad when seeing where we use these. But I feel
> like hiding this fact by marking the parameter as `const` is a bit of a
> weird design choice. I'd much rather prefer we force this onto the
> callers so that they are aware of this, but I haven't seen the end
> result of that. So maybe it's just too ugly.

You can see the effect already in the diff. In the preimage all of the
callers had to cast away const-ness in order to pass the string to
free(). We could keep doing that here, but since this function has
exactly one purpose (to free the string we pass it) it seems like a nice
syntactic convenience to push the cast in here.

Though you may want to look at the "2/1" I sent, which pushes the check
for free_removed_argv_elements into this function. And then the cast and
that check are side-by-side.

-Peff