On Thu, May 08, 2025 at 11:44:57PM +0000, brian m. carlson wrote:
> +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.
