From: Junio C Hamano Date: Mon, 23 Oct 2017 05:37:21 +0000 (+0900) Subject: Merge branch 'jk/write-in-full-fix' into maint X-Git-Tag: v2.14.3~3 X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/96c6bb566ee5354a1b07530a94d3f85055e46032?ds=inline;hp=-c Merge branch 'jk/write-in-full-fix' into maint Many codepaths did not diagnose write failures correctly when disks go full, due to their misuse of write_in_full() helper function, which have been corrected. * jk/write-in-full-fix: read_pack_header: handle signed/unsigned comparison in read result config: flip return value of store_write_*() notes-merge: use ssize_t for write_in_full() return value pkt-line: check write_in_full() errors against "< 0" convert less-trivial versions of "write_in_full() != len" avoid "write_in_full(fd, buf, len) != len" pattern get-tar-commit-id: check write_in_full() return against 0 config: avoid "write_in_full(fd, buf, len) < len" pattern --- 96c6bb566ee5354a1b07530a94d3f85055e46032 diff --combined config.c index 1a8e24f32c,d70fe71273..17e1349370 --- a/config.c +++ b/config.c @@@ -16,6 -16,7 +16,6 @@@ #include "string-list.h" #include "utf8.h" #include "dir.h" -#include "color.h" struct config_source { struct config_source *prev; @@@ -1350,6 -1351,9 +1350,6 @@@ int git_default_config(const char *var if (starts_with(var, "advice.")) return git_default_advice_config(var, value); - if (git_color_config(var, value, dummy) < 0) - return -1; - if (!strcmp(var, "pager.color") || !strcmp(var, "color.pager")) { pager_use_color = git_config_bool(var,value); return 0; @@@ -2155,7 -2159,7 +2155,7 @@@ static struct size_t *offset; unsigned int offset_alloc; enum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state; - int seen; + unsigned int seen; } store; static int matches(const char *key, const char *value) @@@ -2247,10 -2251,11 +2247,11 @@@ static int write_error(const char *file 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); @@@ -2266,15 -2271,16 +2267,16 @@@ 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; @@@ -2314,10 -2320,10 +2316,10 @@@ } 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, @@@ -2400,7 -2406,7 +2402,7 @@@ int git_config_set_multivar_in_file_gen { int fd = -1, in_fd = -1; int ret; - struct lock_file *lock = NULL; + static struct lock_file lock; char *filename_buf = NULL; char *contents = NULL; size_t contents_sz; @@@ -2419,7 -2425,8 +2421,7 @@@ * The lock serves a purpose in addition to locking: the new * contents of .git/config will be written into it. */ - lock = xcalloc(1, sizeof(struct lock_file)); - fd = hold_lock_file_for_update(lock, config_filename, 0); + fd = hold_lock_file_for_update(&lock, config_filename, 0); if (fd < 0) { error_errno("could not lock config file %s", config_filename); free(store.key); @@@ -2446,8 -2453,8 +2448,8 @@@ } 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; @@@ -2532,8 -2539,8 +2534,8 @@@ close(in_fd); in_fd = -1; - if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) { - error_errno("chmod on %s failed", get_lock_file_path(lock)); + if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) { + error_errno("chmod on %s failed", get_lock_file_path(&lock)); ret = CONFIG_NO_WRITE; goto out_free; } @@@ -2557,11 -2564,10 +2559,10 @@@ /* 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) + write_str_in_full(fd, "\n") < 0) goto write_err_out; } copy_begin = store.offset[i]; @@@ -2570,37 -2576,45 +2571,36 @@@ /* 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; } /* 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); contents = NULL; } - if (commit_lock_file(lock) < 0) { + if (commit_lock_file(&lock) < 0) { error_errno("could not write config file %s", config_filename); ret = CONFIG_NO_WRITE; - lock = NULL; goto out_free; } - /* - * lock is committed, so don't try to roll it back below. - * NOTE: Since lockfile.c keeps a linked list of all created - * lock_file structures, it isn't safe to free(lock). It's - * better to just leave it hanging around. - */ - lock = NULL; ret = 0; /* Invalidate the config cache */ git_config_clear(); out_free: - if (lock) - rollback_lock_file(lock); + rollback_lock_file(&lock); free(filename_buf); if (contents) munmap(contents, contents_sz); @@@ -2609,7 -2623,7 +2609,7 @@@ return ret; write_err_out: - ret = write_error(get_lock_file_path(lock)); + ret = write_error(get_lock_file_path(&lock)); goto out_free; } @@@ -2758,7 -2772,7 +2758,7 @@@ int git_config_rename_section_in_file(c 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; } @@@ -2784,7 -2798,7 +2784,7 @@@ 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 --combined diff.c index fa77b00492,6f58bca2f9..8406a8324e --- a/diff.c +++ b/diff.c @@@ -299,9 -299,6 +299,9 @@@ int git_diff_ui_config(const char *var return 0; } + if (git_color_config(var, value, cb) < 0) + return -1; + return git_diff_basic_config(var, value, cb); } @@@ -828,7 -825,7 +828,7 @@@ static void emit_rewrite_diff(const cha struct diff_words_buffer { mmfile_t text; - long alloc; + unsigned long alloc; struct diff_words_orig { const char *begin, *end; } *orig; @@@ -2978,7 -2975,7 +2978,7 @@@ static void prep_temp_blob(const char * 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 --combined fast-import.c index 365d3191aa,395524039b..32ac5323f6 --- a/fast-import.c +++ b/fast-import.c @@@ -2951,7 -2951,7 +2951,7 @@@ static void parse_reset_branch(const ch 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"); } @@@ -3188,10 -3188,10 +3188,10 @@@ static void checkpoint(void checkpoint_requested = 0; if (object_count) { cycle_packfile(); - dump_branches(); - dump_tags(); - dump_marks(); } + dump_branches(); + dump_tags(); + dump_marks(); } static void parse_checkpoint(void) diff --combined pkt-line.c index f364944b93,4823d3bb9d..647bbd3bce --- a/pkt-line.c +++ b/pkt-line.c @@@ -94,9 -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) @@@ -136,20 -136,18 +136,19 @@@ static void format_packet(struct strbu static int packet_write_fmt_1(int fd, int gently, const char *fmt, va_list args) { - struct strbuf buf = STRBUF_INIT; + static struct strbuf buf = STRBUF_INIT; - ssize_t count; + strbuf_reset(&buf); 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, ...) @@@ -184,9 -182,9 +183,9 @@@ static int packet_write_gently(const in 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, ...) diff --combined refs.c index 41aec6505d,a5abca04b7..7edcf6c91f --- a/refs.c +++ b/refs.c @@@ -592,7 -592,7 +592,7 @@@ static int write_pseudoref(const char * } } - 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; @@@ -921,8 -921,6 +921,8 @@@ int ref_transaction_update(struct ref_t return -1; } + flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS; + flags |= (new_sha1 ? REF_HAVE_NEW : 0) | (old_sha1 ? REF_HAVE_OLD : 0); ref_transaction_add_update(transaction, refname, flags, diff --combined sha1_file.c index 5911364a81,20a9d39c00..4d0e0c554d --- a/sha1_file.c +++ b/sha1_file.c @@@ -2761,6 -2761,7 +2761,6 @@@ off_t find_pack_entry_one(const unsigne const uint32_t *level1_ofs = p->index_data; const unsigned char *index = p->index_data; unsigned hi, lo, stride; - static int use_lookup = -1; static int debug_lookup = -1; if (debug_lookup < 0) @@@ -2790,6 -2791,16 +2790,6 @@@ printf("%02x%02x%02x... lo %u hi %u nr %"PRIu32"\n", sha1[0], sha1[1], sha1[2], lo, hi, p->num_objects); - if (use_lookup < 0) - use_lookup = !!getenv("GIT_USE_LOOKUP"); - if (use_lookup) { - int pos = sha1_entry_pos(index, stride, 0, - lo, hi, p->num_objects, sha1); - if (pos < 0) - return 0; - return nth_packed_object_offset(p, pos); - } - while (lo < hi) { unsigned mi = (lo + hi) / 2; int cmp = hashcmp(index + mi * stride, sha1); @@@ -2952,14 -2963,10 +2952,14 @@@ static int sha1_loose_object_info(cons } else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0) status = error("unable to parse %s header", sha1_to_hex(sha1)); - if (status >= 0 && oi->contentp) + if (status >= 0 && oi->contentp) { *oi->contentp = unpack_sha1_rest(&stream, hdr, *oi->sizep, sha1); - else + if (!*oi->contentp) { + git_inflate_end(&stream); + status = -1; + } + } else git_inflate_end(&stream); munmap(map, mapsize); @@@ -3715,7 -3722,7 +3715,7 @@@ int index_path(unsigned char *sha1, con 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; diff --combined transport-helper.c index 2b830b2290,2128a32f1c..a72ed18efb --- a/transport-helper.c +++ b/transport-helper.c @@@ -44,8 -44,7 +44,7 @@@ static void sendline(struct helper_dat { 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 +73,7 @@@ static void write_constant(int fd, cons { 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"); } @@@ -1117,13 -1116,6 +1116,13 @@@ int transport_helper_init(struct transp __attribute__((format (printf, 1, 2))) static void transfer_debug(const char *fmt, ...) { + /* + * NEEDSWORK: This function is sometimes used from multiple threads, and + * we end up using debug_enabled racily. That "should not matter" since + * we always write the same value, but it's still wrong. This function + * is listed in .tsan-suppressions for the time being. + */ + va_list args; char msgbuf[PBUFFERSIZE]; static int debug_enabled = -1;