Re: [PATCH v5 4/4] builtin/stash: provide a way to import stashes from a ref

From: Jeff King <peff@peff.net>
Date: May 10, 2025, 21:33 UTC
Message-ID: <20250510213324.GB1139735@coredump.intra.peff.net>
In-Reply-To: <20250508234458.3665894-5-sandals@crustytoothpaste.net>

On Thu, May 08, 2025 at 11:44:57PM +0000, brian m. carlson wrote:

Show 31 quoted lines

> +static int do_import_stash(struct repository *r, const char *rev)
> [...]
> +	struct oid_array items = OID_ARRAY_INIT;
> +	int i;
> [...]
> +	/*
> +	 * Walk the commit history, finding each stash entry, and load data into
> +	 * the array.
> +	 */
> +	for (i = 0;; i++) {
> +		struct object_id tree, oid;
> +		char revision[GIT_MAX_HEXSZ + 1];
> +
> +		oid_to_hex_r(revision, &chain);
> +
> +		if (get_oidf(&tree, "%s:", revision) ||
> +		    !oideq(&tree, r->hash_algo->empty_tree)) {
> +			res = error(_("%s is not a valid exported stash commit"), revision);
> +			goto out;
> +		}
> +		if (get_oidf(&chain, "%s^1", revision) ||
> +		    get_oidf(&oid, "%s^2", revision))
> +			break;
> +		oid_array_append(&items, &oid);
> +	}
> +
> +	/*
> +	 * Now, walk each entry, adding it to the stash as a normal stash
> +	 * commit.
> +	 */
> +	for (i = items.nr - 1; i >= 0; i--) {

Coverity complains about possible integer overflow here. It's an interesting case. items.nr is a size_t, coming from the oid_array, and so it's unsigned. You use a signed int to iterate, which is needed to catch walking past the zero. But in that initial assignment, the subtraction of 1 is done on an unsigned value. If items.nr is zero, then it wraps around to a big (usually 64-bit) number, which is then truncated and forced into a signed 32-bit int.

I _think_ that usually works out, because the overflowed size_t is going to be all-bits-1, and then the truncation to int is also all-bits-1, which taken as a signed value is -1.

Probably there's some light violation of the standard there, but I think it should be OK. But I thought I'd mention it in case I'm missing something.

-Peff

PS Sorry for the flurry of emails on a v5; this hit jch, so it got
   sucked into my usual testing / analysis flow.