lib/lock: nasty races and memory leaks

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

diff --git a/open-vm-tools/lib/lock/ul.c b/open-vm-tools/lib/lock/ul.c
index 2d579e4789a7b54442b4362021be31dba246a73a..ebd1db8876200f9b4c43c18ca0f125a4d7c9f79a 100644 (file)
--- a/open-vm-tools/lib/lock/ul.c
+++ b/open-vm-tools/lib/lock/ul.c
@@ -93,6 +93,10 @@ typedef struct {
    MXUserHeader  *lockArray[MXUSER_MAX_LOCKS_PER_THREAD];
 } MXUserPerThread;
 
+#if defined(_WIN32) && !defined(VMX86_VMX)
+static Atomic_Ptr hashLockMem;
+#endif
+
 static Atomic_Ptr hashTableMem;
 
 
@@ -122,12 +126,20 @@ MXUserGetPerThread(void *tid,      // IN: thread ID
                    Bool mayAlloc)  // IN: alloc perThread if not present?
 {
    HashTable *hash;
-   MXUserPerThread *perThread;
+   MXUserPerThread *perThread = NULL;
+
+#if defined(_WIN32) && !defined(VMX86_VMX)
+   MXRecLock *hashLock = MXUserInternalSingleton(&hashLockMem);
+
+   ASSERT(hashLock);
+
+   hash = HashTable_AllocOnce(&hashTableMem, 1024, HASH_INT_KEY, NULL);
 
+   MXRecLockAcquire(hashLock);
+#else
    hash = HashTable_AllocOnce(&hashTableMem, 1024,
                               HASH_INT_KEY | HASH_FLAG_ATOMIC, NULL);
-
-   perThread = NULL;
+#endif
 
    if (!HashTable_Lookup(hash, tid, (void **) &perThread)) {
       /* No entry for this tid was found, allocate one? */
@@ -137,9 +149,8 @@ MXUserGetPerThread(void *tid,      // IN: thread ID
                                                      sizeof(MXUserPerThread));
 
          /*
-          * Attempt to (racey) insert a perThread on behalf of the specified
-          * thread. If yet another thread takes care of this first, clean up
-          * the mess.
+          * Attempt to insert a perThread on behalf of the specified thread.
+          * If another thread has taken care of this first, clean up the mess.
           */
 
          perThread = HashTable_LookupOrInsert(hash, tid, newEntry);
@@ -153,6 +164,10 @@ MXUserGetPerThread(void *tid,      // IN: thread ID
       }
    }
 
+#if defined(_WIN32) && !defined(VMX86_VMX)
+   MXRecLockRelease(hashLock);
+#endif
+
    return perThread;
 }
 
@@ -436,6 +451,34 @@ MXUserReleaseTracking(MXUserHeader *header)  // IN: lock, via its header
 
    perThread->lockArray[lastEntry] = NULL;  // tidy up memory
    perThread->locksHeld--;
+
+#if defined(_WIN32) && !defined(VMX86_VMX)
+   /*
+    * On Windows thread IDs aren't greedily recycled. If a process creates and
+    * destroys many threads this can cause a memory leak of perThread data
+    * (and its overhead). We avoid this by atomically (via a lock) creating
+    * (upon first lock acquired) and deleting (upon last lock release) a
+    * perThread.
+    *
+    * Yes, this is a performance cost but it only affects Windows debug
+    * builds and then not by very much - we tend to run for a long time
+    * with either no locks held or at least one lock held.
+    */
+
+   if (perThread->locksHeld == 0) {
+      HashTable *hash = Atomic_ReadPtr(&hashTableMem);
+      MXRecLock *hashLock = MXUserInternalSingleton(&hashLockMem);
+
+      ASSERT(hash);
+      ASSERT(hashLock);
+
+      MXRecLockAcquire(hashLock);
+      HashTable_Delete(hash, tid);
+      MXRecLockRelease(hashLock);
+
+      free(perThread);
+   }
+#endif
 }
 
 

diff --git a/open-vm-tools/lib/lock/ulExcl.c b/open-vm-tools/lib/lock/ulExcl.c
index 20b85b3885e804c4e60351c3d583eeb173609159..13ce43a8197835f65f2cf92084fa8e2bb616908a 100644 (file)
--- a/open-vm-tools/lib/lock/ulExcl.c
+++ b/open-vm-tools/lib/lock/ulExcl.c
@@ -145,9 +145,11 @@ MXUserDumpExclLock(MXUserHeader *header)  // IN:
 
    Warning("\tcount %u\n", lock->recursiveLock.referenceCount);
 
-   Warning("\towner 0x%p\n",
+   Warning("\tnative threadID 0x%p\n",
            (void *)(uintptr_t)lock->recursiveLock.nativeThreadID);
 
+   Warning("\tVThreadID %u\n", lock->recursiveLock.vmwThreadID);
+
    if (stats && (stats->holder != NULL)) {
       Warning("\tholder %p\n", stats->holder);
    }

diff --git a/open-vm-tools/lib/lock/ulInt.h b/open-vm-tools/lib/lock/ulInt.h
index caa64d54ecf4c6cc9961d11e81ab3f0f94433a8f..57e82041f30d09ae36756b42b88d3f9295b2fc9f 100644 (file)
--- a/open-vm-tools/lib/lock/ulInt.h
+++ b/open-vm-tools/lib/lock/ulInt.h
@@ -59,6 +59,7 @@ typedef struct {
 
    int              referenceCount;   // Acquisition count
    MXThreadID       nativeThreadID;   // Native thread ID
+   VThreadID        vmwThreadID;      // VMW thread ID
 } MXRecLock;
 
 
@@ -123,6 +124,7 @@ static INLINE void
 MXRecLockSetNoOwner(MXRecLock *lock)  // IN:
 {
    lock->nativeThreadID = MXUSER_INVALID_OWNER;
+   lock->vmwThreadID = VTHREAD_INVALID_ID;
 }
 
 
@@ -130,6 +132,7 @@ static INLINE void
 MXRecLockSetOwner(MXRecLock *lock)  // IN/OUT:
 {
    lock->nativeThreadID = GetCurrentThreadId();
+   lock->vmwThreadID = VThread_CurID();
 }
 
 
@@ -183,6 +186,7 @@ MXRecLockSetNoOwner(MXRecLock *lock)  // IN/OUT:
 {
    /* a hack but it works portably */
    memset((void *) &lock->nativeThreadID, 0xFF, sizeof(lock->nativeThreadID));
+   lock->vmwThreadID = VTHREAD_INVALID_ID;
 }
 
 
@@ -190,6 +194,7 @@ static INLINE void
 MXRecLockSetOwner(MXRecLock *lock)  // IN:
 {
    lock->nativeThreadID = pthread_self();
+   lock->vmwThreadID = VThread_CurID();
 }
 
 
@@ -387,7 +392,18 @@ MXUserGetThreadID(void)
    /* All thread types must fit into a uintptr_t  */
 
    ASSERT_ON_COMPILE(sizeof(VThreadID) <= sizeof (void *));
-   return (void *) (uintptr_t) VThread_CurID();
+
+   /*
+    * We must use native (OS provided) thread IDs which are guaranteed to
+    * always be unique. Any ID system that could possibly alias, under
+    * an circumstances, is unacceptable.
+    */
+
+#if defined(_WIN32)
+   return (void *) (uintptr_t) GetCurrentThreadId();
+#else
+   return (void *) (uintptr_t) pthread_self();
+#endif
 }
 
 

diff --git a/open-vm-tools/lib/lock/ulRec.c b/open-vm-tools/lib/lock/ulRec.c
index c8c4dd7f497915f0fe31a97eec5daf9452321b06..1b5258aae1d0e547dc0b47b0070e0170b9bdb302 100644 (file)
--- a/open-vm-tools/lib/lock/ulRec.c
+++ b/open-vm-tools/lib/lock/ulRec.c
@@ -162,9 +162,11 @@ MXUserDumpRecLock(MXUserHeader *header)  // IN:
 
       Warning("\tcount %u\n", lock->recursiveLock.referenceCount);
 
-      Warning("\towner 0x%p\n",
+      Warning("\tnative threadID 0x%p\n",
               (void *)(uintptr_t)lock->recursiveLock.nativeThreadID);
 
+      Warning("\tVThreadID %u\n", lock->recursiveLock.vmwThreadID);
+
       if (stats && (stats->holder != NULL)) {
          Warning("\tholder %p\n", stats->holder);
       }