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 29, 2022, 19:04 UTC
Message-ID: <YuQvUDlGPTHu2sOm@coredump.intra.peff.net>
In-Reply-To: <xmqqk07v69bh.fsf@gitster.g>

On Fri, Jul 29, 2022 at 11:52:02AM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > The more interesting question is whether it causes any use-after-free
> > bugs.
> 
> Thanks for mentioning this.  All the "plug more leaks" patches make
> me worried for exactly that.  Another is a potential subtle breakage
> hidden by use of FREE_AND_NULL() and friends, which the sanitizers
> would probably not see, but can appear as behaviour change.

Yeah, agreed that is a potential pitfall.

> > I don't think it does, and certainly SANITIZE=address agrees.
> 
> ;-)

Just to expound a bit: I'm quite sure _this_ call site is fine, because we are passing pretty vanilla options to setup_revisions(), and I looked at how we handle the strings there. I'd worry a little more that there's some dark corner of setup_revisions() which relies on memory lifetimes, and so this isn't a safe thing to do in the general case.

But IMHO we should assume that setup_revisions() makes copies of strings it needs, and if there is a dark corner where it doesn't, we should find and fix that.

-Peff