Windows: not safe to dereference before locking

Throughout cm_server.c, input parameters to functions that
are protected by cm_serverLock are dereferenced by assignment
during variable initialization prior to the cm_serverLock being
obtained.  As a result there is a race which can result in
either list corruption or dereferencing freed memory.

Reviewed-on: http://gerrit.openafs.org/4985
Reviewed-by: Jeffrey Altman <jaltman@openafs.org>
Tested-by: Jeffrey Altman <jaltman@openafs.org>
(cherry picked from commit 130155ff3c48f2da2433b359588346b4438d24a2)

Change-Id: I02e83faa889bb55b025253bbd1c51a389434eee4
Reviewed-on: http://gerrit.openafs.org/5014
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Derrick Brashear <shadow@dementia.org>
Tested-by: Derrick Brashear <shadow@dementia.org>

diff --git a/src/WINNT/afsd/cm_server.c b/src/WINNT/afsd/cm_server.c
index 81716d9fee40073283bef99cae9fab68124e08ae..be8ff4cc3841f1918aef2ac65740550471eda698 100644 (file)
--- a/src/WINNT/afsd/cm_server.c
+++ b/src/WINNT/afsd/cm_server.c
@@ -1036,10 +1036,13 @@ LONG_PTR cm_ChecksumServerList(cm_serverRef_t *serversp)
 */
 void cm_InsertServerList(cm_serverRef_t** list, cm_serverRef_t* element)
 {
-    cm_serverRef_t     *current=*list;
-    unsigned short ipRank = element->server->ipRank;
+    cm_serverRef_t     *current;
+    unsigned short ipRank;
 
     lock_ObtainWrite(&cm_serverLock);
+    current=*list;
+    ipRank = element->server->ipRank;
+
     element->refCount++;                /* increase refCount */
 
     /* insertion into empty list  or at the beginning of the list */
@@ -1068,14 +1071,19 @@ void cm_InsertServerList(cm_serverRef_t** list, cm_serverRef_t* element)
 */
 long cm_ChangeRankServer(cm_serverRef_t** list, cm_server_t*   server)
 {
-    cm_serverRef_t  **current=list;
-    cm_serverRef_t     *element=0;
+    cm_serverRef_t  **current;
+    cm_serverRef_t   *element;
+
+    lock_ObtainWrite(&cm_serverLock);
+    current=list;
+    element=0;
 
     /* if there is max of one element in the list, nothing to sort */
-    if ( (!*current) || !((*current)->next)  )
+    if ( (!*current) || !((*current)->next)  ) {
+        lock_ReleaseWrite(&cm_serverLock);
         return 1;              /* list unchanged: return success */
+    }
 
-    lock_ObtainWrite(&cm_serverLock);
     /* if the server is on the list, delete it from list */
     while ( *current )
     {
@@ -1109,14 +1117,17 @@ long cm_ChangeRankServer(cm_serverRef_t** list, cm_server_t*    server)
 void cm_RandomizeServer(cm_serverRef_t** list)
 {
     int                count, picked;
-    cm_serverRef_t*    tsrp = *list, *lastTsrp;
+    cm_serverRef_t*    tsrp, *lastTsrp;
     unsigned short     lowestRank;
 
+    lock_ObtainWrite(&cm_serverLock);
+    tsrp = *list;
+
     /* an empty list or a list with only one element */
-    if ( !tsrp || ! tsrp->next )
+    if ( !tsrp || ! tsrp->next ) {
+        lock_ReleaseWrite(&cm_serverLock);
         return ;
-
-    lock_ObtainWrite(&cm_serverLock);
+    }
 
     /* count the number of servers with the lowest rank */
     lowestRank = tsrp->server->ipRank;
@@ -1238,11 +1249,14 @@ void cm_RemoveVolumeFromServer(cm_server_t * serverp, afs_uint32 volID)
 
 void cm_FreeServerList(cm_serverRef_t** list, afs_uint32 flags)
 {
-    cm_serverRef_t  **current = list;
-    cm_serverRef_t  **nextp = 0;
-    cm_serverRef_t  * next = 0;
+    cm_serverRef_t  **current;
+    cm_serverRef_t  **nextp;
+    cm_serverRef_t  * next;
 
     lock_ObtainWrite(&cm_serverLock);
+    current = list;
+    nextp = 0;
+    next = 0;
 
     if (*list == NULL)
         goto done;