Re: [PATCH 8/7] read_pack_header: handle signed/unsigned comparison in read result

Jeff King wrote:

Subject: [PATCH] read_pack_header: handle signed/unsigned comparison in read  result

The result of read_in_full() may be -1 if we saw an error.
But in comparing it to a sizeof() result, that "-1" will be
promoted to size_t. In fact, the largest possible size_t
which is much bigger than our struct size. This means that
our "< sizeof(header)" error check won't trigger.

In practice, we'd go on to read uninitialized memory and
compare it to the PACK signature, which is likely to fail.
But we shouldn't get there.

We can fix this by making a direct "!=" comparison to the
requested size, rather than "<". This means that errors get
lumped in with short reads, but that's sufficient for our
purposes here. There's no PH_ERROR tp represent our case.
And anyway, this function reads from pipes and network
sockets. A network error may racily appear as EOF to us
anyway if there's data left in the socket buffers.

Signed-off-by: Jeff King <redacted>
---
 sha1_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

This patch 8 (but not patches 2-7) is
Reviewed-by: Jonathan Nieder <redacted>

Thanks.

An alternative would be to use something like

	< (int) sizeof(*header)

to force it to be signed, but I think this is clearer.

Using the following semantic patch, I find this and the example from
patch 1 and no others:

  @@
  expression fd;
  expression buf;
  expression len;
  size_t rhs;
  @@
  -read_in_full(fd, buf, len) < rhs
  +ERROR()

  @@
  expression fd;
  expression buf;
  expression len;
  size_t rhs;
  @@
  -write_in_full(fd, buf, len) < rhs
  +ERROR()

Thanks,
Jonathan