Re: [PATCH v3 07/15] sign-compare: 32-bit support

From: Jeff King <peff@peff.net>
Date: Dec 5, 2024, 19:34 UTC
Message-ID: <20241205193439.GC2629822@coredump.intra.peff.net>
In-Reply-To: <20241205-pks-sign-compare-v3-7-e211ee089717@pks.im>

On Thu, Dec 05, 2024 at 10:36:29AM +0100, Patrick Steinhardt wrote:

Show 7 quoted lines

> @@ -24,7 +23,7 @@ static void verify_buffer_or_die(struct hashfile *f,
>  
>  	if (ret < 0)
>  		die_errno("%s: sha1 file read error", f->name);
> -	if (ret != count)
> +	if ((size_t)ret != (size_t)count)
>  		die("%s: sha1 file truncated", f->name);

You really only need the cast on the left-hand side here. "count" is already an unsigned value (and will get promoted as necessary on a system where "unsigned int" is smaller than "size_t").

It's probably not hurting too much, but my philosophy is that we should do as few casts as strictly necessary. Casts are a blunt instrument for telling the compiler we know what we are doing, and can cover up issues (in this case a false positive, but imagine "count" was switched to "int").

IMHO "count" should probably be a size_t here anyway, since we are dealing with a buffer size. If you look at the call stack, it is based on hashfile.buffer, which we'd expect to be small. But it is initialized from a size_t, so really it is one errant hashfd_internal() from being a truncation bug. That's not a mistake I expect to be likely, but I think we are better off in general making code as obviously/trivially correct as possible.

I think truncation is getting out of scope for your series, though, so probably not worth doing right at this moment.

Show 12 quoted lines

> diff --git a/pkt-line.c b/pkt-line.c
> index 90ea2b6974b1d0957cfdc5e2f9a2c30720723f12..f48b558ad23dd99f334d2d60e954ce9a83ac6114 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -363,7 +363,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,
>  	}
>  
>  	/* And complain if we didn't get enough bytes to satisfy the read. */
> -	if (ret != size) {
> +	if ((size_t)ret != (size_t)size) {
>  		if (options & PACKET_READ_GENTLE_ON_EOF)
>  			return -1;

Likewise here, "size" is already unsigned.

I also wondered if there was a safer solution than a bare cast here. Both of these are OK because the lines immediately before them checked for the negative value, but there's nothing at the compiler level to enforce that.

I guess a solution that uses the type system would be akin to Option from Rust, et al. A helper that checks for negative values and also promotes to an unsigned type, like:

  ssize_t ret = read_in_full(fd, buf, count);
  size_t bytes_read;

  if (!signed_to_unsigned(ret, &bytes_read))
	die_errno(...); /* error */
  if (bytes_read != count)
        ...

I don't know if there's a more ergonomic way that ditches the extra variable. Or if there are enough cases like this to merit having a helper.

-Peff