From: VMware, Inc <> Date: Thu, 17 Jun 2010 21:27:15 +0000 (-0700) Subject: lib/lock: assertion failure due to MXUser condVar X-Git-Tag: 2010.06.16-268169~126 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3456af5e7d2bb48980813a2d1a230a4ad9f2249f;p=thirdparty%2Fopen-vm-tools.git 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 --- diff --git a/open-vm-tools/lib/lock/ulCondVar.c b/open-vm-tools/lib/lock/ulCondVar.c index a0835321f..591a639ca 100644 --- 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 074aaca72..4c4a76550 100644 --- 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 */