Merge branch 'jk/index-pack-threading-races'
authorJunio C Hamano <gitster@pobox.com>
Fri, 19 Sep 2014 18:38:33 +0000 (11:38 -0700)
committerJunio C Hamano <gitster@pobox.com>
Fri, 19 Sep 2014 18:38:34 +0000 (11:38 -0700)
When receiving an invalid pack stream that records the same object
twice, multiple threads got confused due to a race. We should
reject or correct such a stream upon receiving, but that will be a
larger change.

* jk/index-pack-threading-races:
index-pack: fix race condition with duplicate bases

1  2 
builtin/index-pack.c
diff --combined builtin/index-pack.c
index 5568a5bc3b69be79f0d5fa09c694fd976c8319cc,885fa93fd3038ad67ea1d74ea7562843efe3ec2a..eebf1a8fc21fd551f3917ad23ab5a45dff302f23
@@@ -40,13 -40,17 +40,13 @@@ struct base_data 
        int ofs_first, ofs_last;
  };
  
 -#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)
 -/* pread() emulation is not thread-safe. Disable threading. */
 -#define NO_PTHREADS
 -#endif
 -
  struct thread_local {
  #ifndef NO_PTHREADS
        pthread_t thread;
  #endif
        struct base_data *base_cache;
        size_t base_cache_used;
 +      int pack_fd;
  };
  
  /*
@@@ -87,8 -91,7 +87,8 @@@ static off_t consumed_bytes
  static unsigned deepest_delta;
  static git_SHA_CTX input_ctx;
  static uint32_t input_crc32;
 -static int input_fd, output_fd, pack_fd;
 +static int input_fd, output_fd;
 +static const char *curr_pack;
  
  #ifndef NO_PTHREADS
  
@@@ -112,6 -115,10 +112,10 @@@ static pthread_mutex_t deepest_delta_mu
  #define deepest_delta_lock()  lock_mutex(&deepest_delta_mutex)
  #define deepest_delta_unlock()        unlock_mutex(&deepest_delta_mutex)
  
+ static pthread_mutex_t type_cas_mutex;
+ #define type_cas_lock()               lock_mutex(&type_cas_mutex)
+ #define type_cas_unlock()     unlock_mutex(&type_cas_mutex)
  static pthread_key_t key;
  
  static inline void lock_mutex(pthread_mutex_t *mutex)
@@@ -131,36 -138,28 +135,38 @@@ static inline void unlock_mutex(pthread
   */
  static void init_thread(void)
  {
 +      int i;
        init_recursive_mutex(&read_mutex);
        pthread_mutex_init(&counter_mutex, NULL);
        pthread_mutex_init(&work_mutex, NULL);
+       pthread_mutex_init(&type_cas_mutex, NULL);
        if (show_stat)
                pthread_mutex_init(&deepest_delta_mutex, NULL);
        pthread_key_create(&key, NULL);
        thread_data = xcalloc(nr_threads, sizeof(*thread_data));
 +      for (i = 0; i < nr_threads; i++) {
 +              thread_data[i].pack_fd = open(curr_pack, O_RDONLY);
 +              if (thread_data[i].pack_fd == -1)
 +                      die_errno(_("unable to open %s"), curr_pack);
 +      }
 +
        threads_active = 1;
  }
  
  static void cleanup_thread(void)
  {
 +      int i;
        if (!threads_active)
                return;
        threads_active = 0;
        pthread_mutex_destroy(&read_mutex);
        pthread_mutex_destroy(&counter_mutex);
        pthread_mutex_destroy(&work_mutex);
+       pthread_mutex_destroy(&type_cas_mutex);
        if (show_stat)
                pthread_mutex_destroy(&deepest_delta_mutex);
 +      for (i = 0; i < nr_threads; i++)
 +              close(thread_data[i].pack_fd);
        pthread_key_delete(key);
        free(thread_data);
  }
@@@ -207,13 -206,8 +213,13 @@@ static unsigned check_object(struct obj
        if (!(obj->flags & FLAG_CHECKED)) {
                unsigned long size;
                int type = sha1_object_info(obj->sha1, &size);
 -              if (type != obj->type || type <= 0)
 -                      die(_("object of unexpected type"));
 +              if (type <= 0)
 +                      die(_("did not receive expected object %s"),
 +                            sha1_to_hex(obj->sha1));
 +              if (type != obj->type)
 +                      die(_("object %s: expected type %s, found %s"),
 +                          sha1_to_hex(obj->sha1),
 +                          typename(obj->type), typename(type));
                obj->flags |= FLAG_CHECKED;
                return 1;
        }
@@@ -300,13 -294,13 +306,13 @@@ static const char *open_pack_file(cons
                        output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);
                if (output_fd < 0)
                        die_errno(_("unable to create '%s'"), pack_name);
 -              pack_fd = output_fd;
 +              nothread_data.pack_fd = output_fd;
        } else {
                input_fd = open(pack_name, O_RDONLY);
                if (input_fd < 0)
                        die_errno(_("cannot open packfile '%s'"), pack_name);
                output_fd = -1;
 -              pack_fd = input_fd;
 +              nothread_data.pack_fd = input_fd;
        }
        git_SHA1_Init(&input_ctx);
        return pack_name;
@@@ -362,7 -356,8 +368,7 @@@ static void set_thread_data(struct thre
  
  static struct base_data *alloc_base_data(void)
  {
 -      struct base_data *base = xmalloc(sizeof(struct base_data));
 -      memset(base, 0, sizeof(*base));
 +      struct base_data *base = xcalloc(1, sizeof(struct base_data));
        base->ref_last = -1;
        base->ofs_last = -1;
        return base;
@@@ -553,7 -548,7 +559,7 @@@ static void *unpack_data(struct object_
  
        do {
                ssize_t n = (len < 64*1024) ? len : 64*1024;
 -              n = pread(pack_fd, inbuf, n, from);
 +              n = xpread(get_thread_data()->pack_fd, inbuf, n, from);
                if (n < 0)
                        die_errno(_("cannot pread pack file"));
                if (!n)
@@@ -785,8 -780,7 +791,8 @@@ static void sha1_object(const void *dat
                        }
                        if (obj->type == OBJ_COMMIT) {
                                struct commit *commit = (struct commit *) obj;
 -                              commit->buffer = NULL;
 +                              if (detach_commit_buffer(commit, NULL) != data)
 +                                      die("BUG: parse_object_buffer transmogrified our buffer");
                        }
                        obj->flags |= FLAG_CHECKED;
                }
@@@ -862,7 -856,6 +868,6 @@@ static void resolve_delta(struct object
  {
        void *base_data, *delta_data;
  
-       delta_obj->real_type = base->obj->real_type;
        if (show_stat) {
                delta_obj->delta_depth = base->obj->delta_depth + 1;
                deepest_delta_lock();
        counter_unlock();
  }
  
+ /*
+  * Standard boolean compare-and-swap: atomically check whether "*type" is
+  * "want"; if so, swap in "set" and return true. Otherwise, leave it untouched
+  * and return false.
+  */
+ static int compare_and_swap_type(enum object_type *type,
+                                enum object_type want,
+                                enum object_type set)
+ {
+       enum object_type old;
+       type_cas_lock();
+       old = *type;
+       if (old == want)
+               *type = set;
+       type_cas_unlock();
+       return old == want;
+ }
  static struct base_data *find_unresolved_deltas_1(struct base_data *base,
                                                  struct base_data *prev_base)
  {
                struct object_entry *child = objects + deltas[base->ref_first].obj_no;
                struct base_data *result = alloc_base_data();
  
-               assert(child->real_type == OBJ_REF_DELTA);
+               if (!compare_and_swap_type(&child->real_type, OBJ_REF_DELTA,
+                                          base->obj->real_type))
+                       die("BUG: child->real_type != OBJ_REF_DELTA");
                resolve_delta(child, base, result);
                if (base->ref_first == base->ref_last && base->ofs_last == -1)
                        free_base_data(base);
                struct base_data *result = alloc_base_data();
  
                assert(child->real_type == OBJ_OFS_DELTA);
+               child->real_type = base->obj->real_type;
                resolve_delta(child, base, result);
                if (base->ofs_first == base->ofs_last)
                        free_base_data(base);
@@@ -1303,7 -1320,7 +1332,7 @@@ static void final(const char *final_pac
                if (keep_fd < 0) {
                        if (errno != EEXIST)
                                die_errno(_("cannot write keep file '%s'"),
 -                                        keep_name);
 +                                        keep_name ? keep_name : name);
                } else {
                        if (keep_msg_len > 0) {
                                write_or_die(keep_fd, keep_msg, keep_msg_len);
                        }
                        if (close(keep_fd) != 0)
                                die_errno(_("cannot close written keep file '%s'"),
 -                                  keep_name);
 +                                        keep_name ? keep_name : name);
                        report = "keep";
                }
        }
@@@ -1502,11 -1519,10 +1531,11 @@@ static void show_pack_info(int stat_onl
  int cmd_index_pack(int argc, const char **argv, const char *prefix)
  {
        int i, fix_thin_pack = 0, verify = 0, stat_only = 0;
 -      const char *curr_pack, *curr_index;
 +      const char *curr_index;
        const char *index_name = NULL, *pack_name = NULL;
        const char *keep_name = NULL, *keep_msg = NULL;
 -      char *index_name_buf = NULL, *keep_name_buf = NULL;
 +      struct strbuf index_name_buf = STRBUF_INIT,
 +                    keep_name_buf = STRBUF_INIT;
        struct pack_idx_entry **idx_objects;
        struct pack_idx_option opts;
        unsigned char pack_sha1[20];
        if (argc == 2 && !strcmp(argv[1], "-h"))
                usage(index_pack_usage);
  
 -      read_replace_refs = 0;
 +      check_replace_refs = 0;
  
        reset_pack_idx_option(&opts);
        git_config(git_index_pack_config, &opts);
                                stat_only = 1;
                        } else if (!strcmp(arg, "--keep")) {
                                keep_msg = "";
 -                      } else if (!prefixcmp(arg, "--keep=")) {
 +                      } else if (starts_with(arg, "--keep=")) {
                                keep_msg = arg + 7;
 -                      } else if (!prefixcmp(arg, "--threads=")) {
 +                      } else if (starts_with(arg, "--threads=")) {
                                char *end;
                                nr_threads = strtoul(arg+10, &end, 0);
                                if (!arg[10] || *end || nr_threads < 0)
                                                  "ignoring %s"), arg);
                                nr_threads = 1;
  #endif
 -                      } else if (!prefixcmp(arg, "--pack_header=")) {
 +                      } else if (starts_with(arg, "--pack_header=")) {
                                struct pack_header *hdr;
                                char *c;
  
                                if (index_name || (i+1) >= argc)
                                        usage(index_pack_usage);
                                index_name = argv[++i];
 -                      } else if (!prefixcmp(arg, "--index-version=")) {
 +                      } else if (starts_with(arg, "--index-version=")) {
                                char *c;
                                opts.version = strtoul(arg + 16, &c, 10);
                                if (opts.version > 2)
        if (fix_thin_pack && !from_stdin)
                die(_("--fix-thin cannot be used without --stdin"));
        if (!index_name && pack_name) {
 -              int len = strlen(pack_name);
 -              if (!has_extension(pack_name, ".pack"))
 +              size_t len;
 +              if (!strip_suffix(pack_name, ".pack", &len))
                        die(_("packfile name '%s' does not end with '.pack'"),
                            pack_name);
 -              index_name_buf = xmalloc(len);
 -              memcpy(index_name_buf, pack_name, len - 5);
 -              strcpy(index_name_buf + len - 5, ".idx");
 -              index_name = index_name_buf;
 +              strbuf_add(&index_name_buf, pack_name, len);
 +              strbuf_addstr(&index_name_buf, ".idx");
 +              index_name = index_name_buf.buf;
        }
        if (keep_msg && !keep_name && pack_name) {
 -              int len = strlen(pack_name);
 -              if (!has_extension(pack_name, ".pack"))
 +              size_t len;
 +              if (!strip_suffix(pack_name, ".pack", &len))
                        die(_("packfile name '%s' does not end with '.pack'"),
                            pack_name);
 -              keep_name_buf = xmalloc(len);
 -              memcpy(keep_name_buf, pack_name, len - 5);
 -              strcpy(keep_name_buf + len - 5, ".keep");
 -              keep_name = keep_name_buf;
 +              strbuf_add(&keep_name_buf, pack_name, len);
 +              strbuf_addstr(&keep_name_buf, ".idx");
 +              keep_name = keep_name_buf.buf;
        }
        if (verify) {
                if (!index_name)
        else
                close(input_fd);
        free(objects);
 -      free(index_name_buf);
 -      free(keep_name_buf);
 +      strbuf_release(&index_name_buf);
 +      strbuf_release(&keep_name_buf);
        if (pack_name == NULL)
                free((void *) curr_pack);
        if (index_name == NULL)