Re: [PATCH v2 1/1] zlib.c: use size_t for size

From: Jeff King <peff@peff.net>
Date: Oct 14, 2018, 02:52 UTC
Message-ID: <20181014025207.GA1527@sigill.intra.peff.net>
In-Reply-To: <fb6367b3-975b-1bc6-e95b-a457fb9e3a33@ramsayjones.plus.com>

On Sun, Oct 14, 2018 at 03:16:36AM +0100, Ramsay Jones wrote:

Show 16 quoted lines

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index b059b86aee..3b5f2c38b3 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -269,12 +269,12 @@ static void copy_pack_data(struct hashfile *f,
>  		off_t len)
>  {
>  	unsigned char *in;
> -	unsigned long avail;
> +	size_t avail;
>  
>  	while (len) {
>  		in = use_pack(p, w_curs, offset, &avail);
>  		if (avail > len)
> -			avail = (unsigned long)len;
> +			avail = xsize_t(len);

We don't actually care about truncation here. The idea is that we take a bite-sized chunk via use_pack, and loop as necessary. So mod-2^32 truncation via a cast would be bad (we might not make forward progress), but truncating to SIZE_MAX would be fine.

That said, we know that would not truncate here, because we must strictly be shrinking "avail", which was already a size_t (unless "len" is negative, but then we are really screwed ;) ).

So I kind of wonder if a comment would be better than xsize_t here. Something like:

  if (avail > len) {
	/*
	 * This can never truncate because we know that len is smaller
	 * than avail, which is already a size_t.
	 */
	avail = (size_t)len;
  }

-Peff