From: Junio C Hamano Date: Fri, 19 Sep 2025 17:13:45 GMT Subject: Re: [BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear) Message-ID: In-Reply-To: <20250919164820.GA24939@coredump.intra.peff.net> Jeff King writes: > I think we'll have leaked the string holding "-p" in this instance, > though. We probably need to pass in a setup_revision_opt struct with its > free_removed_argv_elements flag set. > > That's true even without your patch, too, of course. Yeah, while I was preparing the "alternative", I noticed that option being paid attention to by the code, but you are right. Anybody who passes strvec (which owns its contents, unlike the traditional "we got this argv[] from the operating system" callers) needs to flip that bit set, or they would leak. > I'm mildly > surprised that the test suite doesn't hit this in leak-checking mode, > since it is a problem any time we rearrange argv. E.g., I think: > > git stash show -p -- > > leaks (I was surprised that "stash show -p --stat" didn't leak, but it > doesn't seem to rearrange?). Yeah. > I wonder if the best solution is a setup_revisions() wrapper for strvecs > that will: > > - turn on the free_removed_argv_elements option automatically > > - collect the return value of setup_revisions() and use it to fix > the .nr field of the strvec > > - restore the NULL invariant at the end of the array (though I would > also be happy if setup_revisions() just did this itself) That would be nice. I only did the third one in my "alternative" patch I sent earlier.