On Thu, Dec 05, 2024 at 10:36:29AM +0100, Patrick Steinhardt wrote:
> @@ -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.
> 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
