Skip to content

Commit efacf60

Browse files
peffgitster
authored andcommitted
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. 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: # 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. Instead, it silently ignores the failure and renames config.lock into place, leaving you with a totally empty config file! Reported-by: demerphq <[email protected]> Signed-off-by: Jeff King <[email protected]> Reviewed-by: Jonathan Nieder <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 94c9fd2 commit efacf60

File tree

1 file changed

+2
-4
lines changed

1 file changed

+2
-4
lines changed

config.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2562,8 +2562,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
25622562
/* write the first part of the config */
25632563
if (copy_end > copy_begin) {
25642564
if (write_in_full(fd, contents + copy_begin,
2565-
copy_end - copy_begin) <
2566-
copy_end - copy_begin)
2565+
copy_end - copy_begin) < 0)
25672566
goto write_err_out;
25682567
if (new_line &&
25692568
write_str_in_full(fd, "\n") != 1)
@@ -2585,8 +2584,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
25852584
/* write the rest of the config */
25862585
if (copy_begin < contents_sz)
25872586
if (write_in_full(fd, contents + copy_begin,
2588-
contents_sz - copy_begin) <
2589-
contents_sz - copy_begin)
2587+
contents_sz - copy_begin) < 0)
25902588
goto write_err_out;
25912589

25922590
munmap(contents, contents_sz);

0 commit comments

Comments
 (0)