Merge pull request #5595 from lhchavez/odb-race-free

Make the odb race-free

script/thread-sanitizer.supp

@@ -8,11 +8,6 @@ deadlock:attr_cache_lock
 # data races.
 called_from_lib:libc.so.6
 
-# TODO(#5595): Remove these once the fixes land.
-race:src/odb.c
-race:git_repository_odb__weakptr
-race:cache_store
-
-# TODO(#5595): Investigate and fix this. It can be triggered by the `thread`
+# TODO(#5592): Investigate and fix this. It can be triggered by the `thread`
 # test suite.
 race:git_filter_list__load_ext

src/config_cache.c

@@ -111,17 +111,18 @@ int git_config__configmap_lookup(int *out, git_config *config, git_configmap_ite
 
 int git_repository__configmap_lookup(int *out, git_repository *repo, git_configmap_item item)
 {
-	*out = repo->configmap_cache[(int)item];
+	*out = (int)(intptr_t)git__load(repo->configmap_cache[(int)item]);
 
 	if (*out == GIT_CONFIGMAP_NOT_CACHED) {
 		int error;
+		int oldval = GIT_CONFIGMAP_NOT_CACHED;
 		git_config *config;
 
 		if ((error = git_repository_config__weakptr(&config, repo)) < 0 ||
 			(error = git_config__configmap_lookup(out, config, item)) < 0)
 			return error;
 
-		repo->configmap_cache[(int)item] = *out;
+		git__compare_and_swap(&repo->configmap_cache[(int)item], &oldval, out);
 	}
 
 	return 0;

src/odb.h

@@ -40,6 +40,7 @@ struct git_odb_object {
 /* EXPORT */
 struct git_odb {
 	git_refcount rc;
+	git_mutex lock;  /* protects backends */
 	git_vector backends;
 	git_cache own_cache;
 	unsigned int do_fsync :1;

src/pack.c

@@ -12,6 +12,7 @@
 #include "mwindow.h"
 #include "odb.h"
 #include "oid.h"
+#include "oidarray.h"
 
 /* Option to bypass checking existence of '.keep' files */
 bool git_disable_pack_keep_file_checks = false;
@@ -195,7 +196,8 @@ static void pack_index_free(struct git_pack_file *p)
 	}
 }
 
-static int pack_index_check(const char *path, struct git_pack_file *p)
+/* Run with the packfile lock held */
+static int pack_index_check_locked(const char *path, struct git_pack_file *p)
 {
 	struct git_pack_idx_header *hdr;
 	uint32_t version, nr, i, *index;
@@ -301,7 +303,8 @@ static int pack_index_check(const char *path, struct git_pack_file *p)
 	return 0;
 }
 
-static int pack_index_open(struct git_pack_file *p)
+/* Run with the packfile lock held */
+static int pack_index_open_locked(struct git_pack_file *p)
 {
 	int error = 0;
 	size_t name_len;
@@ -324,18 +327,11 @@ static int pack_index_open(struct git_pack_file *p)
 		return -1;
 	}
 
-	if ((error = git_mutex_lock(&p->lock)) < 0) {
-		git_buf_dispose(&idx_name);
-		return error;
-	}
-
 	if (p->index_version == -1)
-		error = pack_index_check(idx_name.ptr, p);
+		error = pack_index_check_locked(idx_name.ptr, p);
 
 	git_buf_dispose(&idx_name);
 
-	git_mutex_unlock(&p->lock);
-
 	return error;
 }
 
@@ -1015,13 +1011,14 @@ static int packfile_open(struct git_pack_file *p)
 	git_oid sha1;
 	unsigned char *idx_sha1;
 
-	if (p->index_version == -1 && pack_index_open(p) < 0)
-		return git_odb__error_notfound("failed to open packfile", NULL, 0);
-
-	/* if mwf opened by another thread, return now */
 	if (git_mutex_lock(&p->lock) < 0)
 		return packfile_error("failed to get lock for open");
 
+	if (pack_index_open_locked(p) < 0) {
+		git_mutex_unlock(&p->lock);
+		return git_odb__error_notfound("failed to open packfile", NULL, 0);
+	}
+
 	if (p->mwf.fd >= 0) {
 		git_mutex_unlock(&p->lock);
 		return 0;
@@ -1210,32 +1207,40 @@ int git_pack_foreach_entry(
 	git_odb_foreach_cb cb,
 	void *data)
 {
-	const unsigned char *index = p->index_map.data, *current;
+	const unsigned char *index, *current;
 	uint32_t i;
 	int error = 0;
+	git_array_oid_t oids = GIT_ARRAY_INIT;
+	git_oid *oid;
 
-	if (index == NULL) {
-		if ((error = pack_index_open(p)) < 0)
-			return error;
+	if (git_mutex_lock(&p->lock) < 0)
+		return packfile_error("failed to get lock for git_pack_foreach_entry");
 
-		GIT_ASSERT(p->index_map.data);
-		index = p->index_map.data;
+	if ((error = pack_index_open_locked(p)) < 0) {
+		git_mutex_unlock(&p->lock);
+		return error;
 	}
 
-	if (p->index_version > 1) {
+	GIT_ASSERT(p->index_map.data);
+	index = p->index_map.data;
+
+	if (p->index_version > 1)
 		index += 8;
-	}
 
 	index += 4 * 256;
 
 	if (p->oids == NULL) {
 		git_vector offsets, oids;
 
-		if ((error = git_vector_init(&oids, p->num_objects, NULL)))
+		if ((error = git_vector_init(&oids, p->num_objects, NULL))) {
+			git_mutex_unlock(&p->lock);
 			return error;
+		}
 
-		if ((error = git_vector_init(&offsets, p->num_objects, git__memcmp4)))
+		if ((error = git_vector_init(&offsets, p->num_objects, git__memcmp4))) {
+			git_mutex_unlock(&p->lock);
 			return error;
+		}
 
 		if (p->index_version > 1) {
 			const unsigned char *off = index + 24 * p->num_objects;
@@ -1256,10 +1261,33 @@ int git_pack_foreach_entry(
 		p->oids = (git_oid **)git_vector_detach(NULL, NULL, &oids);
 	}
 
-	for (i = 0; i < p->num_objects; i++)
-		if ((error = cb(p->oids[i], data)) != 0)
-			return git_error_set_after_callback(error);
+	/* We need to copy the OIDs to another array before we relinquish the lock to avoid races. */
+	git_array_init_to_size(oids, p->num_objects);
+	if (!oids.ptr) {
+		git_mutex_unlock(&p->lock);
+		git_array_clear(oids);
+		GIT_ERROR_CHECK_ARRAY(oids);
+	}
+	for (i = 0; i < p->num_objects; i++) {
+		oid = git_array_alloc(oids);
+		if (!oid) {
+			git_mutex_unlock(&p->lock);
+			git_array_clear(oids);
+			GIT_ERROR_CHECK_ALLOC(oid);
+		}
+		git_oid_cpy(oid, p->oids[i]);
+	}
+
+	git_mutex_unlock(&p->lock);
+
+	git_array_foreach(oids, i, oid) {
+		if ((error = cb(oid, data)) != 0) {
+			git_error_set_after_callback(error);
+			break;
+		}
+	}
 
+	git_array_clear(oids);
 	return error;
 }
 
@@ -1297,18 +1325,17 @@ static int pack_entry_find_offset(
 	int pos, found = 0;
 	off64_t offset;
 	const unsigned char *current = 0;
+	int error = 0;
 
 	*offset_out = 0;
 
-	if (p->index_version == -1) {
-		int error;
-
-		if ((error = pack_index_open(p)) < 0)
-			return error;
+	if (git_mutex_lock(&p->lock) < 0)
+		return packfile_error("failed to get lock for pack_entry_find_offset");
 
-		GIT_ASSERT(p->index_map.data);
-	}
+	if ((error = pack_index_open_locked(p)) < 0)
+		goto cleanup;
 
+	GIT_ASSERT(p->index_map.data);
 	index = p->index_map.data;
 	level1_ofs = p->index_map.data;
 
@@ -1360,14 +1387,19 @@ static int pack_entry_find_offset(
 		}
 	}
 
-	if (!found)
-		return git_odb__error_notfound("failed to find offset for pack entry", short_oid, len);
-	if (found > 1)
-		return git_odb__error_ambiguous("found multiple offsets for pack entry");
+	if (!found) {
+		error = git_odb__error_notfound("failed to find offset for pack entry", short_oid, len);
+		goto cleanup;
+	}
+	if (found > 1) {
+		error = git_odb__error_ambiguous("found multiple offsets for pack entry");
+		goto cleanup;
+	}
 
 	if ((offset = nth_packed_object_offset(p, pos)) < 0) {
 		git_error_set(GIT_ERROR_ODB, "packfile index is corrupt");
-		return -1;
+		error = -1;
+		goto cleanup;
 	}
 
 	*offset_out = offset;
@@ -1382,7 +1414,9 @@ static int pack_entry_find_offset(
 	}
 #endif
 
-	return 0;
+cleanup:
+	git_mutex_unlock(&p->lock);
+	return error;
 }
 
 int git_pack_entry_find(

src/repository.c

@@ -1107,7 +1107,8 @@ int git_repository_odb__weakptr(git_odb **out, git_repository *repo)
 	GIT_ASSERT_ARG(repo);
 	GIT_ASSERT_ARG(out);
 
-	if (repo->_odb == NULL) {
+	*out = git__load(repo->_odb);
+	if (*out == NULL) {
 		git_buf odb_path = GIT_BUF_INIT;
 		git_odb *odb;
 
@@ -1131,9 +1132,9 @@ int git_repository_odb__weakptr(git_odb **out, git_repository *repo)
 		}
 
 		git_buf_dispose(&odb_path);
+		*out = git__load(repo->_odb);
 	}
 
-	*out = repo->_odb;
 	return error;
 }