Re: [PATCH 2/7] get-tar-commit-id: check write_in_full() return against 0

From: Jeff King <peff@peff.net>
Date: Sep 13, 2017, 18:37 UTC
Message-ID: <20170913183700.amtrquhg66hjrbsp@sigill.intra.peff.net>
In-Reply-To: <20170913180232.luxcnx7fbcpbt7bw@sigill.intra.peff.net>

On Wed, Sep 13, 2017 at 02:02:32PM -0400, Jeff King wrote:

Show 13 quoted lines

> > Does read_in_full need a similar treatment?
> 
> It might actually return fewer than the requested number of bytes, so it
> can't just use "< 0" in the same way (nor be adapted to return 0 on
> success).  But I think it's still a bug to do:
> 
>   char buf[20];
>   size_t len = sizeof(buf);
>   if (read_in_full(fd, buf, len) < len)
>           die(...);
> 
> since that will promote the -1 to a size_t. So it's probably worth
> auditing.

I looked it over and found one potentially buggy case. In read-pack_header(), we do:

          if (read_in_full(fd, header, sizeof(*header)) < sizeof(*header))
                  /* "eof before pack header was fully read" */
                  return PH_ERROR_EOF;

which will treat a read error as a silent success. I don't think it's harmful in practice because the next line checks that the header matches the PACK signature (which yes, means we're reading uninitialized bytes, but the chances are 1 in 2^32 that they match the signature. Unless the buffer had an old PACK signature in it, of course ;) ).

There's one other harmless "< len" check. There are a bunch of "!=" checks which are OK as-is. Converting them to something equally short is hard, though we could introduce a helper similar to your write_fully(), like:

  int read_exactly(int fd, char *buf, size_t len)
  {
	ssize_t ret = read_in_full(fd, buf, len);
	if (ret < 0)
		return -1; /* real error */
	else if (ret < len)
		return -1; /* early eof */
	return 0;
  }

But the trouble is that a _good_ caller actually handles those cases separately to provide better error messages (by doing that same if-cascade themselves). If those if's were turned into error() or die() calls, then we'd actually be improving the callsites. But of course not all of them actually print an error or die. And when they do, they usually say something specific about _what_ they were trying to read.

So it may not be worth trying to do a mass-cleanup in the same way here.

-Peff