lib/lock: assertion failure due to MXUser condVar

If an attempt was made to acquire the lock involved
with a condVar (on one thread) while the lock had been freed as part
of a condVar wait (on another thread), an assertion failure would
occur on the acquisition.

The problem is caused by the implicit unlocking and relocking of the
lock involved with the condVar. The MXUser lock reference count was
not in sync with the locked/unlocked state. Decrement and increment
the reference count before/after the native condVar wait.

Signed-off-by: Marcelo Vanzin <mvanzin@vmware.com>

diff --git a/open-vm-tools/lib/lock/ulCondVar.c b/open-vm-tools/lib/lock/ulCondVar.c
index a0835321fbea4c9a7539dd5100b795e67c26cf27..591a639caa298de7e203e706a25c4c5b8827243c 100644 (file)
--- a/open-vm-tools/lib/lock/ulCondVar.c
+++ b/open-vm-tools/lib/lock/ulCondVar.c
@@ -238,11 +238,21 @@ MXUserWaitCondVar(MXUserHeader *header,    // IN:
 
    Atomic_Inc(&condVar->referenceCount);
 
+   /*
+    * When using the native lock found within the MXUser lock, be sure to
+    * decrement the count before the wait/sleep and increment it after the
+    * wait/sleep - the (native) wait/sleep will perform a lock release before
+    * the wait/sleep and a lock acquisition after the wait/sleep. The
+    * MXUser internal accounting information must be maintained.
+    */
+
 #if defined(_WIN32)
    if (pSleepConditionVariableCS) {
+      MXRecLockDecCount(lock);
       err = (*pSleepConditionVariableCS)(&condVar->x.condObject,
                                          &lock->nativeLock, INFINITE) ?
                                          0 : GetLastError();
+      MXRecLockIncCount(lock, GetReturnAddress());
    } else {
       Bool done = FALSE;
 
@@ -277,7 +287,9 @@ MXUserWaitCondVar(MXUserHeader *header,    // IN:
       err = 0;
    }
 #else
+   MXRecLockDecCount(lock);
    err = pthread_cond_wait(&condVar->condObject, &lock->nativeLock);
+   MXRecLockIncCount(lock, GetReturnAddress());
 #endif
 
    if (err != 0) {

diff --git a/open-vm-tools/lib/lock/ulInt.h b/open-vm-tools/lib/lock/ulInt.h
index 074aaca728798697a9d9fd2464c3bcd7ca7b70cc..4c4a765507d9b197d48df24bb4290175e7b9d919 100644 (file)
--- a/open-vm-tools/lib/lock/ulInt.h
+++ b/open-vm-tools/lib/lock/ulInt.h
@@ -246,6 +246,32 @@ MXRecLockDestroy(MXRecLock *lock)  // IN/OUT:
    if (vmx86_debug && (err != 0)) {
       Panic("%s: MXRecLockDestroyInternal returned %d\n", __FUNCTION__, err);
    }
+} 
+
+
+static INLINE uint32
+MXRecLockCount(const MXRecLock *lock)  // IN:
+{
+   return lock->referenceCount;
+}
+
+
+static INLINE void
+MXRecLockIncCount(MXRecLock *lock,  // IN/OUT:
+                  void *location)   // IN:
+{
+   if (MXRecLockCount(lock) == 0) {
+#if defined(MXUSER_DEBUG)
+      ASSERT(lock->portableThreadID == VTHREAD_INVALID_ID);
+
+      lock->ownerRetAddr = location;
+      lock->portableThreadID = VThread_CurID();
+#endif
+
+      MXRecLockSetOwner(lock);
+   }
+
+   lock->referenceCount++;
 }
 
 
@@ -255,11 +281,9 @@ MXRecLockAcquire(MXRecLock *lock,  // IN/OUT:
 {
    Bool contended;
 
-   if ((lock->referenceCount != 0) && MXRecLockIsOwner(lock)) {
-      ASSERT((lock->referenceCount > 0) &&
-             (lock->referenceCount < MXUSER_MAX_REC_DEPTH));
-
-      lock->referenceCount++;
+   if ((MXRecLockCount(lock) != 0) && MXRecLockIsOwner(lock)) {
+      ASSERT((MXRecLockCount(lock) > 0) &&
+             (MXRecLockCount(lock) < MXUSER_MAX_REC_DEPTH));
 
       contended = FALSE;
    } else {
@@ -283,18 +307,10 @@ MXRecLockAcquire(MXRecLock *lock,  // IN/OUT:
       }
 
       ASSERT(lock->referenceCount == 0);
-
-#if defined(MXUSER_DEBUG)
-      ASSERT(lock->portableThreadID == VTHREAD_INVALID_ID);
-
-      lock->ownerRetAddr = location;
-      lock->portableThreadID = VThread_CurID();
-#endif
-
-      MXRecLockSetOwner(lock);
-      lock->referenceCount = 1;
    }
 
+   MXRecLockIncCount(lock, location);
+
    return contended;
 }
 
@@ -309,21 +325,10 @@ MXRecLockTryAcquire(MXRecLock *lock,  // IN/OUT:
    err = MXRecLockTryAcquireInternal(lock);
 
    if (err == 0) {
-      ASSERT((lock->referenceCount >= 0) &&
-             (lock->referenceCount < MXUSER_MAX_REC_DEPTH));
-
-      if (lock->referenceCount == 0) {
-#if defined(MXUSER_DEBUG)
-         ASSERT(lock->portableThreadID == VTHREAD_INVALID_ID);
-
-         lock->ownerRetAddr = location;
-         lock->portableThreadID = VThread_CurID();
-#endif
-
-         MXRecLockSetOwner(lock);
-      }
+      ASSERT((MXRecLockCount(lock) > 0) &&
+             (MXRecLockCount(lock) < MXUSER_MAX_REC_DEPTH));
 
-      lock->referenceCount++;
+      MXRecLockIncCount(lock, location);
 
       acquired = TRUE;
    } else {
@@ -338,26 +343,32 @@ MXRecLockTryAcquire(MXRecLock *lock,  // IN/OUT:
    return acquired;
 }
 
-
 static INLINE void
-MXRecLockRelease(MXRecLock *lock)  // IN/OUT:
+MXRecLockDecCount(MXRecLock *lock)  // IN/OUT:
 {
-   ASSERT((lock->referenceCount > 0) &&
-          (lock->referenceCount < MXUSER_MAX_REC_DEPTH));
-
    lock->referenceCount--;
 
-   if (lock->referenceCount == 0) {
-      int err;
-
+   if (MXRecLockCount(lock) == 0) {
       MXRecLockSetNoOwner(lock);
 
 #if defined(MXUSER_DEBUG)
       lock->ownerRetAddr = NULL;
       lock->portableThreadID = VTHREAD_INVALID_ID;
 #endif
+   }
+}
+
+
+static INLINE void
+MXRecLockRelease(MXRecLock *lock)  // IN/OUT:
+{
+   ASSERT((MXRecLockCount(lock) > 0) &&
+          (MXRecLockCount(lock) < MXUSER_MAX_REC_DEPTH));
 
-      err = MXRecLockReleaseInternal(lock);
+   MXRecLockDecCount(lock);
+
+   if (MXRecLockCount(lock) == 0) {
+      int err = MXRecLockReleaseInternal(lock);
 
       if (vmx86_debug && (err != 0)) {
          Panic("%s: MXRecLockReleaseInternal returned %d\n", __FUNCTION__,
@@ -367,12 +378,6 @@ MXRecLockRelease(MXRecLock *lock)  // IN/OUT:
 }
 
 
-static INLINE uint32
-MXRecLockCount(const MXRecLock *lock)  // IN:
-{
-   return lock->referenceCount;
-}
-
 /*
  * MXUser lock header - all MXUser locks start with this
  */