From: Jeff King <peff@peff.net>
Date: Fri, 19 Sep 2025 17:20:07 GMT
Subject: Re: [BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear)
Message-ID: <20250919172007.GA59895@coredump.intra.peff.net>
In-Reply-To: <xmqqldma5qha.fsf@gitster.g>

On Fri, Sep 19, 2025 at 09:58:25AM -0700, Junio C Hamano wrote:

> For completeness, here is how the other approach may look like, but
> I have made my share of off-by-one mistakes all over the place over
> the ears, so somebody else needs to lend an eyeball and check it for
> sanity.

I _thought_ there was an off-by-one at first, but I think what you have
is correct:

> +	/*
> +	 * NULL out the leftover args we did not understand, which has
> +	 * shallow copies in earlier slots in the array.
> +	 */
> +	while (left < argc--)
> +		argv[argc] = NULL;
>  	return left;

We definitely want argv[left] to be NULL, which I thought at first did
not happen because of the "<". But because it is post-increment that
happens in the loop condition, it works.

I probably would have written:

  while (left <= argc)
	argv[argc--] = NULL;

which I think is the same (but I didn't test it, so it probably does
have an off-by-one!).

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.

> diff --git c/t/t3903-stash.sh w/t/t3903-stash.sh
> index 0bb4648e36..dd70deb3b3 100755
> --- c/t/t3903-stash.sh
> +++ w/t/t3903-stash.sh
> @@ -69,6 +69,11 @@ test_expect_success 'stash some dirty working directory' '
>  	setup_stash
>  '
>  
> +test_expect_success 'controlled error return on unrecognized option' '
> +	test_expect_code 129 git stash show -p --no-such 2>usage &&
> +	grep -e "^usage: git stash show" usage
> +'

This passes now, but fails with SANITIZE=leak. Along with a bunch of
other tests, as we are now overwriting entries in strvecs with NULL, so
it has no opportunity to free them. We need to respect the
setup_revision_opt to free.

I'm working up a few alternative patches.

-Peff