Re: [PATCH v2] config: avoid "write_in_full(fd, buf, len) < len" pattern

Jeff King wrote:

What I missed is that copy_begin and copy_end here are actually size_t
variables, not the pointers. Sorry for the confusion, and here's an
updated version of the patch with this paragraph amended (the patch
itself is identical):

Subtle.  The world makes more sense now.  Thanks for figuring it out.

-- >8 --
Subject: [PATCH] config: avoid "write_in_full(fd, buf, len) < len" pattern

The return type of write_in_full() is a signed ssize_t,
because we may return "-1" on failure (even if we succeeded
in writing some bytes). But "len" itself is may be an
unsigned type (the function takes a size_t, but of course we
may have something else in the calling function). So while
it seems like:

  if (write_in_full(fd, buf, len) < len)
	die_errno("write error");

would trigger on error, it won't if "len" is unsigned.  The
compiler sees a signed/unsigned comparison and promotes the
signed value, resulting in (size_t)-1, the highest possible
size_t (or again, whatever type the caller has). This cannot
possibly be smaller than "len", and so the conditional can
never trigger.

I scoured the code base for cases of this, but it turns out
that these two in git_config_set_multivar_in_file_gently()
are the only ones. Here our "len" is the difference between
two size_t variables, making the result an unsigned size_t.
We can fix this by just checking for a negative return value
directly, as write_in_full() will never return any value
except -1 or the full count.

[...]

Reported-by: demerphq <redacted>
Signed-off-by: Jeff King <redacted>
---

For what it's worth,
Reviewed-by: Jonathan Nieder <redacted>

Thank you.

Compilers' signed/unsigned comparison warning can be noisy, but I'm
starting to feel it's worth the suppression noise to turn it on when
DEVELOPER=1 anyway.  What do you think?  Is there a way to turn it on
selectively for certain functions on the LHS (like read() and write()
style functions)?

Sincerely,
Jonathan