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

From: Jeff King <peff@peff.net>
Date: Oct 13, 2018, 02:46 UTC
Message-ID: <20181013024624.GB15595@sigill.intra.peff.net>
In-Reply-To: <20181013023845.GA15595@sigill.intra.peff.net>

On Fri, Oct 12, 2018 at 10:38:45PM -0400, Jeff King wrote:

Show 7 quoted lines

> So right now let's imagine that off_t is 64-bit, and "unsigned long" is
> 32-bit (e.g., 32-bit system, or an IL32P64 model like Windows). We'll
> repeatedly ask use_pack() for a window, and it will tell us how many
> bytes we have in "avail". So even as a 32-bit value, that just means
> we'll process chunks smaller than 4GB, and this is correct (or at least
> this part of it -- hold on). But we can still process the whole "len"
> given by the off_t eventually.

So this "hold on" was because I thought I had found another bug in use_pack(), but I actually think it's OK.

In use_pack(), we do this:

          if (left)
                  *left = win->len - xsize_t(offset);

where win->len is a size_t. Before this patch, "left" is a pointer to unsigned long. So that has the usual broken-on-Windows mismatch. This patch makes it a size_t, which is good.

But what's up with that xsize_t(offset)? We'll complain about truncation _before_ we do any offset subtraction. So at first glance, I thought this meant we were broken for larger-than-4GB packs on 32-bit systems when trying to read past the 4GB mark.

But no, right before that we have this line:

          offset -= win->offset;

So offset is in fact no longer its original meaning of "offset into the packfile", but is now an offset of the specific request into the window we found.

So I think it's correct, but it sure confused me. I wonder if another variable might help, like:

  size_t offset_in_window;
  ...

  /*
   * We know this difference will fit in a size_t, because our mmap
   * window by * definition can be no larger than a size_t.
   */
  offset_in_window = xsize_t(offset - win->offset);
  if (left)
	*left = win->len - offset_in_window;
  return win->base + offset_in_window;

I dunno. Maybe it is overkill.

-Peff