Merge branch 'jk/write-in-full-fix' into maint
authorJunio C Hamano <gitster@pobox.com>
Mon, 23 Oct 2017 05:37:21 +0000 (14:37 +0900)
committerJunio C Hamano <gitster@pobox.com>
Mon, 23 Oct 2017 05:37:22 +0000 (14:37 +0900)
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

1  2 
config.c
diff.c
fast-import.c
pkt-line.c
refs.c
sha1_file.c
transport-helper.c
diff --combined config.c
index 1a8e24f32ceda05861fda9704e195a81b28ce8ce,d70fe71273ceb47fd4ab2103b22ccd5e93856bc7..17e13493700e0a6883cc1ba26f00cdfc6900342d
+++ 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);
                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;
                }
        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;
         * 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);
                }
  
                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;
                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;
                }
                        /* 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];
                /* 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);
        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;
                                }
                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 fa77b00492688c67f0cff4d7f1ac54a1868403d4,6f58bca2f9256ae4ff413db5a13eb249866945e5..8406a8324ee281f55f59afbe1406ec1d91c0ca0c
--- 1/diff.c
--- 2/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 365d3191aa1abfb42b4d0da6973612126cc74f3b,395524039b97d3132e4f37f5a066c3f2f4d3a0b6..32ac5323f6310e40595df23b77698dea1622f9fa
@@@ -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 f364944b931a756b3b4819fb49ff5e9e01daf676,4823d3bb9db002fadf47124184ccea39d66fac68..647bbd3bceda71f15fdf137a37f3fa53e6fa6d86
@@@ -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 41aec6505d1309fa2efed2bd9aca0a0ef2646444,a5abca04b7888e1813942136955f0f7dd25efaa9..7edcf6c91f38e3d07b42939e35dbcebb3662261e
--- 1/refs.c
--- 2/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 5911364a81fa3d1351fa13d14a63c8854fd60e22,20a9d39c008a8b7bca1c6b8583b703396afa7e72..4d0e0c554d348d4104157bc16dd4795c1810689a
@@@ -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)
                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 2b830b22900545209fd4e697d88c6faf3d1fd431,2128a32f1cd11b96b63c03aa4e38cb43e08fc546..a72ed18efbfeedffb217a3d66daa9f2626cf6479
@@@ -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;