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

On Wed, Sep 13, 2017 at 10:53:57AM -0700, Jonathan Nieder wrote:

Jeff King wrote:

quoted

We ask to write 41 bytes and make sure that the return value
is at least 41. This is the same "dangerous" pattern that
was fixed in the prior commit (wherein a negative return
value is promoted to unsigned), though it is not dangerous
here because our "41" is a constant, not an unsigned
variable.

But we should convert it anyway to avoid modeling a
dangerous construct.

If the above logic is correct, then I suspect this series does not go
far enough.  write_in_full() would be one of those APIs that is easy
to misuse and difficult to use correctly, and if so we should fix that
at the source instead of trying to teach callers not to hold it wrong.

Yes, this series is just removing bad examples. It doesn't do anything
to make write_in_full() less potentially dangerous.

On the other hand, it's no more or less dangerous than write(), which
has the same return-value semantics.

E.g. what would you think of

 1. Introduce a write_fully (sorry, I am bad at names) function
    that returns 0 on success and a coccinelle semantic patch in
    contrib/coccinelle to migrate callers in "make coccicheck":

Yes, I considered that, though some callers really do care about
assigning the number of bytes written. The fact that write() has the
same problem, plus the fact that there were only 2 buggy instances
across the code base made me think there's not a huge gain to that extra
step.

@@
expression E;
expression F;
expression G;
@@
-write_in_full(E, F, G) < G
+write_fully(E, F, G)

 2. Run "make coccicheck" and apply the result.
 3. Remove the write_in_full function.

There's a step between those where you have to update all of the
write_in_full() callers that store the result. Some of them would be
trivial conversions, but some of them actually care about the length
E.g., the one in imap-send.c, which is the only one I didn't convert
away from "!= len" because it's half of an #ifdef with SSL_write()
(which uses an "int" as the return value!).

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.

-Peff