From efacf609c8f832be3e4ead634c054c8d52a00ad0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 13 Sep 2017 14:15:16 -0400 Subject: 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 Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- config.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'config.c') diff --git a/config.c b/config.c index acebc85d8..6219086a2 100644 --- a/config.c +++ b/config.c @@ -2562,8 +2562,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) @@ -2585,8 +2584,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); -- cgit v1.2.1 From 06f46f237afa823c0a2775e60ed8fbd80e7c751f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 13 Sep 2017 13:16:03 -0400 Subject: avoid "write_in_full(fd, buf, len) != len" pattern The return value of write_in_full() is either "-1", or the requested number of bytes[1]. If we make a partial write before seeing an error, we still return -1, not a partial value. This goes back to f6aa66cb95 (write_in_full: really write in full or return error on disk full., 2007-01-11). So checking anything except "was the return value negative" is pointless. And there are a couple of reasons not to do so: 1. It can do a funny signed/unsigned comparison. If your "len" is signed (e.g., a size_t) then the compiler will promote the "-1" to its unsigned variant. This works out for "!= len" (unless you really were trying to write the maximum size_t bytes), but is a bug if you check "< len" (an example of which was fixed recently in config.c). We should avoid promoting the mental model that you need to check the length at all, so that new sites are not tempted to copy us. 2. Checking for a negative value is shorter to type, especially when the length is an expression. 3. Linus says so. In d34cf19b89 (Clean up write_in_full() users, 2007-01-11), right after the write_in_full() semantics were changed, he wrote: I really wish every "write_in_full()" user would just check against "<0" now, but this fixes the nasty and stupid ones. Appeals to authority aside, this makes it clear that writing it this way does not have an intentional benefit. It's a historical curiosity that we never bothered to clean up (and which was undoubtedly cargo-culted into new sites). So let's convert these obviously-correct cases (this includes write_str_in_full(), which is just a wrapper for write_in_full()). [1] A careful reader may notice there is one way that write_in_full() can return a different value. If we ask write() to write N bytes and get a return value that is _larger_ than N, we could return a larger total. But besides the fact that this would imply a totally broken version of write(), it would already invoke undefined behavior. Our internal remaining counter is an unsigned size_t, which means that subtracting too many byte will wrap it around to a very large number. So we'll instantly begin reading off the end of the buffer, trying to write gigabytes (or petabytes) of data. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'config.c') diff --git a/config.c b/config.c index 6219086a2..90b699575 100644 --- a/config.c +++ b/config.c @@ -2565,7 +2565,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, copy_end - copy_begin) < 0) goto write_err_out; if (new_line && - write_str_in_full(fd, "\n") != 1) + write_str_in_full(fd, "\n") < 0) goto write_err_out; } copy_begin = store.offset[i]; @@ -2796,7 +2796,7 @@ int git_config_rename_section_in_file(const char *config_filename, if (remove) continue; length = strlen(output); - if (write_in_full(out_fd, output, length) != length) { + if (write_in_full(out_fd, output, length) < 0) { ret = write_error(get_lock_file_path(lock)); goto out; } -- cgit v1.2.1 From d9bd4cbb9cce9f872cc4427d1c27a62c6768b12a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 13 Sep 2017 13:17:57 -0400 Subject: config: flip return value of store_write_*() The store_write_section() and store_write_pairs() functions are basically high-level wrappers around write(). But their return values are flipped from our usual convention, using "1" for success and "0" for failure. Let's flip them to follow the usual write() conventions and update all callers. As these are local to config.c, it's unlikely that we'd have new callers in any topics in flight (which would be silently broken by our change). But just to be on the safe side, let's rename them to just write_section() and write_pairs(). That also accentuates their relationship with write(). Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- config.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) (limited to 'config.c') diff --git a/config.c b/config.c index 90b699575..d70fe7127 100644 --- a/config.c +++ b/config.c @@ -2251,10 +2251,11 @@ static int write_error(const char *filename) return 4; } -static int store_write_section(int fd, const char *key) +static ssize_t write_section(int fd, const char *key) { const char *dot; - int i, success; + int i; + ssize_t ret; struct strbuf sb = STRBUF_INIT; dot = memchr(key, '.', store.baselen); @@ -2270,15 +2271,16 @@ static int store_write_section(int fd, const char *key) strbuf_addf(&sb, "[%.*s]\n", store.baselen, key); } - success = write_in_full(fd, sb.buf, sb.len) == sb.len; + ret = write_in_full(fd, sb.buf, sb.len); strbuf_release(&sb); - return success; + return ret; } -static int store_write_pair(int fd, const char *key, const char *value) +static ssize_t write_pair(int fd, const char *key, const char *value) { - int i, success; + int i; + ssize_t ret; int length = strlen(key + store.baselen + 1); const char *quote = ""; struct strbuf sb = STRBUF_INIT; @@ -2318,10 +2320,10 @@ static int store_write_pair(int fd, const char *key, const char *value) } strbuf_addf(&sb, "%s\n", quote); - success = write_in_full(fd, sb.buf, sb.len) == sb.len; + ret = write_in_full(fd, sb.buf, sb.len); strbuf_release(&sb); - return success; + return ret; } static ssize_t find_beginning_of_line(const char *contents, size_t size, @@ -2451,8 +2453,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, } store.key = (char *)key; - if (!store_write_section(fd, key) || - !store_write_pair(fd, key, value)) + if (write_section(fd, key) < 0 || + write_pair(fd, key, value) < 0) goto write_err_out; } else { struct stat st; @@ -2574,10 +2576,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, /* write the pair (value == NULL means unset) */ if (value != NULL) { if (store.state == START) { - if (!store_write_section(fd, key)) + if (write_section(fd, key) < 0) goto write_err_out; } - if (!store_write_pair(fd, key, value)) + if (write_pair(fd, key, value) < 0) goto write_err_out; } @@ -2770,7 +2772,7 @@ int git_config_rename_section_in_file(const char *config_filename, continue; } store.baselen = strlen(new_name); - if (!store_write_section(out_fd, new_name)) { + if (write_section(out_fd, new_name) < 0) { ret = write_error(get_lock_file_path(lock)); goto out; } -- cgit v1.2.1