From: Jeff King <peff@peff.net>
Date: Sun, 05 Apr 2026 06:56:02 GMT
Subject: Re: [PATCH v2 01/16] object-file: fix -Wsign-compare warnings
Message-ID: <20260405065602.GA1481314@coredump.intra.peff.net>
In-Reply-To: <20250717-pks-object-file-wo-the-repository-v2-1-36d2cd6c700e@pks.im>

On Thu, Jul 17, 2025 at 06:56:27AM +0200, Patrick Steinhardt wrote:

> @@ -1268,7 +1265,7 @@ int index_fd(struct index_state *istate, struct object_id *oid,
>  		ret = index_stream_convert_blob(istate, oid, fd, path, flags);
>  	else if (!S_ISREG(st->st_mode))
>  		ret = index_pipe(istate, oid, fd, type, path, flags);
> -	else if (st->st_size <= repo_settings_get_big_file_threshold(the_repository) ||
> +	else if ((st->st_size >= 0 && (size_t) st->st_size <= repo_settings_get_big_file_threshold(the_repository)) ||
>  		 type != OBJ_BLOB ||
>  		 (path && would_convert_to_git(istate, path)))
>  		ret = index_core(istate, oid, fd, xsize_t(st->st_size),

This is an old thread, but I happened across this code while looking at
the function for something more recent. This cast seems quite
suspicious. What happens when st->st_size is larger than a size_t (e.g.,
a 4.1GB file on a 32-bit system)? We'll truncate and get a wrong answer.

I think this ought to be comparing in the off_t space instead. Which I
_think_ was happening before the cast due to C's integer promotion rules
(even though off_t is signed, because it's larger than size_t on a
32-bit system and can hold all of size_t's values, then size_t is
promoted).

It's sort of academic, since both sides of the conditional end up
calling xsize_t(), which will then bail. But it _could_ handle off_t
correctly in the "else" clause here, which tries to stream the content.

More interesting to me (and why I responded to this old patch series) is
that inserting a cast to silence -Wsign-compare actually broke the
comparison in this case.

-Peff