From efacf609c8f832be3e4ead634c054c8d52a00ad0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 13 Sep 2017 14:15:16 -0400 Subject: [PATCH 1/8] 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(-) diff --git a/config.c b/config.c index acebc85d81..6219086a2b 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); From 68a423ab3e5c160e1162382d2ef0831039b298d4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 13 Sep 2017 13:11:28 -0400 Subject: [PATCH 2/8] get-tar-commit-id: check write_in_full() return against 0 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. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/get-tar-commit-id.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c index e21c5416cd..6d9a79f9b3 100644 --- a/builtin/get-tar-commit-id.c +++ b/builtin/get-tar-commit-id.c @@ -33,8 +33,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix) if (!skip_prefix(content, "52 comment=", &comment)) return 1; - n = write_in_full(1, comment, 41); - if (n < 41) + if (write_in_full(1, comment, 41) < 0) die_errno("git get-tar-commit-id: write error"); return 0; From 06f46f237afa823c0a2775e60ed8fbd80e7c751f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 13 Sep 2017 13:16:03 -0400 Subject: [PATCH 3/8] 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 --- builtin/receive-pack.c | 2 +- builtin/rerere.c | 2 +- builtin/unpack-file.c | 2 +- config.c | 4 ++-- diff.c | 2 +- fast-import.c | 2 +- http-backend.c | 4 ++-- ll-merge.c | 2 +- read-cache.c | 6 +++--- refs.c | 2 +- refs/files-backend.c | 8 ++++---- rerere.c | 2 +- shallow.c | 6 +++--- t/helper/test-delta.c | 2 +- transport-helper.c | 5 ++--- wrapper.c | 2 +- 16 files changed, 26 insertions(+), 27 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index cabdc55e09..01dea59f58 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -742,7 +742,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, size_t n; if (feed(feed_state, &buf, &n)) break; - if (write_in_full(proc.in, buf, n) != n) + if (write_in_full(proc.in, buf, n) < 0) break; } close(proc.in); diff --git a/builtin/rerere.c b/builtin/rerere.c index ffb66e2907..0bc40298c2 100644 --- a/builtin/rerere.c +++ b/builtin/rerere.c @@ -18,7 +18,7 @@ static int outf(void *dummy, mmbuffer_t *ptr, int nbuf) { int i; for (i = 0; i < nbuf; i++) - if (write_in_full(1, ptr[i].ptr, ptr[i].size) != ptr[i].size) + if (write_in_full(1, ptr[i].ptr, ptr[i].size) < 0) return -1; return 0; } diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c index 73f1334191..672a54fcdf 100644 --- a/builtin/unpack-file.c +++ b/builtin/unpack-file.c @@ -15,7 +15,7 @@ static char *create_temp_file(unsigned char *sha1) xsnprintf(path, sizeof(path), ".merge_file_XXXXXX"); fd = xmkstemp(path); - if (write_in_full(fd, buf, size) != size) + if (write_in_full(fd, buf, size) < 0) die_errno("unable to write temp-file"); close(fd); return path; diff --git a/config.c b/config.c index 6219086a2b..90b699575b 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; } diff --git a/diff.c b/diff.c index 9c38258030..6f58bca2f9 100644 --- a/diff.c +++ b/diff.c @@ -2975,7 +2975,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp, blob = buf.buf; size = buf.len; } - if (write_in_full(fd, blob, size) != size) + if (write_in_full(fd, blob, size) < 0) die_errno("unable to write temp-file"); close_tempfile(&temp->tempfile); temp->name = get_tempfile_path(&temp->tempfile); diff --git a/fast-import.c b/fast-import.c index a959161b46..395524039b 100644 --- a/fast-import.c +++ b/fast-import.c @@ -2951,7 +2951,7 @@ static void parse_reset_branch(const char *arg) static void cat_blob_write(const char *buf, unsigned long size) { - if (write_in_full(cat_blob_fd, buf, size) != size) + if (write_in_full(cat_blob_fd, buf, size) < 0) die_errno("Write to frontend failed"); } diff --git a/http-backend.c b/http-backend.c index 519025d2c3..5daff8ccab 100644 --- a/http-backend.c +++ b/http-backend.c @@ -357,7 +357,7 @@ static void inflate_request(const char *prog_name, int out, int buffer_input) die("zlib error inflating request, result %d", ret); n = stream.total_out - cnt; - if (write_in_full(out, out_buf, n) != n) + if (write_in_full(out, out_buf, n) < 0) die("%s aborted reading request", prog_name); cnt += n; @@ -378,7 +378,7 @@ static void copy_request(const char *prog_name, int out) ssize_t n = read_request(0, &buf); if (n < 0) die_errno("error reading request body"); - if (write_in_full(out, buf, n) != n) + if (write_in_full(out, buf, n) < 0) die("%s aborted reading request", prog_name); close(out); free(buf); diff --git a/ll-merge.c b/ll-merge.c index 9fb855a900..a6ad2ec12d 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -154,7 +154,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len) xsnprintf(path, len, ".merge_file_XXXXXX"); fd = xmkstemp(path); - if (write_in_full(fd, src->ptr, src->size) != src->size) + if (write_in_full(fd, src->ptr, src->size) < 0) die_errno("unable to write temp-file"); close(fd); } diff --git a/read-cache.c b/read-cache.c index acfb028f48..5e6d24d444 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1921,7 +1921,7 @@ static int ce_write_flush(git_SHA_CTX *context, int fd) unsigned int buffered = write_buffer_len; if (buffered) { git_SHA1_Update(context, write_buffer, buffered); - if (write_in_full(fd, write_buffer, buffered) != buffered) + if (write_in_full(fd, write_buffer, buffered) < 0) return -1; write_buffer_len = 0; } @@ -1970,7 +1970,7 @@ static int ce_flush(git_SHA_CTX *context, int fd, unsigned char *sha1) /* Flush first if not enough space for SHA1 signature */ if (left + 20 > WRITE_BUFFER_SIZE) { - if (write_in_full(fd, write_buffer, left) != left) + if (write_in_full(fd, write_buffer, left) < 0) return -1; left = 0; } @@ -1979,7 +1979,7 @@ static int ce_flush(git_SHA_CTX *context, int fd, unsigned char *sha1) git_SHA1_Final(write_buffer + left, context); hashcpy(sha1, write_buffer + left); left += 20; - return (write_in_full(fd, write_buffer, left) != left) ? -1 : 0; + return (write_in_full(fd, write_buffer, left) < 0) ? -1 : 0; } static void ce_smudge_racily_clean_entry(struct cache_entry *ce) diff --git a/refs.c b/refs.c index ea2b9f84f8..a5abca04b7 100644 --- a/refs.c +++ b/refs.c @@ -592,7 +592,7 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1, } } - if (write_in_full(fd, buf.buf, buf.len) != buf.len) { + if (write_in_full(fd, buf.buf, buf.len) < 0) { strbuf_addf(err, "could not write to '%s'", filename); rollback_lock_file(&lock); goto done; diff --git a/refs/files-backend.c b/refs/files-backend.c index 0404f2c233..924e8537f3 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2118,8 +2118,8 @@ static int write_ref_to_lockfile(struct ref_lock *lock, return -1; } fd = get_lock_file_fd(lock->lk); - if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ || - write_in_full(fd, &term, 1) != 1 || + if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) < 0 || + write_in_full(fd, &term, 1) < 0 || close_ref(lock) < 0) { strbuf_addf(err, "couldn't write '%s'", get_lock_file_path(lock->lk)); @@ -3338,8 +3338,8 @@ static int files_reflog_expire(struct ref_store *ref_store, strerror(errno)); } else if (update && (write_in_full(get_lock_file_fd(lock->lk), - oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ || - write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 || + oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) < 0 || + write_str_in_full(get_lock_file_fd(lock->lk), "\n") < 0 || close_ref(lock) < 0)) { status |= error("couldn't write %s", get_lock_file_path(lock->lk)); diff --git a/rerere.c b/rerere.c index 70634d456c..51376cf6da 100644 --- a/rerere.c +++ b/rerere.c @@ -258,7 +258,7 @@ static int write_rr(struct string_list *rr, int out_fd) rerere_id_hex(id), rr->items[i].string, 0); - if (write_in_full(out_fd, buf.buf, buf.len) != buf.len) + if (write_in_full(out_fd, buf.buf, buf.len) < 0) die("unable to write rerere record"); strbuf_release(&buf); diff --git a/shallow.c b/shallow.c index 54359d5490..e8429a9a84 100644 --- a/shallow.c +++ b/shallow.c @@ -296,7 +296,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra) if (write_shallow_commits(&sb, 0, extra)) { fd = xmks_tempfile(&temporary_shallow, git_path("shallow_XXXXXX")); - if (write_in_full(fd, sb.buf, sb.len) != sb.len) + if (write_in_full(fd, sb.buf, sb.len) < 0) die_errno("failed to write to %s", get_tempfile_path(&temporary_shallow)); close_tempfile(&temporary_shallow); @@ -321,7 +321,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock, LOCK_DIE_ON_ERROR); check_shallow_file_for_update(); if (write_shallow_commits(&sb, 0, extra)) { - if (write_in_full(fd, sb.buf, sb.len) != sb.len) + if (write_in_full(fd, sb.buf, sb.len) < 0) die_errno("failed to write to %s", get_lock_file_path(shallow_lock)); *alternate_shallow_file = get_lock_file_path(shallow_lock); @@ -368,7 +368,7 @@ void prune_shallow(int show_only) LOCK_DIE_ON_ERROR); check_shallow_file_for_update(); if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) { - if (write_in_full(fd, sb.buf, sb.len) != sb.len) + if (write_in_full(fd, sb.buf, sb.len) < 0) die_errno("failed to write to %s", get_lock_file_path(&shallow_lock)); commit_lock_file(&shallow_lock); diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c index 59937dc1be..591730adc4 100644 --- a/t/helper/test-delta.c +++ b/t/helper/test-delta.c @@ -69,7 +69,7 @@ int cmd_main(int argc, const char **argv) } fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666); - if (fd < 0 || write_in_full(fd, out_buf, out_size) != out_size) { + if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) { perror(argv[4]); return 1; } diff --git a/transport-helper.c b/transport-helper.c index 33cff38cc0..2128a32f1c 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -44,8 +44,7 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer) { if (debug) fprintf(stderr, "Debug: Remote helper: -> %s", buffer->buf); - if (write_in_full(helper->helper->in, buffer->buf, buffer->len) - != buffer->len) + if (write_in_full(helper->helper->in, buffer->buf, buffer->len) < 0) die_errno("Full write to remote helper failed"); } @@ -74,7 +73,7 @@ static void write_constant(int fd, const char *str) { if (debug) fprintf(stderr, "Debug: Remote helper: -> %s", str); - if (write_in_full(fd, str, strlen(str)) != strlen(str)) + if (write_in_full(fd, str, strlen(str)) < 0) die_errno("Full write to remote helper failed"); } diff --git a/wrapper.c b/wrapper.c index 36630e5d18..61aba0b5c1 100644 --- a/wrapper.c +++ b/wrapper.c @@ -652,7 +652,7 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...) void write_file_buf(const char *path, const char *buf, size_t len) { int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666); - if (write_in_full(fd, buf, len) != len) + if (write_in_full(fd, buf, len) < 0) die_errno(_("could not write to %s"), path); if (close(fd)) die_errno(_("could not close %s"), path); From 564bde9ae69dc3d60e764078745275b637f90991 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 13 Sep 2017 13:16:28 -0400 Subject: [PATCH 4/8] convert less-trivial versions of "write_in_full() != len" The prior commit converted many sites to check the return value of write_in_full() for negativity, rather than a mismatch with the input length. This patch covers similar cases, but where the return value is stored in an intermediate variable. These should get the same treatment, but they need to be reviewed more carefully since it would be a bug if the return value is stored in an unsigned type (which indeed, it is in one of the cases). Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- entry.c | 5 +++-- refs/files-backend.c | 2 +- streaming.c | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/entry.c b/entry.c index 65458f07a4..3c1818f1d4 100644 --- a/entry.c +++ b/entry.c @@ -244,7 +244,8 @@ static int write_entry(struct cache_entry *ce, char *new; struct strbuf buf = STRBUF_INIT; unsigned long size; - size_t wrote, newsize = 0; + ssize_t wrote; + size_t newsize = 0; struct stat st; const struct submodule *sub; @@ -319,7 +320,7 @@ static int write_entry(struct cache_entry *ce, fstat_done = fstat_output(fd, state, &st); close(fd); free(new); - if (wrote != size) + if (wrote < 0) return error("unable to write file %s", path); break; case S_IFGITLINK: diff --git a/refs/files-backend.c b/refs/files-backend.c index 924e8537f3..f21a954ce7 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2039,7 +2039,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid, written = len <= maxlen ? write_in_full(fd, logrec, len) : -1; free(logrec); - if (written != len) + if (written < 0) return -1; return 0; diff --git a/streaming.c b/streaming.c index 9afa66b8be..c8b85e4498 100644 --- a/streaming.c +++ b/streaming.c @@ -539,7 +539,7 @@ int stream_blob_to_fd(int fd, const struct object_id *oid, struct stream_filter kept = 0; wrote = write_in_full(fd, buf, readlen); - if (wrote != readlen) + if (wrote < 0) goto close_and_exit; } if (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 || From 4c95e3dd28342878e0deed264ddc784b775361b7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 13 Sep 2017 13:17:30 -0400 Subject: [PATCH 5/8] pkt-line: check write_in_full() errors against "< 0" As with the previous two commits, we prefer to check write_in_full()'s return value to see if it is negative, rather than comparing it to the input length. These cases actually flip the logic to check for success, making conversion a little different than in other cases. We could of course write: if (write_in_full(...) >= 0) return 0; return error(...); But our usual method of spelling write() error checks is just "< 0". So let's flip the logic for each of these conditionals to our usual style. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- pkt-line.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/pkt-line.c b/pkt-line.c index 7db9119573..4823d3bb9d 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -94,9 +94,9 @@ void packet_flush(int fd) int packet_flush_gently(int fd) { packet_trace("0000", 4, 1); - if (write_in_full(fd, "0000", 4) == 4) - return 0; - return error("flush packet write failed"); + if (write_in_full(fd, "0000", 4) < 0) + return error("flush packet write failed"); + return 0; } void packet_buf_flush(struct strbuf *buf) @@ -137,18 +137,17 @@ static int packet_write_fmt_1(int fd, int gently, const char *fmt, va_list args) { struct strbuf buf = STRBUF_INIT; - ssize_t count; format_packet(&buf, fmt, args); - count = write_in_full(fd, buf.buf, buf.len); - if (count == buf.len) - return 0; - - if (!gently) { - check_pipe(errno); - die_errno("packet write with format failed"); + if (write_in_full(fd, buf.buf, buf.len) < 0) { + if (!gently) { + check_pipe(errno); + die_errno("packet write with format failed"); + } + return error("packet write with format failed"); } - return error("packet write with format failed"); + + return 0; } void packet_write_fmt(int fd, const char *fmt, ...) @@ -183,9 +182,9 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size) packet_size = size + 4; set_packet_header(packet_write_buffer, packet_size); memcpy(packet_write_buffer + 4, buf, size); - if (write_in_full(fd_out, packet_write_buffer, packet_size) == packet_size) - return 0; - return error("packet write failed"); + if (write_in_full(fd_out, packet_write_buffer, packet_size) < 0) + return error("packet write failed"); + return 0; } void packet_buf_write(struct strbuf *buf, const char *fmt, ...) From 634eb82b1aa142e2743acd836447002fd9ebfa10 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 13 Sep 2017 13:17:44 -0400 Subject: [PATCH 6/8] notes-merge: use ssize_t for write_in_full() return value We store the return value of write_in_full() in a long, though the return is actually an ssize_t. This probably doesn't matter much in practice (since the buffer size is alredy an unsigned long), but it might if the size if between what can be represented in "long" and "unsigned long", and if your size_t is larger than a "long" (as it is on 64-bit Windows). Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- notes-merge.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notes-merge.c b/notes-merge.c index c12b354f10..01cecbdda3 100644 --- a/notes-merge.c +++ b/notes-merge.c @@ -302,7 +302,7 @@ static void write_buf_to_worktree(const struct object_id *obj, fd = xopen(path, O_WRONLY | O_EXCL | O_CREAT, 0666); while (size > 0) { - long ret = write_in_full(fd, buf, size); + ssize_t ret = write_in_full(fd, buf, size); if (ret < 0) { /* Ignore epipe */ if (errno == EPIPE) From d9bd4cbb9cce9f872cc4427d1c27a62c6768b12a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 13 Sep 2017 13:17:57 -0400 Subject: [PATCH 7/8] 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(-) diff --git a/config.c b/config.c index 90b699575b..d70fe71273 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; } From f48ecd38cb86b86f01914e875d74c92c077bf493 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 13 Sep 2017 14:47:22 -0400 Subject: [PATCH 8/8] read_pack_header: handle signed/unsigned comparison in read result The result of read_in_full() may be -1 if we saw an error. But in comparing it to a sizeof() result, that "-1" will be promoted to size_t. In fact, the largest possible size_t which is much bigger than our struct size. This means that our "< sizeof(header)" error check won't trigger. In practice, we'd go on to read uninitialized memory and compare it to the PACK signature, which is likely to fail. But we shouldn't get there. We can fix this by making a direct "!=" comparison to the requested size, rather than "<". This means that errors get lumped in with short reads, but that's sufficient for our purposes here. There's no PH_ERROR tp represent our case. And anyway, this function reads from pipes and network sockets. A network error may racily appear as EOF to us anyway if there's data left in the socket buffers. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- sha1_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sha1_file.c b/sha1_file.c index 4fa4b185f3..20a9d39c00 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -3722,7 +3722,7 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, unsigned int read_pack_header(int fd, struct pack_header *header) { - if (read_in_full(fd, header, sizeof(*header)) < sizeof(*header)) + if (read_in_full(fd, header, sizeof(*header)) != sizeof(*header)) /* "eof before pack header was fully read" */ return PH_ERROR_EOF;