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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 13, 2017, 19:11 UTC
Message-ID: <20170913191102.GF27425@aiede.mtv.corp.google.com>
In-Reply-To: <20170913184722.imcexldnufskugqz@sigill.intra.peff.net>

Jeff King wrote:

Show 24 quoted lines

> 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 <peff@peff.net>
> ---
>  sha1_file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This patch 8 (but not patches 2-7) is
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

Show 11 quoted lines

> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1850,7 +1850,7 @@ int index_path(struct object_id *oid, const char *path, struct stat *st, unsigne
>  
>  int read_pack_header(int fd, struct pack_header *header)
>  {
> -	if (read_in_full(fd, header, sizeof(*header)) < sizeof(*header))
> +	if (read_in_full(fd, header, sizeof(*header)) != sizeof(*header))
>  		/* "eof before pack header was fully read" */
>  		return PH_ERROR_EOF;
>