diff options
author | lhchavez <lhchavez@lhchavez.com> | 2020-08-02 16:26:25 -0700 |
---|---|---|
committer | lhchavez <lhchavez@lhchavez.com> | 2020-11-28 15:12:14 -0800 |
commit | 4ae41f9c639d246d34dac89c3f1d9451c9cfa8d3 (patch) | |
tree | 5e207fe654fb3c5a7a6a4bbcc490f982e2ececc7 | |
parent | ef6de8d5d1c0e70f974d9c11e74f87f852b7c3ee (diff) | |
download | libgit2-4ae41f9c639d246d34dac89c3f1d9451c9cfa8d3.tar.gz |
Make the odb race-free
This change adds all the necessary locking to the odb to avoid races in
the backends.
Part of: #5592
-rw-r--r-- | script/thread-sanitizer.supp | 7 | ||||
-rw-r--r-- | src/config_cache.c | 5 | ||||
-rw-r--r-- | src/odb.c | 170 | ||||
-rw-r--r-- | src/odb.h | 1 | ||||
-rw-r--r-- | src/pack.c | 114 | ||||
-rw-r--r-- | src/repository.c | 5 |
6 files changed, 235 insertions, 67 deletions
diff --git a/script/thread-sanitizer.supp b/script/thread-sanitizer.supp index 757a0e79c..359a9b35e 100644 --- a/script/thread-sanitizer.supp +++ b/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 diff --git a/src/config_cache.c b/src/config_cache.c index e4e071b54..1a28ba96c 100644 --- a/src/config_cache.c +++ b/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; @@ -450,12 +450,18 @@ int git_odb_new(git_odb **out) git_odb *db = git__calloc(1, sizeof(*db)); GIT_ERROR_CHECK_ALLOC(db); + if (git_mutex_init(&db->lock) < 0) { + git__free(db); + return -1; + } if (git_cache_init(&db->own_cache) < 0) { + git_mutex_free(&db->lock); git__free(db); return -1; } if (git_vector_init(&db->backends, 4, backend_sort_cmp) < 0) { git_cache_dispose(&db->own_cache); + git_mutex_free(&db->lock); git__free(db); return -1; } @@ -487,13 +493,18 @@ static int add_backend_internal( internal->is_alternate = is_alternate; internal->disk_inode = disk_inode; + if (git_mutex_lock(&odb->lock) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return -1; + } if (git_vector_insert(&odb->backends, internal) < 0) { + git_mutex_unlock(&odb->lock); git__free(internal); return -1; } - git_vector_sort(&odb->backends); internal->backend->odb = odb; + git_mutex_unlock(&odb->lock); return 0; } @@ -509,8 +520,19 @@ int git_odb_add_alternate(git_odb *odb, git_odb_backend *backend, int priority) size_t git_odb_num_backends(git_odb *odb) { + size_t length; + bool locked = true; + GIT_ASSERT_ARG(odb); - return odb->backends.length; + + if (git_mutex_lock(&odb->lock) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + locked = false; + } + length = odb->backends.length; + if (locked) + git_mutex_unlock(&odb->lock); + return length; } static int git_odb__error_unsupported_in_backend(const char *action) @@ -524,19 +546,28 @@ static int git_odb__error_unsupported_in_backend(const char *action) int git_odb_get_backend(git_odb_backend **out, git_odb *odb, size_t pos) { backend_internal *internal; + int error; GIT_ASSERT_ARG(out); GIT_ASSERT_ARG(odb); + + if ((error = git_mutex_lock(&odb->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return error; + } internal = git_vector_get(&odb->backends, pos); - if (internal && internal->backend) { - *out = internal->backend; - return 0; + if (!internal || !internal->backend) { + git_mutex_unlock(&odb->lock); + + git_error_set(GIT_ERROR_ODB, "no ODB backend loaded at index %" PRIuZ, pos); + return GIT_ENOTFOUND; } + *out = internal->backend; + git_mutex_unlock(&odb->lock); - git_error_set(GIT_ERROR_ODB, "no ODB backend loaded at index %" PRIuZ, pos); - return GIT_ENOTFOUND; + return 0; } int git_odb__add_default_backends( @@ -567,11 +598,18 @@ int git_odb__add_default_backends( inode = st.st_ino; + if (git_mutex_lock(&db->lock) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return -1; + } for (i = 0; i < db->backends.length; ++i) { backend_internal *backend = git_vector_get(&db->backends, i); - if (backend->disk_inode == inode) + if (backend->disk_inode == inode) { + git_mutex_unlock(&db->lock); return 0; + } } + git_mutex_unlock(&db->lock); #endif /* add the loose object backend */ @@ -687,7 +725,12 @@ int git_odb__set_caps(git_odb *odb, int caps) static void odb_free(git_odb *db) { size_t i; + bool locked = true; + if (git_mutex_lock(&db->lock) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + locked = false; + } for (i = 0; i < db->backends.length; ++i) { backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *backend = internal->backend; @@ -696,9 +739,12 @@ static void odb_free(git_odb *db) git__free(internal); } + if (locked) + git_mutex_unlock(&db->lock); git_vector_free(&db->backends); git_cache_dispose(&db->own_cache); + git_mutex_free(&db->lock); git__memzero(db, sizeof(*db)); git__free(db); @@ -719,7 +765,12 @@ static int odb_exists_1( { size_t i; bool found = false; + int error; + if ((error = git_mutex_lock(&db->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return error; + } for (i = 0; i < db->backends.length && !found; ++i) { backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; @@ -730,6 +781,7 @@ static int odb_exists_1( if (b->exists != NULL) found = (bool)b->exists(b, id); } + git_mutex_unlock(&db->lock); return (int)found; } @@ -741,7 +793,12 @@ static int odb_freshen_1( { size_t i; bool found = false; + int error; + if ((error = git_mutex_lock(&db->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return error; + } for (i = 0; i < db->backends.length && !found; ++i) { backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; @@ -754,6 +811,7 @@ static int odb_freshen_1( else if (b->exists != NULL) found = b->exists(b, id); } + git_mutex_unlock(&db->lock); return (int)found; } @@ -805,6 +863,11 @@ static int odb_exists_prefix_1(git_oid *out, git_odb *db, int error = GIT_ENOTFOUND, num_found = 0; git_oid last_found = {{0}}, found; + if ((error = git_mutex_lock(&db->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return error; + } + error = GIT_ENOTFOUND; for (i = 0; i < db->backends.length; ++i) { backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; @@ -818,18 +881,23 @@ static int odb_exists_prefix_1(git_oid *out, git_odb *db, error = b->exists_prefix(&found, b, key, len); if (error == GIT_ENOTFOUND || error == GIT_PASSTHROUGH) continue; - if (error) + if (error) { + git_mutex_unlock(&db->lock); return error; + } /* make sure found item doesn't introduce ambiguity */ if (num_found) { - if (git_oid__cmp(&last_found, &found)) + if (git_oid__cmp(&last_found, &found)) { + git_mutex_unlock(&db->lock); return git_odb__error_ambiguous("multiple matches for prefix"); + } } else { git_oid_cpy(&last_found, &found); num_found++; } } + git_mutex_unlock(&db->lock); if (!num_found) return GIT_ENOTFOUND; @@ -971,6 +1039,10 @@ static int odb_read_header_1( return 0; } + if ((error = git_mutex_lock(&db->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return error; + } for (i = 0; i < db->backends.length; ++i) { backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; @@ -992,9 +1064,11 @@ static int odb_read_header_1( case GIT_ENOTFOUND: break; default: + git_mutex_unlock(&db->lock); return error; } } + git_mutex_unlock(&db->lock); return passthrough ? GIT_PASSTHROUGH : GIT_ENOTFOUND; } @@ -1067,6 +1141,10 @@ static int odb_read_1(git_odb_object **out, git_odb *db, const git_oid *id, return error; } + if ((error = git_mutex_lock(&db->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return error; + } for (i = 0; i < db->backends.length && !found; ++i) { backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; @@ -1079,12 +1157,15 @@ static int odb_read_1(git_odb_object **out, git_odb *db, const git_oid *id, if (error == GIT_PASSTHROUGH || error == GIT_ENOTFOUND) continue; - if (error < 0) + if (error < 0) { + git_mutex_unlock(&db->lock); return error; + } found = true; } } + git_mutex_unlock(&db->lock); if (!found) return GIT_ENOTFOUND; @@ -1177,6 +1258,10 @@ static int read_prefix_1(git_odb_object **out, git_odb *db, bool found = false; git_odb_object *object; + if ((error = git_mutex_lock(&db->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return error; + } for (i = 0; i < db->backends.length; ++i) { backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; @@ -1193,8 +1278,10 @@ static int read_prefix_1(git_odb_object **out, git_odb *db, continue; } - if (error) + if (error) { + git_mutex_unlock(&db->lock); goto out; + } git__free(data); data = raw.data; @@ -1209,6 +1296,7 @@ static int read_prefix_1(git_odb_object **out, git_odb *db, error = git_odb__error_ambiguous(buf.ptr); git_buf_dispose(&buf); + git_mutex_unlock(&db->lock); goto out; } @@ -1216,6 +1304,7 @@ static int read_prefix_1(git_odb_object **out, git_odb *db, found = true; } } + git_mutex_unlock(&db->lock); if (!found) return GIT_ENOTFOUND; @@ -1283,16 +1372,32 @@ int git_odb_read_prefix( int git_odb_foreach(git_odb *db, git_odb_foreach_cb cb, void *payload) { unsigned int i; + git_vector backends = GIT_VECTOR_INIT; backend_internal *internal; + int error = 0; - git_vector_foreach(&db->backends, i, internal) { + /* Make a copy of the backends vector to invoke the callback without holding the lock. */ + if ((error = git_mutex_lock(&db->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + goto cleanup; + } + error = git_vector_dup(&backends, &db->backends, NULL); + git_mutex_unlock(&db->lock); + + if (error < 0) + goto cleanup; + + git_vector_foreach(&backends, i, internal) { git_odb_backend *b = internal->backend; - int error = b->foreach(b, cb, payload); + error = b->foreach(b, cb, payload); if (error != 0) - return error; + goto cleanup; } - return 0; +cleanup: + git_vector_free(&backends); + + return error; } int git_odb_write( @@ -1314,6 +1419,10 @@ int git_odb_write( if (git_odb__freshen(db, oid)) return 0; + if ((error = git_mutex_lock(&db->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return error; + } for (i = 0, error = GIT_ERROR; i < db->backends.length && error < 0; ++i) { backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; @@ -1325,6 +1434,7 @@ int git_odb_write( if (b->write != NULL) error = b->write(b, oid, data, len, type); } + git_mutex_unlock(&db->lock); if (!error || error == GIT_PASSTHROUGH) return 0; @@ -1366,6 +1476,11 @@ int git_odb_open_wstream( GIT_ASSERT_ARG(stream); GIT_ASSERT_ARG(db); + if ((error = git_mutex_lock(&db->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return error; + } + error = GIT_ERROR; for (i = 0; i < db->backends.length && error < 0; ++i) { backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; @@ -1382,6 +1497,7 @@ int git_odb_open_wstream( error = init_fake_wstream(stream, b, size, type); } } + git_mutex_unlock(&db->lock); if (error < 0) { if (error == GIT_PASSTHROUGH) @@ -1477,6 +1593,11 @@ int git_odb_open_rstream( GIT_ASSERT_ARG(stream); GIT_ASSERT_ARG(db); + if ((error = git_mutex_lock(&db->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return error; + } + error = GIT_ERROR; for (i = 0; i < db->backends.length && error < 0; ++i) { backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; @@ -1486,6 +1607,7 @@ int git_odb_open_rstream( error = b->readstream(stream, len, type, b, oid); } } + git_mutex_unlock(&db->lock); if (error == GIT_PASSTHROUGH) error = 0; @@ -1503,6 +1625,11 @@ int git_odb_write_pack(struct git_odb_writepack **out, git_odb *db, git_indexer_ GIT_ASSERT_ARG(out); GIT_ASSERT_ARG(db); + if ((error = git_mutex_lock(&db->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return error; + } + error = GIT_ERROR; for (i = 0; i < db->backends.length && error < 0; ++i) { backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; @@ -1516,6 +1643,7 @@ int git_odb_write_pack(struct git_odb_writepack **out, git_odb *db, git_indexer_ error = b->writepack(out, b, db, progress_cb, progress_payload); } } + git_mutex_unlock(&db->lock); if (error == GIT_PASSTHROUGH) error = 0; @@ -1547,19 +1675,27 @@ void git_odb_backend_data_free(git_odb_backend *backend, void *data) int git_odb_refresh(struct git_odb *db) { size_t i; + int error; GIT_ASSERT_ARG(db); + if ((error = git_mutex_lock(&db->lock)) < 0) { + git_error_set(GIT_ERROR_ODB, "failed to acquire the odb lock"); + return error; + } for (i = 0; i < db->backends.length; ++i) { backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; if (b->refresh != NULL) { int error = b->refresh(b); - if (error < 0) + if (error < 0) { + git_mutex_unlock(&db->lock); return error; + } } } + git_mutex_unlock(&db->lock); return 0; } @@ -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; diff --git a/src/pack.c b/src/pack.c index f81bc91f2..5a96ac5b5 100644 --- a/src/pack.c +++ b/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( diff --git a/src/repository.c b/src/repository.c index 697d6dfe9..bef84caaf 100644 --- a/src/repository.c +++ b/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; } |