From: Jeffrey Altman <jaltman@your-file-system.com>
Date: Wed, 13 Jul 2011 12:15:04 +0000 (-0400)
Subject: Windows: not safe to dereference before locking
X-Git-Tag: upstream/1.6.0.pre7^2~8
X-Git-Url: https://git.michaelhowe.org/gitweb/?a=commitdiff_plain;h=5452fa62c98b08b5ec3a85d2b7c08585294d4906;p=packages%2Fo%2Fopenafs.git

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 81716d9fe..be8ff4cc3 100644
--- 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;