patch, 7 partsread/write_in_full leftovers

This series addresses the bits leftover from the discussion two weeks ago in:

  https://public-inbox.org/git/20170913170807.cyx7rrpoyhaauvol@sigill.intra.peff.net/

and its subthread. I don't think any of these is a real problem in practice, so this can be considered as a cleanup. I'm on the fence over whether the final one is a good idea. See the commit message for details.

  [1/7]: files-backend: prefer "0" for write_in_full() error check
  [2/7]: notes-merge: drop dead zero-write code
  [3/7]: read_in_full: reset errno before reading
  [4/7]: get-tar-commit-id: prefer "!=" for read_in_full() error check
  [5/7]: worktree: use xsize_t to access file size
  [6/7]: worktree: check the result of read_in_full()
  [7/7]: add xread_in_full() helper

 builtin/get-tar-commit-id.c |  5 +----
 builtin/worktree.c          | 11 ++++++++---
 bulk-checkin.c              |  4 +---
 cache.h                     |  7 +++++++
 combine-diff.c              |  8 +-------
 csum-file.c                 |  6 +-----
 notes-merge.c               |  2 --
 pack-write.c                |  3 +--
 refs/files-backend.c        |  2 +-
 wrapper.c                   | 12 ++++++++++++
 10 files changed, 33 insertions(+), 27 deletions(-)

-Peff

Commit 06f46f237a (avoid "write_in_full(fd, buf, len) != len" pattern, 2017-09-13) converted this callsite from:

  write_in_full(...) != 1

to

  write_in_full(...) < 0

But during the conflict resolution in c50424a6f0 (Merge branch 'jk/write-in-full-fix', 2017-09-25), this morphed into

  write_in_full(...) < 1

This behaves as we want, but we prefer to avoid modeling the "less than length" error-check which can be subtly buggy, as shown in efacf609c8 (config: avoid "write_in_full(fd, buf, len) < len" pattern, 2017-09-13).

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Jeff King wrote:

Good eyes.  Thanks.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

By the way, the description from that merge commit

    Many codepaths did not diagnose write failures correctly when disks
    go full, due to their misuse of write_in_full() helper function,
    which have been corrected.

does not look accurate to me. At least the "Many codepaths" part. All of those changes except for three should be no-ops. The scariest one is the 'long ret = write_in_full(fd, buf, size)' in notes-merge.c, which is about overflowing a "long" on Windows instead of error handling.

Thanks, Jonathan

On Mon, Sep 25, 2017 at 02:59:27PM -0700, Jonathan Nieder wrote:

Sort of. :) I usually continually rebase my topics until they end up empty. So I notice bits like this either during conflict resolution, or when the topic is left unexpectedly non-empty.

It's not foolproof, though. Sometimes rebasing a topic on itself is so painful that I "rebase --skip" liberally in the early parts, and would miss any merge funniness.

The ones in config.c are definitely wrong, too. Though I'd agree that "many" might be overstating it.

-Peff

We call write_in_full() with a size that we know is greater than zero. The return value can never be zero, then, since write_in_full() converts such a failed write() into ENOSPC and returns -1. We can just drop this branch of the error handling entirely.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 notes-merge.c | 2 --
 1 file changed, 2 deletions(-)

Jeff King wrote:

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for tying up this loose end.

Many callers of read_in_full() complain when we do not read their full byte-count. But a check like:

  if (read_in_full(fd, buf, len) != len)
	  return error_errno("unable to read");

conflates two problem conditions:

  1. A real error from read().

  2. There were fewer than "len" bytes available.

In the first case, showing the user strerror(errno) is useful. But in the second, we may see a random errno that was set by some previous system call.

In an ideal world, callers would always distinguish between these cases and give a useful message for each. But as an easy way to make our imperfect world better, let's reset errno to a known value. The best we can do is "0", which will yield something like:

  unable to read: Success

That's not great, but at least it's deterministic and makes it clear that we didn't see an error from read().

Signed-off-by: Jeff King <peff@peff.net>
---
 wrapper.c | 1 +
 1 file changed, 1 insertion(+)

Jeff King wrote:

Yuck.  Can we set errno to something more specific instead?

read(2) also doesn't promise not to clobber errno on success.

How about something like the following?

-- >8 --
Subject: read_in_full: set errno for partial reads

Many callers of read_in_full() complain when we do not read their full byte-count. But a check like:

  if (read_in_full(fd, buf, len) != len)
	return error_errno("unable to read");

conflates two problem conditions:

  1. A real error from read().
  2. There were fewer than "len" bytes available.

In the first case, showing the user strerror(errno) is useful. In the second, we may see a random errno that was set by some previous system call.

In an ideal world, callers would always distinguish between these cases and give a useful message for each. But as an easy way to make our imperfect world better, let's set errno in the second case to a known value. There is no standard "Unexpected end of file" errno value, so instead use EILSEQ, which yields a message like

  unable to read: Illegal byte sequence

That's not great, but at least it's deterministic and makes it possible to reverse-engineer what went wrong.

Change-Id: I48138052f71b7c6cf35c2d00a10dc8febaca4f10
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 wrapper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On Mon, Sep 25, 2017 at 03:09:14PM -0700, Jonathan Nieder wrote:

Yes, but what? You've suggested EILSEQ here, but that feels a bit...funny. I guess at least it's unlikely that read() would ever set errno to that itself, so we can be reasonably sure that seeing it is a sign of a short read. I thought ERANGE might be a possible candidate, but it doesn't quite fit. Ditto for ENODATA.

I _would_ like to have something meaningful, as it would mean the whole xread_in_full() nonsense from the final patch would be unnecessary. It would simply be reasonable to show the errno after read_in_full.

Another question is whether EILSEQ (or the others) is actually defined everywhere we compile. If it isn't, we'll have to define it ourselves (but to what? strerror() won't return anything useful for it).

Yes, though it's only a problem if you're using something other than 0.

Speaking of funny errno clobbering, with your patch:

If I do this:

  errno = 0;
  read_in_full(fd, buf, sizeof(buf));
  if (errno)
	die_errno("oops");

then we'll claim an error, even though there was none (remember that it's only an error for _some_ callers to not read the whole length).

This may be sufficiently odd that we don't need to care about it. There are some calls (like strtoul) which require this kind of clear-and-check strategy with errno, but in general we frown on it for calls like read().

We could also introduce a better helper like this:

  int read_exactly(int fd, void *buf, size_t count)
  {
	ssize_t ret = read_in_full(fd, buf, count);
	if (ret < 0)
		return -1;
	if (ret != count) {
		errno = EILSEQ;
		return -1;
	}
	return 0;
  }

Then we know that touching errno always coincides with an error return. And it's shorter to check "< 0" compared to "!= count" in the caller. But of course a caller which wants to distinguish the two cases for its error messages then has to look at errno:

  if (read_exactly(fd, buf, len) < 0) {
	if (errno == EILSEQ) /* eek, now this abstraction is leaky */
		die("short read");
	else
		die_errno("read error");
  }

I dunno. All options seem pretty gross to me.

-Peff

Jeff King wrote:

Correct. Actually more than "frown on": except for with the few calls like strtoul that are advertised to work this way, POSIX does not make the guarantee the above code would rely on, at all.

So it's not just frowned upon: it's so unportable that the standard calls it out as something that won't work.

That sounds nice, but it doesn't solve the original problem of callers using read_in_full that way.

Thanks, Jonathan

On Mon, Sep 25, 2017 at 04:33:16PM -0700, Jonathan Nieder wrote:

Is it unportable? Certainly read() is free reset errno to zero on success. But is it allowed to set it to another random value?

I think we're getting pretty academic here, but I'm curious if you have a good reference.

Right. None of the options discussed in this patch can do so. They can only take a caller which doesn't distinguish between the cases, and give it a deterministic value in errno for the short-read case.

IMHO as long as it _is_ deterministic and recognize as not an error from read(), that's the best we can do. Which is why I went with "0" in the first place. Seeing "read error: success" is a common-ish idiom (to me anyway) for "read didn't fail, but some user-space logic did", if only because it often happens accidentally.

-Peff

On Mon, Sep 25, 2017 at 07:37:32PM -0400, Jeff King wrote:

Answering my own question. POSIX says:

  No function in this volume of IEEE Std 1003.1-2001 shall set errno to
  0. The setting of errno after a successful call to a function is
  unspecified unless the description of that function specifies that
  errno shall not be modified.

So that does seem to outlaw errno-only checks for most functions.

It makes me wonder if the recent getdelim() fix is technically violating this. It should instead be explicitly checking for feof().

Another option I ran across from POSIX:

  [EOVERFLOW]
    The file is a regular file, nbyte is greater than 0, the starting
    position is before the end-of-file, and the starting position is
    greater than or equal to the offset maximum established in the open
    file description associated with fildes.

That's not _exactly_ what's going on here, but it's pretty close. And is what you would get if you implemented read_exactly() in terms of something like pread().

-Peff

Jeff King wrote:

All that really matters is the strerror() that the user would see.

For EOVERFLOW, that is

	Value too large to be stored in data type

For EILSEQ, it is

	Illegal byte sequence

Neither is perfect, but the latter seems like a better fit for the kind of file format errors we're describing here. Of course, it's even better if we fix the callers and don't try to wedge this into errno.

If you are okay with the too-long/too-short confusion in EOVERFLOW, then there is EMSGSIZE:

	Message too long

Hope that helps, Jonathan

On Mon, Sep 25, 2017 at 04:55:10PM -0700, Jonathan Nieder wrote:

Agreed, though I didn't think those were necessarily standardized.

Interestingly that does not seem to be a good description for the case POSIX describes above.

I find that one actively misleading. It's not the bytes we saw, it's the lack of them.

Yes. This patch is just a stop-gap. Perhaps we should abandon it entirely. But I couldn't find a way to fix all the callers. If you have a function that just returns "-1" when it sees a read_in_full() error, how does _its_ caller tell the difference?

I guess the answer is that the interface of the sub-function calling read_in_full() needs to change.

I don't really like that one either.

I actually prefer "0" of the all of the options discussed. At least it is immediately clear that it is not a syscall error.

-Peff

On Mon, Sep 25, 2017 at 5:01 PM, Jeff King <peff@peff.net> wrote:

After reading this discussion from the sideline, maybe

  ENODATA         No message is available on the STREAM head read
queue (POSIX.1)

is not too bad after all. Or

  ESPIPE          Invalid seek (POSIX.1)

Technically we do not seek, but one could imagine we'd seek there to read?

On Mon, Sep 25, 2017 at 05:06:58PM -0700, Stefan Beller wrote:

ENODATA is not too bad. On my glibc system it yields "No data available" from strerror(), which is at least comprehensible.

We're still left with the question of whether it is defined everywhere (and what to fallback to when it isn't).

-Peff

On Mon, Sep 25, 2017 at 08:09:13PM -0400, Jeff King wrote:

So,

#ifndef EUNDERFLOW #ifdef ENODATA #define ENODATA EUNDERFLOW #else #define ENODATA ESPIPE #endif #endif

? Windows has ESPIPE, and I'm not sure what other non-POSIX platform we need to worry about.

Thanks, Jonathan

Jonathan Nieder wrote:

Uh, pretend I said

#ifndef EUNDERFLOW # ifdef ENODATA # define EUNDERFLOW ENODATA # else # define EUNDERFLOW ESPIPE # endif #endif

Sorry for the nonsense. Jonathan

On Mon, Sep 25, 2017 at 05:17:32PM -0700, Jonathan Nieder wrote:

Right, I think our mails just crossed but I'm leaning in this direction. I'd prefer to call it SHORT_READ_ERRNO or something, though. Your "#ifndef EUNDERFLOW" had me thinking that this was something that a real platform might have, but AFAICT you just made it up.

-Peff

Jeff King wrote:

Agreed.  Two of the risks of replying too quickly. :)

Jonathan

Jeff King <peff@peff.net> writes:

Hmph, I may be slow (or may be skimming the exchanges too fast), but what exactly is wrong with "0"? As long as we do not have to tell two or more "not exactly an error from syscall in errno" cases, I would think "0" is the best value to use.

If the syserror message _is_ the issue, then we'd need to either pick an existing errno that is available everywhere (with possibly suboptimal message), or pick something and prepare a fallback for platforms that lack the errno, so picking "0" as the value and use whatever logic we would have used for the "fallback" would not sound too bad. I.e.

	if (read_in_full(..., size) != size)
		if (errno)
			die_errno("oops");
		else
			die("short read");

If the callsite were too numerous,

#define die_errno_or(msg1, msg2) if (errno) die_errno(msg1); else die(msg2)

perhaps?

On Tue, Sep 26, 2017 at 01:11:59PM +0900, Junio C Hamano wrote:

The main reason to avoid "0" is just that the "read error: Success" message is a bit funny.

You actually don't need errno for that. You can write:

  ret = read_in_full(..., size);
  if (ret < 0)
	die_errno("oops");
  else if (ret != size)
	die("short read");

So I think using errno as a sentinel value to tell between the two cases doesn't have much value.

If you just care about dying, you can wrap the whole read_in_full() in a helper (which is what my original patch 7 did). But I found there were cases that didn't want to die. Of course _most_ of those don't bother looking at errno in the first place. I think the only one that does is index_core(), which calls error_errno().

So with two helpers (one for die and one for error), I think we could cover the existing cases. Or we could convert the single error() case to just handle the logic inline.

One of the reasons I dislike the helper I showed earlier is that it involves assembling a lego sentence. But the alternative is requiring the caller to write both sentences from scratch (and writing a good sentence for the short read really is long: you expected X bytes but got Y).

I guess just tweaking the errno value does not really give a good "short read" error either. You get "unable to read foo: No data available", without the X/Y information.

Sometimes a custom "short read" message can be better, if it's "could not read enough bytes for packfile header" or similar. You don't need to know the exact number then.

So I dunno. Maybe I have just talked myself into actually just fixing all of the read_in_full() call sites manually.

-Peff

Jeff King <peff@peff.net> writes:

All sensible considerations. I probably should have read the entire discussion twice before starting to type X-<. Sorry about the noise.

That's true, too.  I dunno, either.

Jeff King wrote:

The standard allows them to vary for the sake of internationalization, but they are de facto constants (probably because of application authors like us abusing them ;-)).

[...]

Yes. :/

I have a basic aversion to ": Success" in error messages. Whenever I see such an error message, I stop trusting the program that produced it not to be seriously buggy. Maybe I'm the only one?

If no actual errno works, we could make a custom strerror-like function with our own custom errors (making them negative to avoid clashing with standard errno values), but this feels like overkill.

In the same spirit of misleadingly confusing too-long and too-short, there is also ERANGE ("Result too large"), which doesn't work here. There's also EPROTO "Protocol error", but that's about protocols, not file formats. More vague and therefor maybe more applicable is EBADMSG "Bad message". There's also ENOMSG "No message of the desired type".

If the goal is to support debugging, an error like EPIPE "Broken pipe" or ESPIPE "Invalid seek" would be likely to lead me in the right direction (wrong file size), even though it is misleading about how the error surfaced.

We could also avoid trying to be cute and use something generic like EIO "Input/output error".

Jonathan

On Mon, Sep 25, 2017 at 05:13:54PM -0700, Jonathan Nieder wrote:

That's a fair criticism, I think.

Yes, I also thought about that. It feels pretty invasive (I guess we'd wrap strerror() with our own custom function so that callers don't have to remember which one to use).

I definitely would prefer to avoid EIO, or anything that an actual read() might return.

What do you think of ENODATA? The glibc text for it is pretty appropriate. If it's not available everywhere, we'd have to fallback to something else (EIO? 0?). I don't know how esoteric it is. The likely candidate to be lacking it is Windows.

-Peff

Jeff King wrote:

ENODATA with a fallback to ESPIPE sounds fine to me.

read() would never produce ESPIPE because it doesn't seek.

So that would be:

/* errno value to use for early EOF */ #ifndef ENODATA #define ENODATA ESPIPE #endif

Thanks, Jonathan

On Mon, Sep 25, 2017 at 05:20:20PM -0700, Jonathan Nieder wrote:

Thanks, I'll re-roll with that (and thank you Stefan for mentioning ENODATA again; I came across it early in my search but discounted it as not quite right semantically. I should have been looking at the strerror() text, which is what really matters).

What do you think of patch 7 in light of this? If the short-read case gives us a sane errno, should we even bother trying to consistently handle its error separately?

-Peff

Jeff King wrote:

I like the read_exactly_or_die variant because it makes callers more concise, but on the other hand a caller handling the error can write a more meaningful error message with the right amount of context.

So I think you're right that it's better to drop patch 7.

Thanks, Jonathan

Jeff King wrote:

[...]

Yes, it is allowed to set it to another random value. It's a common thing for implementations to do when they hit a recoverable error, which they sometimes do (e.g. think EFAULT, EINTR, ETIMEDOUT, or an implementation calling strtoul and getting EINVAL or ERANGE).

glibc only recently started trying to avoid this kind of behavior, because even though the standard allows it, users hate it.

POSIX.1-2008 XSI 2.3 "Error Numbers" says

	Some functions provide the error number in a variable accessed
	through the symbol errno, defined by including the <errno.h>
	header.  The value of errno should only be examined when it is
	indicated to be valid by a function's return value.

See http://austingroupbugs.net/view.php?id=374 for an example where someone wanted to add a new guarantee of that kind to a function.

Hope that helps, Jonathan

Comparing the result of read_in_full() using less-than is potentially dangerous, as discussed in 561598cfcf (read_pack_header: handle signed/unsigned comparison in read result, 2017-09-13).

The instance in get-tar-commit-id is OK, because the HEADERSIZE macro expands to a signed integer. But if it were switched to an unsigned type like:

  size_t HEADERSIZE = ...;

this would be a bug. Let's use the more robust "!=" construct.

We can also drop the useless "n" variable while we're at it.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/get-tar-commit-id.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Jeff King wrote:

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

To read the "gitdir" file into memory, we stat the file and allocate a buffer. But we store the size in an "int", which may be truncated. We should use a size_t and xsize_t(), which will detect truncation.

An overflow is unlikely for a "gitdir" file, but it's a good practice to model.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/worktree.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Jeff King wrote:

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

We try to read "len" bytes into a buffer and just assume that it happened correctly. In practice this should usually be the case, since we just stat'd the file to get the length. But we could be fooled by transient errors or by other processes racily truncating the file.

Let's be more careful. There's a slim chance this could catch a real error, but it also prevents people and tools from getting worried while reading the code.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/worktree.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Jeff King wrote:

I'm a little confused. The 'if' condition checks for a read error but the message says something about 'stat'.

If we're trying to double-check the 'stat' result, shouldn't we read all the way to EOF in case the file got longer?

Puzzled, Jonathan

On Mon, Sep 25, 2017 at 03:14:26PM -0700, Jonathan Nieder wrote:

If you wanted to really double-check the stat result, yes you'd want to make sure there aren't extra bytes either. But really we're just making sure we were able to read _enough_ bytes.

To be honest, I'm tempted to rip out the whole thing and replace it with strbuf_read_file(), which seems more straightforward.

The function does seem to rely on the stat() to get the mtime, so we'd have to leave that part in.

-Peff

Many callers of read_in_full() expect to see exactly "len" bytes, and die if that isn't the case. Since there are two interesting error cases:

  1. We saw a read() error.

  2. We saw EOF with fewer bytes than expected.

we would ideally distinguish between them when reporting to the user. Let's add a helper to make this easier. We can convert cases that currently lump the two conditions together (improving their error messages) and ones that already distinguish (shortening their code).

There are a number of read_in_full() calls left that we can't (or shouldn't) convert, for one or more of these reasons:

 - Some are not trying to read an exact number in the first
   place. So they only care about case 1, and are fine.

 - Some call error() instead of die(). We could possibly
   accommodate these, at the expense of making our helper a
   bit more clunky.

 - Some print nothing and return a failing code up the
   stack, in which case somebody eventually looks at errno.
   Handling these with a helper would be hard. Ideally there
   would be some errno value that we could set for short
   read, but there's no standard one (ENODATA is kind-of
   accurate, but it's not really "no data"; it's "not enough
   data", and we wouldn't want to get confused with a real
   ENODATA error).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/get-tar-commit-id.c |  3 +--
 bulk-checkin.c              |  4 +---
 cache.h                     |  7 +++++++
 combine-diff.c              |  8 +-------
 csum-file.c                 |  6 +-----
 pack-write.c                |  3 +--
 wrapper.c                   | 11 +++++++++++
 7 files changed, 23 insertions(+), 19 deletions(-)

Jeff King wrote:

micronit: Can this be named read_in_full_or_die?

Otherwise it's too easy to mistake for a function like xread, which has different semantics.

I realize that xmalloc, xmemdupz, etc use a different convention. That's yet another reason to be explicit, IMHO.

Thanks, Jonathan

On Mon, Sep 25, 2017 at 03:16:08PM -0700, Jonathan Nieder wrote:

Yeah, we've definitely misused the "x" prefix for different things. I agree that being explicit probably can't hurt.

I wonder if it's worth calling it "read_exactly_or_die()" to emphasize that not reading enough bytes is a die-able offense.

-Peff

On Mon, Sep 25, 2017 at 04:26:47PM -0400, Jeff King wrote:

Here's a followup based on the discussion on v1. There are actually a few changes here.

I dropped the "read_in_full() should set errno on short reads" idea (3/7 in the earlier series). It really is the caller's fault for looking at errno when they know there hasn't been an error in the first place. We should just bite the bullet and have the callers do the right thing.

I also dropped the "xread_in_full" helper (7/7 earlier). The lego sentences it created just weren't worth the hassle. Instead, I've fixed all of the relevant callers to provide good error messages for both cases. It's a few more lines of code, and it's probably rare for users to see these in the first place. But it doesn't hurt too much to be thorough, and I think it's good to model correct error handling. This is in patches 4 and 5 below.

There are a handful of other changes:

  [1/7]: files-backend: prefer "0" for write_in_full() error check
  [2/7]: notes-merge: drop dead zero-write code

    These two are the same as before.

  [3/7]: prefer "!=" when checking read_in_full() result

    Along with the get-tar-commit-id case, I found two other cases which
    aren't bugs, but which should be updated for style reasons.

  [4/7]: avoid looking at errno for short read_in_full() returns
  [5/7]: distinguish error versus short_read from read_in_full()

    These two are new, and add in the messages. The ones in patch 4 are
    actively wrong (they show a bogus errno). The ones in 5 are just
    less descriptive than they could be. So if we find the extra lines
    of code too much, we could drop 5 (or even convert the ones in 4 to
    just stop looking at errno).

  [6/7]: worktree: use xsize_t to access file size

    Same as before.

  [7/7]: worktree: check the result of read_in_full()

    Similar, but now handles errors and short_reads separately. It also
    avoids leaking descriptors and memory in the error path (noticed by
    Coverity).

 builtin/get-tar-commit-id.c |  6 ++++--
 builtin/worktree.c          | 24 +++++++++++++++++++++---
 bulk-checkin.c              |  5 ++++-
 csum-file.c                 |  2 +-
 notes-merge.c               |  2 --
 pack-write.c                |  7 ++++++-
 packfile.c                  | 11 +++++++++--
 pkt-line.c                  |  2 +-
 refs/files-backend.c        |  2 +-
 sha1_file.c                 | 11 ++++++++---
 10 files changed, 55 insertions(+), 17 deletions(-)

-Peff

Commit 06f46f237a (avoid "write_in_full(fd, buf, len) != len" pattern, 2017-09-13) converted this callsite from:

  write_in_full(...) != 1

to

  write_in_full(...) < 0

But during the conflict resolution in c50424a6f0 (Merge branch 'jk/write-in-full-fix', 2017-09-25), this morphed into

  write_in_full(...) < 1

This behaves as we want, but we prefer to avoid modeling the "less than length" error-check which can be subtly buggy, as shown in efacf609c8 (config: avoid "write_in_full(fd, buf, len) < len" pattern, 2017-09-13).

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

We call write_in_full() with a size that we know is greater than zero. The return value can never be zero, then, since write_in_full() converts such a failed write() into ENOSPC and returns -1. We can just drop this branch of the error handling entirely.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 notes-merge.c | 2 --
 1 file changed, 2 deletions(-)

Comparing the result of read_in_full() using less-than is potentially dangerous, as a negative return value may be converted to an unsigned type and be considered a success. This is discussed further in 561598cfcf (read_pack_header: handle signed/unsigned comparison in read result, 2017-09-13).

Each of these instances is actually fine in practice:

 - in get-tar-commit-id, the HEADERSIZE macro expands to a
   signed integer. If it were switched to an unsigned type
   (e.g., a size_t), then it would be a bug.

 - the other two callers check for a short read only after
   handling a negative return separately. This is a fine
   practice, but we'd prefer to model "!=" as a general
   rule.

So all of these cases can be considered cleanups and not actual bugfixes.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/get-tar-commit-id.c | 2 +-
 csum-file.c                 | 2 +-
 pkt-line.c                  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Jeff King <peff@peff.net> writes:

I personally find that this "ret < count" that comes after checking if ret is negative expresses what it is checking in a more natural way than "ret must be exactly count".

It is not a big deal, as either one needs to be read with an understanding that read_in_full() would read at most "count" bytes to see if the right condition is being checked to declare "truncated" anyway. But I somehow find

	ret = read up to count
	if (ret < 0)
		read failed
	if (ret < count)
		we failed to read as much as expected

a bit more natural.

Likewise, even though it is harder to see that this follows another explicit check for "ret < 0".

Thanks.

On Wed, Sep 27, 2017 at 03:59:15PM +0900, Junio C Hamano wrote:

I actually do, too, and I wouldn't terribly mind to drop these. Mostly I didn't want people blindly copying only the second half. The fact that the second condition is OK only because the first condition is present is somewhat subtle.

I also wondered if this would shut up -Wsign-compare. But it doesn't seem to complain about the originals, which kind of surprises me.

-Peff

When a caller tries to read a particular set of bytes via read_in_full(), there are three possible outcomes:

  1. An error, in which case -1 is returned and errno is
     set.

  2. A short read, in which fewer bytes are returned and
     errno is unspecified (we never saw a read error, so we
     may have some random value from whatever syscall failed
     last).

  3. The full read completed successfully.

Many callers handle cases 1 and 2 together by just checking the result against the requested size. If their combined error path looks at errno (e.g., by calling die_errno), they may report a nonsense value.

Let's fix these sites by having them distinguish between the two error cases. That avoids the random errno confusion, and lets us give more detailed error messages.

Signed-off-by: Jeff King <peff@peff.net>
---
 pack-write.c |  7 ++++++-
 sha1_file.c  | 11 ++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

Jeff King <peff@peff.net> writes:

The resulting code might be more verbose but I personally think both of them give a lot more clear error indication.

Many callers of read_in_full() expect to see the exact number of bytes requested, but their error handling lumps together true read errors and short reads due to unexpected EOF.

We can give more specific error messages by separating these cases (showing errno when appropriate, and otherwise describing the short read).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/get-tar-commit-id.c |  4 +++-
 bulk-checkin.c              |  5 ++++-
 packfile.c                  | 11 +++++++++--
 3 files changed, 16 insertions(+), 4 deletions(-)

To read the "gitdir" file into memory, we stat the file and allocate a buffer. But we store the size in an "int", which may be truncated. We should use a size_t and xsize_t(), which will detect truncation.

An overflow is unlikely for a "gitdir" file, but it's a good practice to model.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/worktree.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

We try to read "len" bytes into a buffer and just assume that it happened correctly. In practice this should usually be the case, since we just stat'd the file to get the length. But we could be fooled by transient errors or by other processes racily truncating the file.

Let's be more careful. There's a slim chance this could catch a real error, but it also prevents people and tools from getting worried while reading the code.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/worktree.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Jeff King <peff@peff.net> writes:

Thanks for being thorough. My comment on 3/7 might be taken as contradicting with how 5/7 ties the loose ends up, but I do not care too deeply either way.

Will queue.