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 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 notice an 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. This case is actually quite interesting:
we don't have a size_t, but rather use the subtraction of
two pointers. Which you might think would be a signed
ptrdiff_t, but clearly both gcc and clang treat it as
unsigned (possibly because the conditional just above
guarantees that the result is greater than zero).
We can avoid the whole question by just checking for a
negative return value directly, as write_in_full() will
never return any value except -1 or the full count.
There's no addition to the test suite here, since you need
to convince write() to fail in order to see the problem. The
simplest reproduction recipe I came up with is to trigger
ENOSPC (this only works on Linux, obviously):
# make a limited-size filesystem
dd if=/dev/zero of=small.disk bs=1M count=1
mke2fs small.disk
mkdir mnt
sudo mount -o loop small.disk mnt
cd mnt
sudo chown $USER:$USER .
# make a config file with some content
git config --file=config one.key value
git config --file=config two.key value
# now fill up the disk
dd if=/dev/zero of=fill
# and try to delete a key, which requires copying the rest
# of the file to config.lock, and will fail on write()
git config --file=config --unset two.key
That final command should (and does after this patch)
produce an error message due to the failed write, and leave
the file intact. With the current code it silently ignores
the failure and renames config.lock into place, leaving you
with a totally empty config file!
Reported-by: demerphq <demerphq@xxxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
config.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/config.c b/config.c
index d0d8ce823a..eee4ac0355 100644
--- a/config.c
+++ b/config.c
@@ -2608,8 +2608,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
/* write the first part of the config */
if (copy_end > copy_begin) {
if (write_in_full(fd, contents + copy_begin,
- copy_end - copy_begin) <
- copy_end - copy_begin)
+ copy_end - copy_begin) < 0)
goto write_err_out;
if (new_line &&
write_str_in_full(fd, "\n") != 1)
@@ -2631,8 +2630,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
/* write the rest of the config */
if (copy_begin < contents_sz)
if (write_in_full(fd, contents + copy_begin,
- contents_sz - copy_begin) <
- contents_sz - copy_begin)
+ contents_sz - copy_begin) < 0)
goto write_err_out;
munmap(contents, contents_sz);
--
2.14.1.874.ge7b2e05270
