From b6f7e0da8b51eede9fe2face61c54f090e9eac17 Mon Sep 17 00:00:00 2001 From: gcasa Date: Thu, 25 Sep 2008 04:55:42 +0000 Subject: [PATCH] * Source/synchronization.m: Correct issue in objc_sync_remove_node() method where it could do a NULL dereference. Also moved the locks on the tables to the highest level so to reduce the possibility of threading issues. git-svn-id: svn+ssh://svn.gna.org/svn/gnustep/libs/base/trunk@26877 72102866-910b-0410-8b05-ffd578937521 --- ChangeLog | 7 ++++++ Source/synchronization.m | 47 +++++++++++++++++++++++++++++----------- 2 files changed, 41 insertions(+), 13 deletions(-) diff --git a/ChangeLog b/ChangeLog index 2db60cd02..6b0ef87ec 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2008-09-25 00:18-EDT Gregory John Casamento + + * Source/synchronization.m: Correct issue in objc_sync_remove_node() + method where it could do a NULL dereference. Also moved the locks on + the tables to the highest level so to reduce the possibility of + threading issues. + 2008-09-24 17:26-EDT Gregory John Casamento * Source/NSUserDefaults.m: Corrected previous change. diff --git a/Source/synchronization.m b/Source/synchronization.m index 90d267ecd..752a7ae3e 100644 --- a/Source/synchronization.m +++ b/Source/synchronization.m @@ -74,7 +74,7 @@ static void sync_lock_init() */ lock_node_t* objc_sync_find_node(id obj) { - lock_node_t *current = NULL; + lock_node_t *current = lock_list; if(lock_list != NULL) { @@ -105,9 +105,6 @@ lock_node_t* objc_sync_add_node(id obj) // get the lock... sync_lock_init(); - // restrict access to the table.... - objc_mutex_lock(table_lock); - // if the list hasn't been initialized, initialize it. if(lock_list == NULL) { @@ -154,9 +151,6 @@ lock_node_t* objc_sync_add_node(id obj) current->lock = objc_mutex_allocate(); } - // release access to the table... - objc_mutex_unlock(table_lock); - return current; } @@ -182,10 +176,17 @@ lock_node_t* objc_sync_remove_node(id obj) prev = curr->prev; next = curr->next; - next->prev = prev; - prev->next = next; + if(next != NULL) + { + next->prev = prev; + } + + if(prev != NULL) + { + prev->next = next; + } } - + // return the removed node... return curr; } @@ -198,6 +199,9 @@ int objc_sync_enter(id obj) lock_node_t *node = NULL; int status = 0; + // lock access to the table until we're done.... + objc_mutex_lock(table_lock); + node = objc_sync_find_node(obj); if(node == NULL) { @@ -208,8 +212,15 @@ int objc_sync_enter(id obj) } } + // unlock the table.... + objc_mutex_unlock(table_lock); + + status = objc_mutex_lock(node->lock); - if(status < 1) + // if the status is more than one, then another thread + // has this section locked, so we abort. A status of -1 + // indicates that an error occurred. + if(status > 1 || status == -1) { return OBJC_SYNC_NOT_OWNING_THREAD_ERROR; } @@ -225,20 +236,30 @@ int objc_sync_exit(id obj) lock_node_t *node = NULL; int status = 0; - node = objc_sync_remove_node(obj); + // lock access to the table until we're done.... + objc_mutex_lock(table_lock); + + node = objc_sync_find_node(obj); if(node == NULL) { return OBJC_SYNC_NOT_INITIALIZED; } + // unlock the table.... + objc_mutex_unlock(table_lock); + status = objc_mutex_unlock(node->lock); - if(status < 1) + // if the status is not zero, then we are not the sole + // owner of this node. Also if -1 is returned, this indicates and error + // condition. + if(status > 0 || status == -1) { return OBJC_SYNC_NOT_OWNING_THREAD_ERROR; } // dealloc the node and return success. free(node); + return OBJC_SYNC_SUCCESS; }