From c3320aca7640386a2cfc0c785560ff4d36851ef9 Mon Sep 17 00:00:00 2001 From: Justin Spahr-Summers Date: Sun, 9 Dec 2012 02:22:50 -0800 Subject: [PATCH 1/4] git__mwindow_mutex needs to be initialized even with pthreads This could also use PTHREAD_MUTEX_INITIALIZER, but a dynamic initializer seems like a more portable concept, and we won't need another #define on top of git_mutex_init() --- src/global.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/global.c b/src/global.c index d085089c3..305ec2edd 100644 --- a/src/global.c +++ b/src/global.c @@ -119,6 +119,7 @@ int git_threads_init(void) if (_tls_init) return 0; + git_mutex_init(&git__mwindow_mutex); pthread_key_create(&_tls_key, &cb__free_status); /* Initialize any other subsystems that have global state */ @@ -134,6 +135,7 @@ void git_threads_shutdown(void) { pthread_key_delete(_tls_key); _tls_init = 0; + git_mutex_free(&git__mwindow_mutex); /* Shut down any subsystems that have global state */ git_hash_global_shutdown(); From a35b3864583bd1c98334aa71bb4e4c217ace55aa Mon Sep 17 00:00:00 2001 From: Justin Spahr-Summers Date: Sun, 9 Dec 2012 02:31:39 -0800 Subject: [PATCH 2/4] Always check the result of git_mutex_lock --- src/cache.c | 18 +++++++++++++----- src/mwindow.c | 29 ++++++++++++++++++++++++----- src/pack-objects.c | 18 ++++++++++++++---- 3 files changed, 51 insertions(+), 14 deletions(-) diff --git a/src/cache.c b/src/cache.c index edd3a47dd..67801758d 100644 --- a/src/cache.c +++ b/src/cache.c @@ -52,7 +52,11 @@ void *git_cache_get(git_cache *cache, const git_oid *oid) memcpy(&hash, oid->id, sizeof(hash)); - git_mutex_lock(&cache->lock); + if (git_mutex_lock(&cache->lock)) { + giterr_set(GITERR_THREAD, "unable to lock cache mutex"); + return NULL; + } + { node = cache->nodes[hash & cache->size_mask]; @@ -73,12 +77,16 @@ void *git_cache_try_store(git_cache *cache, void *_entry) memcpy(&hash, &entry->oid, sizeof(uint32_t)); - /* increase the refcount on this object, because - * the cache now owns it */ - git_cached_obj_incref(entry); + if (git_mutex_lock(&cache->lock)) { + giterr_set(GITERR_THREAD, "unable to lock cache mutex"); + return NULL; + } - git_mutex_lock(&cache->lock); { + /* increase the refcount on this object, because + * the cache now owns it */ + git_cached_obj_incref(entry); + git_cached_obj *node = cache->nodes[hash & cache->size_mask]; if (node == NULL) { diff --git a/src/mwindow.c b/src/mwindow.c index 4da5badb6..ee693e4a0 100644 --- a/src/mwindow.c +++ b/src/mwindow.c @@ -44,7 +44,10 @@ void git_mwindow_free_all(git_mwindow_file *mwf) git_mwindow_ctl *ctl = &mem_ctl; unsigned int i; - git_mutex_lock(&git__mwindow_mutex); + if (git_mutex_lock(&git__mwindow_mutex)) { + giterr_set(GITERR_THREAD, "unable to lock mwindow mutex"); + return; + } /* * Remove these windows from the global list @@ -221,7 +224,11 @@ unsigned char *git_mwindow_open( git_mwindow_ctl *ctl = &mem_ctl; git_mwindow *w = *cursor; - git_mutex_lock(&git__mwindow_mutex); + if (git_mutex_lock(&git__mwindow_mutex)) { + giterr_set(GITERR_THREAD, "unable to lock mwindow mutex"); + return NULL; + } + if (!w || !(git_mwindow_contains(w, offset) && git_mwindow_contains(w, offset + extra))) { if (w) { w->inuse_cnt--; @@ -269,7 +276,11 @@ int git_mwindow_file_register(git_mwindow_file *mwf) git_mwindow_ctl *ctl = &mem_ctl; int ret; - git_mutex_lock(&git__mwindow_mutex); + if (git_mutex_lock(&git__mwindow_mutex)) { + giterr_set(GITERR_THREAD, "unable to lock mwindow mutex"); + return -1; + } + if (ctl->windowfiles.length == 0 && git_vector_init(&ctl->windowfiles, 8, NULL) < 0) { git_mutex_unlock(&git__mwindow_mutex); @@ -288,7 +299,11 @@ int git_mwindow_file_deregister(git_mwindow_file *mwf) git_mwindow_file *cur; unsigned int i; - git_mutex_lock(&git__mwindow_mutex); + if (git_mutex_lock(&git__mwindow_mutex)) { + giterr_set(GITERR_THREAD, "unable to lock mwindow mutex"); + return -1; + } + git_vector_foreach(&ctl->windowfiles, i, cur) { if (cur == mwf) { git_vector_remove(&ctl->windowfiles, i); @@ -306,7 +321,11 @@ void git_mwindow_close(git_mwindow **window) { git_mwindow *w = *window; if (w) { - git_mutex_lock(&git__mwindow_mutex); + if (git_mutex_lock(&git__mwindow_mutex)) { + giterr_set(GITERR_THREAD, "unable to lock mwindow mutex"); + return; + } + w->inuse_cnt--; git_mutex_unlock(&git__mwindow_mutex); *window = NULL; diff --git a/src/pack-objects.c b/src/pack-objects.c index 44ad3fd98..2eb69764d 100644 --- a/src/pack-objects.c +++ b/src/pack-objects.c @@ -1025,6 +1025,14 @@ static void *threaded_find_deltas(void *arg) git_cond_signal(&me->pb->progress_cond); git_packbuilder__progress_unlock(me->pb); + if (git_mutex_lock(&me->mutex)) { + giterr_set(GITERR_THREAD, "unable to lock packfile condition mutex"); + return NULL; + } + + while (!me->data_ready) + git_cond_wait(&me->cond, &me->mutex); + /* * We must not set ->data_ready before we wait on the * condition because the main thread may have set it to 1 @@ -1033,9 +1041,6 @@ static void *threaded_find_deltas(void *arg) * was initialized to 0 before this thread was spawned * and we reset it to 0 right away. */ - git_mutex_lock(&me->mutex); - while (!me->data_ready) - git_cond_wait(&me->cond, &me->mutex); me->data_ready = 0; git_mutex_unlock(&me->mutex); } @@ -1168,7 +1173,12 @@ static int ll_find_deltas(git_packbuilder *pb, git_pobject **list, target->working = 1; git_packbuilder__progress_unlock(pb); - git_mutex_lock(&target->mutex); + if (git_mutex_lock(&target->mutex)) { + giterr_set(GITERR_THREAD, "unable to lock packfile condition mutex"); + git__free(p); + return -1; + } + target->data_ready = 1; git_cond_signal(&target->cond); git_mutex_unlock(&target->mutex); From 2bb1c7aa2629e1e4e3a5f04d494b16957da849e1 Mon Sep 17 00:00:00 2001 From: Justin Spahr-Summers Date: Sun, 9 Dec 2012 02:37:33 -0800 Subject: [PATCH 3/4] Treat git_mutex_lock as successful when threads are disabled --- src/thread-utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/thread-utils.h b/src/thread-utils.h index f0a51f28c..d747a0d30 100644 --- a/src/thread-utils.h +++ b/src/thread-utils.h @@ -79,7 +79,7 @@ GIT_INLINE(int) git_atomic_dec(git_atomic *a) /* Pthreads Mutex */ #define git_mutex unsigned int #define git_mutex_init(a) (void)0 -#define git_mutex_lock(a) (void)0 +#define git_mutex_lock(a) 0 #define git_mutex_unlock(a) (void)0 #define git_mutex_free(a) (void)0 From 1d009603343786f6022ef690e513f183c5840747 Mon Sep 17 00:00:00 2001 From: Justin Spahr-Summers Date: Sun, 9 Dec 2012 02:40:16 -0800 Subject: [PATCH 4/4] orite C89 --- src/cache.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cache.c b/src/cache.c index 67801758d..cbd360a02 100644 --- a/src/cache.c +++ b/src/cache.c @@ -83,12 +83,12 @@ void *git_cache_try_store(git_cache *cache, void *_entry) } { + git_cached_obj *node = cache->nodes[hash & cache->size_mask]; + /* increase the refcount on this object, because * the cache now owns it */ git_cached_obj_incref(entry); - git_cached_obj *node = cache->nodes[hash & cache->size_mask]; - if (node == NULL) { cache->nodes[hash & cache->size_mask] = entry; } else if (git_oid_cmp(&node->oid, &entry->oid) == 0) {