diff options
author | Edward Thomson <ethomson@edwardthomson.com> | 2020-11-29 09:40:39 +0000 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-11-29 09:40:39 +0000 |
commit | 87e4597caa1a26b8fdd97213ee2d4041e67f5f0c (patch) | |
tree | b1d9d4d1ee06d5d858ecca2f9d5db988a986dc64 | |
parent | 5699ef81942538bc24df5285876e5830570b65f3 (diff) | |
parent | 322c15ee858622f2e3def514d3e7e1b47023950e (diff) | |
download | libgit2-87e4597caa1a26b8fdd97213ee2d4041e67f5f0c.tar.gz |
Merge pull request #5593 from lhchavez/fix-mwindow-thread-unsafety
Make the pack and mwindow implementations data-race-free
-rw-r--r-- | script/thread-sanitizer.supp | 13 | ||||
-rw-r--r-- | src/indexer.c | 14 | ||||
-rw-r--r-- | src/mwindow.c | 120 | ||||
-rw-r--r-- | src/mwindow.h | 4 | ||||
-rw-r--r-- | src/pack.c | 241 | ||||
-rw-r--r-- | src/pack.h | 7 | ||||
-rw-r--r-- | src/unix/map.c | 2 | ||||
-rw-r--r-- | tests/pack/filelimit.c | 1 | ||||
-rw-r--r-- | tests/pack/threadsafety.c | 60 |
9 files changed, 322 insertions, 140 deletions
diff --git a/script/thread-sanitizer.supp b/script/thread-sanitizer.supp index 359a9b35e..97d23046f 100644 --- a/script/thread-sanitizer.supp +++ b/script/thread-sanitizer.supp @@ -3,6 +3,19 @@ # consistent lock hierarchy that is easy to understand. deadlock:attr_cache_lock +# git_mwindow_file_register has the possibility of evicting some files from the +# global cache. In order to avoid races and closing files that are currently +# being accessed, before evicting any file it will attempt to acquire that +# file's lock. Finally, git_mwindow_file_register is typically called with a +# file lock held, because the caller will use the fd in the mwf immediately +# after registering it. This causes ThreadSanitizer to observe different orders +# of acquisition of the mutex (which implies a possibility of a deadlock), +# _but_ since the files are added to the cache after other files have been +# evicted, there cannot be a case where mwf A is trying to be registered while +# evicting mwf B concurrently and viceversa: at most one of them can be present +# in the cache. +deadlock:git_mwindow_file_register + # When invoking the time/timezone functions from git_signature_now(), they # access libc methods that need to be instrumented to correctly analyze the # data races. diff --git a/src/indexer.c b/src/indexer.c index 7b3db3101..453bb3df7 100644 --- a/src/indexer.c +++ b/src/indexer.c @@ -24,8 +24,6 @@ #include "zstream.h" #include "object.h" -extern git_mutex git__mwindow_mutex; - size_t git_indexer__max_objects = UINT32_MAX; #define UINT31_MAX (0x7FFFFFFF) @@ -679,7 +677,7 @@ static int read_stream_object(git_indexer *idx, git_indexer_progress *stats) return GIT_EBUFS; if (!idx->have_stream) { - error = git_packfile_unpack_header(&entry_size, &type, &idx->pack->mwf, &w, &idx->off); + error = git_packfile_unpack_header(&entry_size, &type, idx->pack, &w, &idx->off); if (error == GIT_EBUFS) { idx->off = entry_start; return error; @@ -970,7 +968,7 @@ static int fix_thin_pack(git_indexer *idx, git_indexer_progress *stats) continue; curpos = delta->delta_off; - error = git_packfile_unpack_header(&size, &type, &idx->pack->mwf, &w, &curpos); + error = git_packfile_unpack_header(&size, &type, idx->pack, &w, &curpos); if (error < 0) return error; @@ -1333,13 +1331,7 @@ void git_indexer_free(git_indexer *idx) git_vector_free_deep(&idx->deltas); - if (!git_mutex_lock(&git__mwindow_mutex)) { - if (!idx->pack_committed) - git_packfile_close(idx->pack, true); - - git_packfile_free(idx->pack); - git_mutex_unlock(&git__mwindow_mutex); - } + git_packfile_free(idx->pack, !idx->pack_committed); iter = 0; while (git_oidmap_iterate((void **) &value, idx->expected_oids, &iter, &key) == 0) diff --git a/src/mwindow.c b/src/mwindow.c index 66fd21823..4f105e659 100644 --- a/src/mwindow.c +++ b/src/mwindow.c @@ -29,10 +29,10 @@ size_t git_mwindow__window_size = DEFAULT_WINDOW_SIZE; size_t git_mwindow__mapped_limit = DEFAULT_MAPPED_LIMIT; size_t git_mwindow__file_limit = DEFAULT_FILE_LIMIT; -/* Mutex to control access */ +/* Mutex to control access to `git_mwindow__mem_ctl` and `git__pack_cache`. */ git_mutex git__mwindow_mutex; -/* Whenever you want to read or modify this, grab git__mwindow_mutex */ +/* Whenever you want to read or modify this, grab `git__mwindow_mutex` */ git_mwindow_ctl git_mwindow__mem_ctl; /* Global list of mwindow files, to open packs once across repos */ @@ -95,10 +95,9 @@ int git_mwindow_get_pack(struct git_pack_file **out, const char *path) error = git_strmap_set(git__pack_cache, pack->pack_name, pack); git_mutex_unlock(&git__mwindow_mutex); - if (error < 0) { - git_packfile_free(pack); - return -1; + git_packfile_free(pack, false); + return error; } *out = pack; @@ -108,6 +107,7 @@ int git_mwindow_get_pack(struct git_pack_file **out, const char *path) int git_mwindow_put_pack(struct git_pack_file *pack) { int count, error; + struct git_pack_file *pack_to_delete = NULL; if ((error = git_mutex_lock(&git__mwindow_mutex)) < 0) return error; @@ -121,34 +121,19 @@ int git_mwindow_put_pack(struct git_pack_file *pack) count = git_atomic_dec(&pack->refcount); if (count == 0) { git_strmap_delete(git__pack_cache, pack->pack_name); - git_packfile_free(pack); + pack_to_delete = pack; } - git_mutex_unlock(&git__mwindow_mutex); - return 0; -} + git_packfile_free(pack_to_delete, false); -int git_mwindow_free_all(git_mwindow_file *mwf) -{ - int error; - - if (git_mutex_lock(&git__mwindow_mutex)) { - git_error_set(GIT_ERROR_THREAD, "unable to lock mwindow mutex"); - return -1; - } - - error = git_mwindow_free_all_locked(mwf); - - git_mutex_unlock(&git__mwindow_mutex); - - return error; + return 0; } /* * Free all the windows in a sequence, typically because we're done - * with the file + * with the file. Needs to hold the git__mwindow_mutex. */ -int git_mwindow_free_all_locked(git_mwindow_file *mwf) +static int git_mwindow_free_all_locked(git_mwindow_file *mwf) { git_mwindow_ctl *ctl = &git_mwindow__mem_ctl; size_t i; @@ -184,6 +169,22 @@ int git_mwindow_free_all_locked(git_mwindow_file *mwf) return 0; } +int git_mwindow_free_all(git_mwindow_file *mwf) +{ + int error; + + if (git_mutex_lock(&git__mwindow_mutex)) { + git_error_set(GIT_ERROR_THREAD, "unable to lock mwindow mutex"); + return -1; + } + + error = git_mwindow_free_all_locked(mwf); + + git_mutex_unlock(&git__mwindow_mutex); + + return error; +} + /* * Check if a window 'win' contains the address 'offset' */ @@ -256,9 +257,9 @@ static bool git_mwindow_scan_recently_used( /* * Close the least recently used window (that is currently not being used) out - * of all the files. Called under lock from new_window. + * of all the files. Called under lock from new_window_locked. */ -static int git_mwindow_close_lru_window(void) +static int git_mwindow_close_lru_window_locked(void) { git_mwindow_ctl *ctl = &git_mwindow__mem_ctl; git_mwindow_file *cur; @@ -292,13 +293,13 @@ static int git_mwindow_close_lru_window(void) } /* - * Close the file that does not have any open windows AND whose + * Finds the file that does not have any open windows AND whose * most-recently-used window is the least-recently used one across all * currently open files. * - * Called under lock from new_window. + * Called under lock from new_window_locked. */ -static int git_mwindow_close_lru_file(void) +static int git_mwindow_find_lru_file_locked(git_mwindow_file **out) { git_mwindow_ctl *ctl = &git_mwindow__mem_ctl; git_mwindow_file *lru_file = NULL, *current_file = NULL; @@ -320,15 +321,12 @@ static int git_mwindow_close_lru_file(void) return -1; } - git_mwindow_free_all_locked(lru_file); - p_close(lru_file->fd); - lru_file->fd = -1; - + *out = lru_file; return 0; } /* This gets called under lock from git_mwindow_open */ -static git_mwindow *new_window( +static git_mwindow *new_window_locked( git_file fd, off64_t size, off64_t offset) @@ -338,12 +336,11 @@ static git_mwindow *new_window( off64_t len; git_mwindow *w; - w = git__malloc(sizeof(*w)); + w = git__calloc(1, sizeof(*w)); if (w == NULL) return NULL; - memset(w, 0x0, sizeof(*w)); w->offset = (offset / walign) * walign; len = size - w->offset; @@ -353,7 +350,7 @@ static git_mwindow *new_window( ctl->mapped += (size_t)len; while (git_mwindow__mapped_limit < ctl->mapped && - git_mwindow_close_lru_window() == 0) /* nop */; + git_mwindow_close_lru_window_locked() == 0) /* nop */; /* * We treat `mapped_limit` as a soft limit. If we can't find a @@ -367,7 +364,7 @@ static git_mwindow *new_window( * we're below our soft limits, so free up what we can and try again. */ - while (git_mwindow_close_lru_window() == 0) + while (git_mwindow_close_lru_window_locked() == 0) /* nop */; if (git_futils_mmap_ro(&w->window_map, fd, w->offset, (size_t)len) < 0) { @@ -423,7 +420,7 @@ unsigned char *git_mwindow_open( * one. */ if (!w) { - w = new_window(mwf->fd, mwf->size, offset); + w = new_window_locked(mwf->fd, mwf->size, offset); if (w == NULL) { git_mutex_unlock(&git__mwindow_mutex); return NULL; @@ -451,8 +448,11 @@ unsigned char *git_mwindow_open( int git_mwindow_file_register(git_mwindow_file *mwf) { + git_vector closed_files = GIT_VECTOR_INIT; git_mwindow_ctl *ctl = &git_mwindow__mem_ctl; - int ret; + int error; + size_t i; + git_mwindow_file *closed_file = NULL; if (git_mutex_lock(&git__mwindow_mutex)) { git_error_set(GIT_ERROR_THREAD, "unable to lock mwindow mutex"); @@ -460,20 +460,48 @@ int git_mwindow_file_register(git_mwindow_file *mwf) } if (ctl->windowfiles.length == 0 && - git_vector_init(&ctl->windowfiles, 8, NULL) < 0) { + (error = git_vector_init(&ctl->windowfiles, 8, NULL)) < 0) { git_mutex_unlock(&git__mwindow_mutex); - return -1; + goto cleanup; } if (git_mwindow__file_limit) { + git_mwindow_file *lru_file; while (git_mwindow__file_limit <= ctl->windowfiles.length && - git_mwindow_close_lru_file() == 0) /* nop */; + git_mwindow_find_lru_file_locked(&lru_file) == 0) { + if ((error = git_vector_insert(&closed_files, lru_file)) < 0) { + /* + * Exceeding the file limit seems preferrable to being open to + * data races that can end up corrupting the heap. + */ + break; + } + git_mwindow_free_all_locked(lru_file); + } } - ret = git_vector_insert(&ctl->windowfiles, mwf); + error = git_vector_insert(&ctl->windowfiles, mwf); git_mutex_unlock(&git__mwindow_mutex); + if (error < 0) + goto cleanup; - return ret; + /* + * Once we have released the global windowfiles lock, we can close each + * individual file. Before doing so, acquire that file's lock to avoid + * closing a file that is currently being used. + */ + git_vector_foreach(&closed_files, i, closed_file) { + error = git_mutex_lock(&closed_file->lock); + if (error < 0) + continue; + p_close(closed_file->fd); + closed_file->fd = -1; + git_mutex_unlock(&closed_file->lock); + } + +cleanup: + git_vector_free(&closed_files); + return error; } void git_mwindow_file_deregister(git_mwindow_file *mwf) diff --git a/src/mwindow.h b/src/mwindow.h index b379fba2d..e3a03f019 100644 --- a/src/mwindow.h +++ b/src/mwindow.h @@ -13,8 +13,6 @@ #include "map.h" #include "vector.h" -extern git_mutex git__mwindow_mutex; - typedef struct git_mwindow { struct git_mwindow *next; git_map window_map; @@ -24,6 +22,7 @@ typedef struct git_mwindow { } git_mwindow; typedef struct git_mwindow_file { + git_mutex lock; /* protects updates to fd */ git_mwindow *windows; int fd; off64_t size; @@ -41,7 +40,6 @@ typedef struct git_mwindow_ctl { int git_mwindow_contains(git_mwindow *win, off64_t offset); int git_mwindow_free_all(git_mwindow_file *mwf); /* locks */ -int git_mwindow_free_all_locked(git_mwindow_file *mwf); /* run under lock */ unsigned char *git_mwindow_open(git_mwindow_file *mwf, git_mwindow **cursor, off64_t offset, size_t extra, unsigned int *left); int git_mwindow_file_register(git_mwindow_file *mwf); void git_mwindow_file_deregister(git_mwindow_file *mwf); diff --git a/src/pack.c b/src/pack.c index 5a96ac5b5..b88c52acf 100644 --- a/src/pack.c +++ b/src/pack.c @@ -17,8 +17,8 @@ /* Option to bypass checking existence of '.keep' files */ bool git_disable_pack_keep_file_checks = false; -static int packfile_open(struct git_pack_file *p); -static off64_t nth_packed_object_offset(const struct git_pack_file *p, uint32_t n); +static int packfile_open_locked(struct git_pack_file *p); +static off64_t nth_packed_object_offset_locked(struct git_pack_file *p, uint32_t n); static int packfile_unpack_compressed( git_rawobj *obj, struct git_pack_file *p, @@ -129,7 +129,7 @@ static void free_lowest_entry(git_pack_cache *cache) git_pack_cache_entry *entry; git_offmap_foreach(cache->entries, offset, entry, { - if (entry && entry->refcount.val == 0) { + if (entry && git_atomic_get(&entry->refcount) == 0) { cache->memory_used -= entry->raw.len; git_offmap_delete(cache->entries, offset); free_cache_object(entry); @@ -308,28 +308,29 @@ static int pack_index_open_locked(struct git_pack_file *p) { int error = 0; size_t name_len; - git_buf idx_name; + git_buf idx_name = GIT_BUF_INIT; if (p->index_version > -1) - return 0; + goto cleanup; /* checked by git_pack_file alloc */ name_len = strlen(p->pack_name); GIT_ASSERT(name_len > strlen(".pack")); - if (git_buf_init(&idx_name, name_len) < 0) - return -1; + if ((error = git_buf_init(&idx_name, name_len)) < 0) + goto cleanup; git_buf_put(&idx_name, p->pack_name, name_len - strlen(".pack")); git_buf_puts(&idx_name, ".idx"); if (git_buf_oom(&idx_name)) { - git_buf_dispose(&idx_name); - return -1; + error = -1; + goto cleanup; } if (p->index_version == -1) error = pack_index_check_locked(idx_name.ptr, p); +cleanup: git_buf_dispose(&idx_name); return error; @@ -341,8 +342,20 @@ static unsigned char *pack_window_open( off64_t offset, unsigned int *left) { - if (p->mwf.fd == -1 && packfile_open(p) < 0) + unsigned char *pack_data = NULL; + + if (git_mutex_lock(&p->lock) < 0) { + git_error_set(GIT_ERROR_THREAD, "unable to lock packfile"); return NULL; + } + if (git_mutex_lock(&p->mwf.lock) < 0) { + git_mutex_unlock(&p->lock); + git_error_set(GIT_ERROR_THREAD, "unable to lock packfile"); + return NULL; + } + + if (p->mwf.fd == -1 && packfile_open_locked(p) < 0) + goto cleanup; /* Since packfiles end in a hash of their content and it's * pointless to ask for an offset into the middle of that @@ -353,11 +366,16 @@ static unsigned char *pack_window_open( * around. */ if (offset > (p->mwf.size - 20)) - return NULL; + goto cleanup; if (offset < 0) - return NULL; + goto cleanup; + + pack_data = git_mwindow_open(&p->mwf, w_cursor, offset, 20, left); - return git_mwindow_open(&p->mwf, w_cursor, offset, 20, left); +cleanup: + git_mutex_unlock(&p->mwf.lock); + git_mutex_unlock(&p->lock); + return pack_data; } /* @@ -433,14 +451,27 @@ static int packfile_unpack_header1( int git_packfile_unpack_header( size_t *size_p, git_object_t *type_p, - git_mwindow_file *mwf, + struct git_pack_file *p, git_mwindow **w_curs, off64_t *curpos) { unsigned char *base; unsigned int left; unsigned long used; - int ret; + int error; + + if ((error = git_mutex_lock(&p->lock)) < 0) + return error; + if ((error = git_mutex_lock(&p->mwf.lock)) < 0) { + git_mutex_unlock(&p->lock); + return error; + } + + if (p->mwf.fd == -1 && (error = packfile_open_locked(p)) < 0) { + git_mutex_unlock(&p->lock); + git_mutex_unlock(&p->mwf.lock); + return error; + } /* pack_window_open() assures us we have [base, base + 20) available * as a range that we can look at at. (Its actually the hash @@ -448,16 +479,17 @@ int git_packfile_unpack_header( * the maximum deflated object size is 2^137, which is just * insane, so we know won't exceed what we have been given. */ -/* base = pack_window_open(p, w_curs, *curpos, &left); */ - base = git_mwindow_open(mwf, w_curs, *curpos, 20, &left); + base = git_mwindow_open(&p->mwf, w_curs, *curpos, 20, &left); + git_mutex_unlock(&p->lock); + git_mutex_unlock(&p->mwf.lock); if (base == NULL) return GIT_EBUFS; - ret = packfile_unpack_header1(&used, size_p, type_p, base, left); + error = packfile_unpack_header1(&used, size_p, type_p, base, left); git_mwindow_close(w_curs); - if (ret == GIT_EBUFS) - return ret; - else if (ret < 0) + if (error == GIT_EBUFS) + return error; + else if (error < 0) return packfile_error("header length is zero"); *curpos += used; @@ -477,10 +509,27 @@ int git_packfile_resolve_header( off64_t base_offset; int error; - if (p->mwf.fd == -1 && (error = packfile_open(p)) < 0) + error = git_mutex_lock(&p->lock); + if (error < 0) { + git_error_set(GIT_ERROR_OS, "failed to lock packfile reader"); + return error; + } + error = git_mutex_lock(&p->mwf.lock); + if (error < 0) { + git_error_set(GIT_ERROR_OS, "failed to lock packfile reader"); + git_mutex_unlock(&p->lock); + return error; + } + + if (p->mwf.fd == -1 && (error = packfile_open_locked(p)) < 0) { + git_mutex_unlock(&p->mwf.lock); + git_mutex_unlock(&p->lock); return error; + } + git_mutex_unlock(&p->mwf.lock); + git_mutex_unlock(&p->lock); - error = git_packfile_unpack_header(&size, &type, &p->mwf, &w_curs, &curpos); + error = git_packfile_unpack_header(&size, &type, p, &w_curs, &curpos); if (error < 0) return error; @@ -507,7 +556,7 @@ int git_packfile_resolve_header( while (type == GIT_OBJECT_OFS_DELTA || type == GIT_OBJECT_REF_DELTA) { curpos = base_offset; - error = git_packfile_unpack_header(&size, &type, &p->mwf, &w_curs, &curpos); + error = git_packfile_unpack_header(&size, &type, p, &w_curs, &curpos); if (error < 0) return error; if (type != GIT_OBJECT_OFS_DELTA && type != GIT_OBJECT_REF_DELTA) @@ -578,8 +627,7 @@ static int pack_dependency_chain(git_dependency_chain *chain_out, elem->base_key = obj_offset; - error = git_packfile_unpack_header(&size, &type, &p->mwf, &w_curs, &curpos); - + error = git_packfile_unpack_header(&size, &type, p, &w_curs, &curpos); if (error < 0) goto on_error; @@ -630,7 +678,23 @@ int git_packfile_unpack( size_t stack_size = 0, elem_pos, alloclen; git_object_t base_type; - if (p->mwf.fd == -1 && (error = packfile_open(p)) < 0) + error = git_mutex_lock(&p->lock); + if (error < 0) { + git_error_set(GIT_ERROR_OS, "failed to lock packfile reader"); + return error; + } + error = git_mutex_lock(&p->mwf.lock); + if (error < 0) { + git_error_set(GIT_ERROR_OS, "failed to lock packfile reader"); + git_mutex_unlock(&p->lock); + return error; + } + + if (p->mwf.fd == -1) + error = packfile_open_locked(p); + git_mutex_unlock(&p->mwf.lock); + git_mutex_unlock(&p->lock); + if (error < 0) return error; /* @@ -974,64 +1038,63 @@ int get_delta_base( * ***********************************************************/ -void git_packfile_close(struct git_pack_file *p, bool unlink_packfile) +void git_packfile_free(struct git_pack_file *p, bool unlink_packfile) { + bool locked = true; + + if (!p) + return; + + cache_free(&p->bases); + + if (git_mutex_lock(&p->lock) < 0) { + git_error_set(GIT_ERROR_OS, "failed to lock packfile"); + locked = false; + } if (p->mwf.fd >= 0) { - git_mwindow_free_all_locked(&p->mwf); + git_mwindow_free_all(&p->mwf); p_close(p->mwf.fd); p->mwf.fd = -1; } + if (locked) + git_mutex_unlock(&p->lock); if (unlink_packfile) p_unlink(p->pack_name); -} - -void git_packfile_free(struct git_pack_file *p) -{ - if (!p) - return; - - cache_free(&p->bases); - - git_packfile_close(p, false); pack_index_free(p); git__free(p->bad_object_sha1); - git_mutex_free(&p->lock); git_mutex_free(&p->bases.lock); + git_mutex_free(&p->mwf.lock); + git_mutex_free(&p->lock); git__free(p); } -static int packfile_open(struct git_pack_file *p) +/* Run with the packfile and mwf locks held */ +static int packfile_open_locked(struct git_pack_file *p) { struct stat st; struct git_pack_header hdr; git_oid sha1; unsigned char *idx_sha1; - 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); + if (pack_index_open_locked(p) < 0) return git_odb__error_notfound("failed to open packfile", NULL, 0); - } - if (p->mwf.fd >= 0) { - git_mutex_unlock(&p->lock); + if (p->mwf.fd >= 0) return 0; - } /* TODO: open with noatime */ p->mwf.fd = git_futils_open_ro(p->pack_name); if (p->mwf.fd < 0) goto cleanup; - if (p_fstat(p->mwf.fd, &st) < 0 || - git_mwindow_file_register(&p->mwf) < 0) + if (p_fstat(p->mwf.fd, &st) < 0) { + git_error_set(GIT_ERROR_OS, "could not stat packfile"); goto cleanup; + } /* If we created the struct before we had the pack we lack size. */ if (!p->mwf.size) { @@ -1071,7 +1134,9 @@ static int packfile_open(struct git_pack_file *p) if (git_oid__cmp(&sha1, (git_oid *)idx_sha1) != 0) goto cleanup; - git_mutex_unlock(&p->lock); + if (git_mwindow_file_register(&p->mwf) < 0) + goto cleanup; + return 0; cleanup: @@ -1081,8 +1146,6 @@ cleanup: p_close(p->mwf.fd); p->mwf.fd = -1; - git_mutex_unlock(&p->lock); - return -1; } @@ -1152,13 +1215,22 @@ int git_packfile_alloc(struct git_pack_file **pack_out, const char *path) p->mtime = (git_time_t)st.st_mtime; p->index_version = -1; - if (git_mutex_init(&p->lock)) { + if (git_mutex_init(&p->lock) < 0) { git_error_set(GIT_ERROR_OS, "failed to initialize packfile mutex"); git__free(p); return -1; } + if (git_mutex_init(&p->mwf.lock) < 0) { + git_error_set(GIT_ERROR_OS, "failed to initialize packfile window mutex"); + git_mutex_free(&p->lock); + git__free(p); + return -1; + } + if (cache_init(&p->bases) < 0) { + git_mutex_free(&p->mwf.lock); + git_mutex_free(&p->lock); git__free(p); return -1; } @@ -1174,28 +1246,29 @@ int git_packfile_alloc(struct git_pack_file **pack_out, const char *path) * ***********************************************************/ -static off64_t nth_packed_object_offset(const struct git_pack_file *p, uint32_t n) +static off64_t nth_packed_object_offset_locked(struct git_pack_file *p, uint32_t n) { - const unsigned char *index = p->index_map.data; - const unsigned char *end = index + p->index_map.len; + const unsigned char *index, *end; + uint32_t off32; + + index = p->index_map.data; + end = index + p->index_map.len; index += 4 * 256; - if (p->index_version == 1) { + if (p->index_version == 1) return ntohl(*((uint32_t *)(index + 24 * n))); - } else { - uint32_t off; - index += 8 + p->num_objects * (20 + 4); - off = ntohl(*((uint32_t *)(index + 4 * n))); - if (!(off & 0x80000000)) - return off; - index += p->num_objects * 4 + (off & 0x7fffffff) * 8; - - /* Make sure we're not being sent out of bounds */ - if (index >= end - 8) - return -1; - return (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) | - ntohl(*((uint32_t *)(index + 4))); - } + index += 8 + p->num_objects * (20 + 4); + off32 = ntohl(*((uint32_t *)(index + 4 * n))); + if (!(off32 & 0x80000000)) + return off32; + index += p->num_objects * 4 + (off32 & 0x7fffffff) * 8; + + /* Make sure we're not being sent out of bounds */ + if (index >= end - 8) + return -1; + + return (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) | + ntohl(*((uint32_t *)(index + 4))); } static int git__memcmp4(const void *a, const void *b) { @@ -1396,7 +1469,7 @@ static int pack_entry_find_offset( goto cleanup; } - if ((offset = nth_packed_object_offset(p, pos)) < 0) { + if ((offset = nth_packed_object_offset_locked(p, pos)) < 0) { git_error_set(GIT_ERROR_ODB, "packfile index is corrupt"); error = -1; goto cleanup; @@ -1442,10 +1515,26 @@ int git_pack_entry_find( if (error < 0) return error; + error = git_mutex_lock(&p->lock); + if (error < 0) { + git_error_set(GIT_ERROR_OS, "failed to lock packfile reader"); + return error; + } + error = git_mutex_lock(&p->mwf.lock); + if (error < 0) { + git_mutex_unlock(&p->lock); + git_error_set(GIT_ERROR_OS, "failed to lock packfile reader"); + return error; + } + /* we found a unique entry in the index; * make sure the packfile backing the index * still exists on disk */ - if (p->mwf.fd == -1 && (error = packfile_open(p)) < 0) + if (p->mwf.fd == -1) + error = packfile_open_locked(p); + git_mutex_unlock(&p->mwf.lock); + git_mutex_unlock(&p->lock); + if (error < 0) return error; e->offset = offset; diff --git a/src/pack.h b/src/pack.h index 544a5d286..7c3be079e 100644 --- a/src/pack.h +++ b/src/pack.h @@ -85,7 +85,7 @@ typedef struct { struct git_pack_file { git_mwindow_file mwf; git_map index_map; - git_mutex lock; /* protect updates to mwf and index_map */ + git_mutex lock; /* protect updates to index_map */ git_atomic refcount; uint32_t num_objects; @@ -140,7 +140,7 @@ int git_packfile__name(char **out, const char *path); int git_packfile_unpack_header( size_t *size_p, git_object_t *type_p, - git_mwindow_file *mwf, + struct git_pack_file *p, git_mwindow **w_curs, off64_t *curpos); @@ -164,8 +164,7 @@ int get_delta_base( git_object_t type, off64_t delta_obj_offset); -void git_packfile_close(struct git_pack_file *p, bool unlink_packfile); -void git_packfile_free(struct git_pack_file *p); +void git_packfile_free(struct git_pack_file *p, bool unlink_packfile); int git_packfile_alloc(struct git_pack_file **pack_out, const char *path); int git_pack_entry_find( diff --git a/src/unix/map.c b/src/unix/map.c index 62c52578b..88f283ce8 100644 --- a/src/unix/map.c +++ b/src/unix/map.c @@ -68,6 +68,8 @@ int p_munmap(git_map *map) { GIT_ASSERT_ARG(map); munmap(map->data, map->len); + map->data = NULL; + map->len = 0; return 0; } diff --git a/tests/pack/filelimit.c b/tests/pack/filelimit.c index b39ab7327..5e0b77e9b 100644 --- a/tests/pack/filelimit.c +++ b/tests/pack/filelimit.c @@ -8,6 +8,7 @@ static size_t expected_open_mwindow_files = 0; static size_t original_mwindow_file_limit = 0; +extern git_mutex git__mwindow_mutex; extern git_mwindow_ctl git_mwindow__mem_ctl; void test_pack_filelimit__initialize_tiny(void) diff --git a/tests/pack/threadsafety.c b/tests/pack/threadsafety.c new file mode 100644 index 000000000..0b479788f --- /dev/null +++ b/tests/pack/threadsafety.c @@ -0,0 +1,60 @@ +#include "clar_libgit2.h" +#include "pool.h" + +#include <git2.h> +#include "git2/sys/commit.h" +#include "git2/sys/mempack.h" + +static size_t original_mwindow_file_limit = 0; + +void test_pack_threadsafety__initialize(void) +{ + size_t open_mwindow_files = 1; + + cl_git_pass(git_libgit2_opts(GIT_OPT_GET_MWINDOW_FILE_LIMIT, &original_mwindow_file_limit)); + cl_git_pass(git_libgit2_opts(GIT_OPT_SET_MWINDOW_FILE_LIMIT, open_mwindow_files)); +} + +void test_pack_threadsafety__cleanup(void) +{ + cl_git_pass(git_libgit2_opts(GIT_OPT_SET_MWINDOW_FILE_LIMIT, original_mwindow_file_limit)); +} + +static void *get_status(void *arg) +{ + const char *repo_path = (const char *)arg; + git_repository *repo; + git_status_list *status; + + cl_git_pass(git_repository_open(&repo, repo_path)); + cl_git_pass(git_status_list_new(&status, repo, NULL)); + git_status_list_free(status); + git_repository_free(repo); + + return NULL; +} + +void test_pack_threadsafety__open_repo_in_multiple_threads(void) +{ +#ifdef GIT_THREADS + const char *repo_path = cl_fixture("../.."); + git_repository *repo; + git_thread threads[8]; + size_t i; + + /* If we can't open the libgit2 repo or if it isn't a full repo + * with proper history, just skip this test */ + if (git_repository_open(&repo, repo_path) < 0) + cl_skip(); + if (git_repository_is_shallow(repo)) + cl_skip(); + git_repository_free(repo); + + for (i = 0; i < ARRAY_SIZE(threads); i++) + git_thread_create(&threads[i], get_status, (void *)repo_path); + for (i = 0; i < ARRAY_SIZE(threads); i++) + git_thread_join(&threads[i], NULL); +#else + cl_skip(); +#endif +} |