From: Jeff King Date: Fri, 29 Jul 2022 19:04:48 GMT Subject: Re: [PATCH 6/6] revisions API: don't leak memory on argv elements that need free()-ing Message-ID: In-Reply-To: On Fri, Jul 29, 2022 at 11:52:02AM -0700, Junio C Hamano wrote: > Jeff King 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