diff --git a/src/coreclr/src/pal/src/include/pal/mutex.hpp b/src/coreclr/src/pal/src/include/pal/mutex.hpp index 8aa9a53bdafc2..d5f6cef009537 100644 --- a/src/coreclr/src/pal/src/include/pal/mutex.hpp +++ b/src/coreclr/src/pal/src/include/pal/mutex.hpp @@ -146,7 +146,6 @@ class NamedMutexProcessData : public SharedMemoryProcessDataBase private: SharedMemoryProcessDataHeader *m_processDataHeader; - NamedMutexSharedData *m_sharedData; SIZE_T m_lockCount; #if !NAMED_MUTEX_USE_PTHREAD_MUTEX HANDLE m_processLockHandle; @@ -154,6 +153,7 @@ class NamedMutexProcessData : public SharedMemoryProcessDataBase #endif // !NAMED_MUTEX_USE_PTHREAD_MUTEX CorUnix::CPalThread *m_lockOwnerThread; NamedMutexProcessData *m_nextInThreadOwnedNamedMutexList; + bool m_hasRefFromLockOwnerThread; public: static SharedMemoryProcessDataHeader *CreateOrOpen(LPCSTR name, bool acquireLockIfCreated, bool *createdRef); @@ -169,8 +169,19 @@ class NamedMutexProcessData : public SharedMemoryProcessDataBase int sharedLockFileDescriptor #endif // !NAMED_MUTEX_USE_PTHREAD_MUTEX ); + +public: + virtual bool CanClose() const override; + virtual bool HasImplicitRef() const override; + virtual void SetHasImplicitRef(bool value) override; virtual void Close(bool isAbruptShutdown, bool releaseSharedData) override; +public: + bool IsLockOwnedByCurrentThread() const + { + return GetSharedData()->IsLockOwnedByCurrentThread(); + } + private: NamedMutexSharedData *GetSharedData() const; void SetLockOwnerThread(CorUnix::CPalThread *lockOwnerThread); diff --git a/src/coreclr/src/pal/src/include/pal/sharedmemory.h b/src/coreclr/src/pal/src/include/pal/sharedmemory.h index 74e1cd4aedc39..ff64e20b5aa74 100644 --- a/src/coreclr/src/pal/src/include/pal/sharedmemory.h +++ b/src/coreclr/src/pal/src/include/pal/sharedmemory.h @@ -188,9 +188,10 @@ class SharedMemorySharedDataHeader class SharedMemoryProcessDataBase { public: - virtual void Close(bool isAbruptShutdown, bool releaseSharedData) - { - } + virtual bool CanClose() const = 0; + virtual bool HasImplicitRef() const = 0; + virtual void SetHasImplicitRef(bool value) = 0; + virtual void Close(bool isAbruptShutdown, bool releaseSharedData) = 0; virtual ~SharedMemoryProcessDataBase() { diff --git a/src/coreclr/src/pal/src/include/pal/synchobjects.hpp b/src/coreclr/src/pal/src/include/pal/synchobjects.hpp index 66d4710acb24b..ab51a005bae06 100644 --- a/src/coreclr/src/pal/src/include/pal/synchobjects.hpp +++ b/src/coreclr/src/pal/src/include/pal/synchobjects.hpp @@ -118,7 +118,6 @@ namespace CorUnix Volatile m_lSharedSynchLockCount; LIST_ENTRY m_leOwnedObjsList; - CRITICAL_SECTION m_ownedNamedMutexListLock; NamedMutexProcessData *m_ownedNamedMutexListHead; ThreadNativeWaitData m_tnwdNativeData; @@ -178,6 +177,7 @@ namespace CorUnix void RemoveOwnedNamedMutex(NamedMutexProcessData *processData); NamedMutexProcessData *RemoveFirstOwnedNamedMutex(); bool OwnsNamedMutex(NamedMutexProcessData *processData); + bool OwnsAnyNamedMutex() const; // The following methods provide access to the native wait lock for // those implementations that need a lock to protect the support for diff --git a/src/coreclr/src/pal/src/sharedmemory/sharedmemory.cpp b/src/coreclr/src/pal/src/sharedmemory/sharedmemory.cpp index 619fae0852826..d61b92c939fd0 100644 --- a/src/coreclr/src/pal/src/sharedmemory/sharedmemory.cpp +++ b/src/coreclr/src/pal/src/sharedmemory/sharedmemory.cpp @@ -878,6 +878,7 @@ void SharedMemoryProcessDataHeader::Close() // nonzero, don't clean up any object or global process-local state. if (m_refCount == 0) { + _ASSERTE(m_data == nullptr || m_data->CanClose()); SharedMemoryManager::RemoveProcessDataHeader(this); } @@ -1015,7 +1016,12 @@ void SharedMemoryProcessDataHeader::IncRefCount() _ASSERTE(SharedMemoryManager::IsCreationDeletionProcessLockAcquired()); _ASSERTE(m_refCount != 0); - ++m_refCount; + if (++m_refCount == 2 && m_data != nullptr && m_data->HasImplicitRef()) + { + // The synchronization object got an explicit ref that will govern its lifetime, remove the implicit ref + --m_refCount; + m_data->SetHasImplicitRef(false); + } } void SharedMemoryProcessDataHeader::DecRefCount() @@ -1023,10 +1029,21 @@ void SharedMemoryProcessDataHeader::DecRefCount() _ASSERTE(SharedMemoryManager::IsCreationDeletionProcessLockAcquired()); _ASSERTE(m_refCount != 0); - if (--m_refCount == 0) + if (--m_refCount != 0) + { + return; + } + + if (m_data != nullptr && !m_data->CanClose()) { - InternalDelete(this); + // Extend the lifetime of the synchronization object. The process data object is responsible for removing this extra ref + // when the synchronization object transitions into a state where it can be closed. + ++m_refCount; + m_data->SetHasImplicitRef(true); + return; } + + InternalDelete(this); } //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/src/coreclr/src/pal/src/synchmgr/synchmanager.cpp b/src/coreclr/src/pal/src/synchmgr/synchmanager.cpp index 9d868c00e09f9..52b8843889d23 100644 --- a/src/coreclr/src/pal/src/synchmgr/synchmanager.cpp +++ b/src/coreclr/src/pal/src/synchmgr/synchmanager.cpp @@ -568,6 +568,17 @@ namespace CorUnix CThreadSynchronizationInfo * pSynchInfo = &pthrTarget->synchronizationInfo; CPalSynchronizationManager * pSynchManager = GetInstance(); + // The shared memory manager's process lock is acquired before calling into some PAL synchronization primitives that may + // take the PAL synchronization manager's synch lock (acquired below). For example, when using a file lock + // implementation for a named mutex (see NamedMutexProcessData::NamedMutexProcessData()), under the shared memory + // manager's process lock, CreateMutex is called, which acquires the PAL synchronization manager's synch lock. The same + // lock order needs to be maintained here to avoid a deadlock. + bool abandonNamedMutexes = pSynchInfo->OwnsAnyNamedMutex(); + if (abandonNamedMutexes) + { + SharedMemoryManager::AcquireCreationDeletionProcessLock(); + } + // Local lock AcquireLocalSynchLock(pthrCurrent); @@ -610,15 +621,18 @@ namespace CorUnix pSynchManager->m_cacheOwnedObjectsListNodes.Add(pthrCurrent, poolnItem); } - // Abandon owned named mutexes - while (true) + if (abandonNamedMutexes) { - NamedMutexProcessData *processData = pSynchInfo->RemoveFirstOwnedNamedMutex(); - if (processData == nullptr) + // Abandon owned named mutexes + while (true) { - break; + NamedMutexProcessData *processData = pSynchInfo->RemoveFirstOwnedNamedMutex(); + if (processData == nullptr) + { + break; + } + processData->Abandon(); } - processData->Abandon(); } if (pthrTarget != pthrCurrent) @@ -660,6 +674,12 @@ namespace CorUnix } ReleaseLocalSynchLock(pthrCurrent); + + if (abandonNamedMutexes) + { + SharedMemoryManager::ReleaseCreationDeletionProcessLock(); + } + DiscardAllPendingAPCs(pthrCurrent, pthrTarget); return palErr; @@ -4036,7 +4056,6 @@ namespace CorUnix m_ownedNamedMutexListHead(nullptr) { InitializeListHead(&m_leOwnedObjsList); - InitializeCriticalSection(&m_ownedNamedMutexListLock); #ifdef SYNCHMGR_SUSPENSION_SAFE_CONDITION_SIGNALING m_lPendingSignalingCount = 0; @@ -4046,7 +4065,6 @@ namespace CorUnix CThreadSynchronizationInfo::~CThreadSynchronizationInfo() { - DeleteCriticalSection(&m_ownedNamedMutexListLock); if (NULL != m_shridWaitAwakened) { free(m_shridWaitAwakened); @@ -4283,20 +4301,21 @@ namespace CorUnix void CThreadSynchronizationInfo::AddOwnedNamedMutex(NamedMutexProcessData *processData) { + _ASSERTE(this == &GetCurrentPalThread()->synchronizationInfo); _ASSERTE(processData != nullptr); + _ASSERTE(processData->IsLockOwnedByCurrentThread()); _ASSERTE(processData->GetNextInThreadOwnedNamedMutexList() == nullptr); - EnterCriticalSection(&m_ownedNamedMutexListLock); processData->SetNextInThreadOwnedNamedMutexList(m_ownedNamedMutexListHead); m_ownedNamedMutexListHead = processData; - LeaveCriticalSection(&m_ownedNamedMutexListLock); } void CThreadSynchronizationInfo::RemoveOwnedNamedMutex(NamedMutexProcessData *processData) { + _ASSERTE(this == &GetCurrentPalThread()->synchronizationInfo); _ASSERTE(processData != nullptr); + _ASSERTE(processData->IsLockOwnedByCurrentThread()); - EnterCriticalSection(&m_ownedNamedMutexListLock); if (m_ownedNamedMutexListHead == processData) { m_ownedNamedMutexListHead = processData->GetNextInThreadOwnedNamedMutexList(); @@ -4321,38 +4340,44 @@ namespace CorUnix } _ASSERTE(found); } - LeaveCriticalSection(&m_ownedNamedMutexListLock); } NamedMutexProcessData *CThreadSynchronizationInfo::RemoveFirstOwnedNamedMutex() { - EnterCriticalSection(&m_ownedNamedMutexListLock); + _ASSERTE(this == &GetCurrentPalThread()->synchronizationInfo); + NamedMutexProcessData *processData = m_ownedNamedMutexListHead; if (processData != nullptr) { + _ASSERTE(processData->IsLockOwnedByCurrentThread()); m_ownedNamedMutexListHead = processData->GetNextInThreadOwnedNamedMutexList(); processData->SetNextInThreadOwnedNamedMutexList(nullptr); } - LeaveCriticalSection(&m_ownedNamedMutexListLock); return processData; } bool CThreadSynchronizationInfo::OwnsNamedMutex(NamedMutexProcessData *processData) { - EnterCriticalSection(&m_ownedNamedMutexListLock); - bool found = false; + _ASSERTE(this == &GetCurrentPalThread()->synchronizationInfo); + for (NamedMutexProcessData *current = m_ownedNamedMutexListHead; current != nullptr; current = current->GetNextInThreadOwnedNamedMutexList()) { + _ASSERTE(current->IsLockOwnedByCurrentThread()); if (current == processData) { - found = true; - break; + return true; } } - LeaveCriticalSection(&m_ownedNamedMutexListLock); - return found; + + return false; + } + + bool CThreadSynchronizationInfo::OwnsAnyNamedMutex() const + { + _ASSERTE(this == &GetCurrentPalThread()->synchronizationInfo); + return m_ownedNamedMutexListHead != nullptr; } #if SYNCHMGR_SUSPENSION_SAFE_CONDITION_SIGNALING diff --git a/src/coreclr/src/pal/src/synchobj/mutex.cpp b/src/coreclr/src/pal/src/synchobj/mutex.cpp index a4bb340a436a9..6c90ab475e721 100644 --- a/src/coreclr/src/pal/src/synchobj/mutex.cpp +++ b/src/coreclr/src/pal/src/synchobj/mutex.cpp @@ -1247,7 +1247,8 @@ NamedMutexProcessData::NamedMutexProcessData( m_sharedLockFileDescriptor(sharedLockFileDescriptor), #endif // !NAMED_MUTEX_USE_PTHREAD_MUTEX m_lockOwnerThread(nullptr), - m_nextInThreadOwnedNamedMutexList(nullptr) + m_nextInThreadOwnedNamedMutexList(nullptr), + m_hasRefFromLockOwnerThread(false) { _ASSERTE(SharedMemoryManager::IsCreationDeletionProcessLockAcquired()); _ASSERTE(processDataHeader != nullptr); @@ -1263,6 +1264,39 @@ NamedMutexProcessData::NamedMutexProcessData( #endif // !NAMED_MUTEX_USE_PTHREAD_MUTEX } +bool NamedMutexProcessData::CanClose() const +{ + _ASSERTE(SharedMemoryManager::IsCreationDeletionProcessLockAcquired()); + + // When using a pthread robust mutex, the mutex may only be unlocked and destroyed by the thread that owns the lock. When + // using file locks, even though any thread could release that lock, the behavior is kept consistent to the more + // conservative case. If the last handle to the mutex is closed when a different thread owns the lock, the mutex cannot be + // closed. Due to these limitations, the behavior in this corner case is necessarily different from Windows. The caller will + // extend the lifetime of the mutex and will call OnLifetimeExtendedDueToCannotClose() shortly. + return m_lockOwnerThread == nullptr || m_lockOwnerThread == GetCurrentPalThread(); +} + +bool NamedMutexProcessData::HasImplicitRef() const +{ + _ASSERTE(SharedMemoryManager::IsCreationDeletionProcessLockAcquired()); + return m_hasRefFromLockOwnerThread; +} + +void NamedMutexProcessData::SetHasImplicitRef(bool value) +{ + _ASSERTE(SharedMemoryManager::IsCreationDeletionProcessLockAcquired()); + _ASSERTE(m_hasRefFromLockOwnerThread != value); + _ASSERTE(!value || !CanClose()); + + // If value == true: + // The mutex could not be closed and the caller extended the lifetime of the mutex. Record that the lock owner thread + // should release the ref when the lock is released on that thread. + // Else: + // The mutex has an implicit ref and got the first explicit reference from this process. Remove the implicit ref from the + // lock owner thread. + m_hasRefFromLockOwnerThread = value; +} + void NamedMutexProcessData::Close(bool isAbruptShutdown, bool releaseSharedData) { _ASSERTE(SharedMemoryManager::IsCreationDeletionProcessLockAcquired()); @@ -1272,20 +1306,23 @@ void NamedMutexProcessData::Close(bool isAbruptShutdown, bool releaseSharedData) // active references to the mutex. So when shutting down abruptly, don't clean up any object or global process-local state. if (!isAbruptShutdown) { + _ASSERTE(CanClose()); + _ASSERTE(!m_hasRefFromLockOwnerThread); + CPalThread *lockOwnerThread = m_lockOwnerThread; - if (lockOwnerThread != nullptr) + if (lockOwnerThread == GetCurrentPalThread()) { - // The mutex was not released before it was closed. If the lock is owned by the current thread, abandon the mutex. - // In both cases, clean up the owner thread's list of owned mutexes. + // The mutex was not released before the last handle to it from this process was closed on the lock-owning thread. + // Another process may still have a handle to the mutex, but since it appears as though this process would not be + // releasing the mutex, abandon the mutex. The only way for this process to otherwise release the mutex is to open + // another handle to it and release the lock on the same thread, which would be incorrect-looking code. The behavior + // in this corner case is different from Windows. lockOwnerThread->synchronizationInfo.RemoveOwnedNamedMutex(this); - if (lockOwnerThread == GetCurrentPalThread()) - { - Abandon(); - } - else - { - m_lockOwnerThread = nullptr; - } + Abandon(); + } + else + { + _ASSERTE(lockOwnerThread == nullptr); } if (releaseSharedData) @@ -1337,18 +1374,20 @@ NamedMutexSharedData *NamedMutexProcessData::GetSharedData() const void NamedMutexProcessData::SetLockOwnerThread(CorUnix::CPalThread *lockOwnerThread) { _ASSERTE(lockOwnerThread == nullptr || lockOwnerThread == GetCurrentPalThread()); - _ASSERTE(GetSharedData()->IsLockOwnedByCurrentThread()); + _ASSERTE(IsLockOwnedByCurrentThread()); m_lockOwnerThread = lockOwnerThread; } NamedMutexProcessData *NamedMutexProcessData::GetNextInThreadOwnedNamedMutexList() const { + _ASSERTE(IsLockOwnedByCurrentThread()); return m_nextInThreadOwnedNamedMutexList; } void NamedMutexProcessData::SetNextInThreadOwnedNamedMutexList(NamedMutexProcessData *next) { + _ASSERTE(IsLockOwnedByCurrentThread()); m_nextInThreadOwnedNamedMutexList = next; } @@ -1367,7 +1406,7 @@ MutexTryAcquireLockResult NamedMutexProcessData::TryAcquireLock(DWORD timeoutMil // at the appropriate time, see ReleaseLock(). if (m_lockCount != 0) { - _ASSERTE(sharedData->IsLockOwnedByCurrentThread()); // otherwise, this thread would not have acquired the lock + _ASSERTE(IsLockOwnedByCurrentThread()); // otherwise, this thread would not have acquired the lock _ASSERTE(GetCurrentPalThread()->synchronizationInfo.OwnsNamedMutex(this)); if (m_lockCount + 1 < m_lockCount) @@ -1442,7 +1481,7 @@ MutexTryAcquireLockResult NamedMutexProcessData::TryAcquireLock(DWORD timeoutMil // Check if it's a recursive lock attempt if (m_lockCount != 0) { - _ASSERTE(sharedData->IsLockOwnedByCurrentThread()); // otherwise, this thread would not have acquired the process lock + _ASSERTE(IsLockOwnedByCurrentThread()); // otherwise, this thread would not have acquired the process lock _ASSERTE(GetCurrentPalThread()->synchronizationInfo.OwnsNamedMutex(this)); if (m_lockCount + 1 < m_lockCount) @@ -1565,12 +1604,13 @@ MutexTryAcquireLockResult NamedMutexProcessData::TryAcquireLock(DWORD timeoutMil void NamedMutexProcessData::ReleaseLock() { - if (!GetSharedData()->IsLockOwnedByCurrentThread()) + if (!IsLockOwnedByCurrentThread()) { throw SharedMemoryException(static_cast(NamedMutexError::ThreadHasNotAcquiredMutex)); } _ASSERTE(GetCurrentPalThread()->synchronizationInfo.OwnsNamedMutex(this)); + _ASSERTE(!m_hasRefFromLockOwnerThread); _ASSERTE(m_lockCount != 0); --m_lockCount; @@ -1586,23 +1626,36 @@ void NamedMutexProcessData::ReleaseLock() void NamedMutexProcessData::Abandon() { + _ASSERTE(SharedMemoryManager::IsCreationDeletionProcessLockAcquired()); + NamedMutexSharedData *sharedData = GetSharedData(); - _ASSERTE(sharedData->IsLockOwnedByCurrentThread()); + _ASSERTE(IsLockOwnedByCurrentThread()); _ASSERTE(m_lockCount != 0); + bool hasRefFromLockOwnerThread = m_hasRefFromLockOwnerThread; + if (hasRefFromLockOwnerThread) + { + m_hasRefFromLockOwnerThread = false; + } + sharedData->SetIsAbandoned(true); m_lockCount = 0; SetLockOwnerThread(nullptr); ActuallyReleaseLock(); + + if (hasRefFromLockOwnerThread) + { + m_processDataHeader->DecRefCount(); + } } void NamedMutexProcessData::ActuallyReleaseLock() { - NamedMutexSharedData *sharedData = GetSharedData(); - _ASSERTE(sharedData->IsLockOwnedByCurrentThread()); + _ASSERTE(IsLockOwnedByCurrentThread()); _ASSERTE(!GetCurrentPalThread()->synchronizationInfo.OwnsNamedMutex(this)); _ASSERTE(m_lockCount == 0); + NamedMutexSharedData *sharedData = GetSharedData(); sharedData->ClearLockOwner(); #if NAMED_MUTEX_USE_PTHREAD_MUTEX diff --git a/src/coreclr/src/pal/tests/palsuite/threading/NamedMutex/test1/namedmutex.cpp b/src/coreclr/src/pal/tests/palsuite/threading/NamedMutex/test1/namedmutex.cpp index 6635d76a80a07..761bb47b5c6be 100644 --- a/src/coreclr/src/pal/tests/palsuite/threading/NamedMutex/test1/namedmutex.cpp +++ b/src/coreclr/src/pal/tests/palsuite/threading/NamedMutex/test1/namedmutex.cpp @@ -60,9 +60,12 @@ extern bool WriteHeaderInfo(const char *path, char sharedMemoryType, char versio { \ if (!g_isParent) \ { \ - Trace("Child process: "); \ + Trace("'paltest_namedmutex_test1' child process failed at line %u. Expression: " #expression "\n", __LINE__); \ + } \ + else \ + { \ + Trace("'paltest_namedmutex_test1' failed at line %u. Expression: " #expression "\n", __LINE__); \ } \ - Trace("'paltest_namedmutex_test1' failed at line %u. Expression: " #expression "\n", __LINE__); \ fflush(stdout); \ return false; \ } \ @@ -516,7 +519,7 @@ bool MutualExclusionTests_Parent() TestAssert(WaitForSingleObject(m, static_cast(-1)) == WAIT_OBJECT_0); // lock the mutex with no timeout and release TestAssert(m.Release()); - UninitializeParent(testName, parentEvents); + TestAssert(UninitializeParent(testName, parentEvents)); return true; } @@ -539,7 +542,7 @@ DWORD PALAPI MutualExclusionTests_Child(void *arg = nullptr) TestAssert(m.Release()); // release the lock } - UninitializeChild(childRunningEvent, parentEvents, childEvents); + TestAssert(UninitializeChild(childRunningEvent, parentEvents, childEvents)); return 0; } @@ -622,7 +625,7 @@ bool LifetimeTests_Parent() TestAssert(YieldToChild(parentEvents, childEvents, ei)); // child closes second reference TestAssert(!TestFileExists(BuildGlobalShmFilePath(testName, name, NamePrefix))); - UninitializeParent(testName, parentEvents); + TestAssert(UninitializeParent(testName, parentEvents)); return true; } @@ -653,7 +656,7 @@ DWORD PALAPI LifetimeTests_Child(void *arg = nullptr) TestAssert(YieldToParent(parentEvents, childEvents, ei)); // parent verifies } - UninitializeChild(childRunningEvent, parentEvents, childEvents); + TestAssert(UninitializeChild(childRunningEvent, parentEvents, childEvents)); return 0; } @@ -702,11 +705,11 @@ bool AbandonTests_Parent() TestAssert(parentEvents[1].Release()); // child sleeps for short duration and abandons the mutex TestAssert(WaitForSingleObject(m, FailTimeoutMilliseconds) == WAIT_ABANDONED_0); // attempt to lock and see abandoned mutex - UninitializeParent(testName, parentEvents, false /* releaseParentEvents */); // parent events are released above + TestAssert(UninitializeParent(testName, parentEvents, false /* releaseParentEvents */)); // parent events are released above } // Verify that the mutex lock is owned by this thread, by starting a new thread and trying to lock it - StartThread(AbandonTests_Child_TryLock); + TestAssert(StartThread(AbandonTests_Child_TryLock)); { AutoCloseMutexHandle parentEvents[2], childEvents[2]; TestAssert(InitializeParent(testName, parentEvents, childEvents)); @@ -714,11 +717,11 @@ bool AbandonTests_Parent() TestAssert(YieldToChild(parentEvents, childEvents, ei)); // child tries to lock mutex - UninitializeParent(testName, parentEvents); + TestAssert(UninitializeParent(testName, parentEvents)); } // Verify that the mutex lock is owned by this thread, by starting a new process and trying to lock it - StartProcess("AbandonTests_Child_TryLock"); + TestAssert(StartProcess("AbandonTests_Child_TryLock")); AutoCloseMutexHandle parentEvents[2], childEvents[2]; TestAssert(InitializeParent(testName, parentEvents, childEvents)); int ei = 0; @@ -730,7 +733,7 @@ bool AbandonTests_Parent() TestAssert(WaitForSingleObject(m, FailTimeoutMilliseconds) == WAIT_OBJECT_0); // lock again to see it's not abandoned anymore TestAssert(m.Release()); - UninitializeParent(testName, parentEvents, false /* releaseParentEvents */); // parent events are released above + TestAssert(UninitializeParent(testName, parentEvents)); // Since the child abandons the mutex, and a child process may not release the file lock on the shared memory file before // indicating completion to the parent, make sure to delete the shared memory file by repeatedly opening/closing the mutex @@ -772,7 +775,7 @@ DWORD PALAPI AbandonTests_Child_GracefulExit_Close(void *arg = nullptr) m.Close(); // close mutex without releasing lock } - UninitializeChild(childRunningEvent, parentEvents, childEvents); + TestAssert(UninitializeChild(childRunningEvent, parentEvents, childEvents)); return 0; } @@ -801,7 +804,7 @@ DWORD AbandonTests_Child_GracefulExit_NoClose(void *arg = nullptr) m.Abandon(); // don't close the mutex } - UninitializeChild(childRunningEvent, parentEvents, childEvents); + TestAssert(UninitializeChild(childRunningEvent, parentEvents, childEvents)); return 0; } @@ -830,7 +833,7 @@ DWORD AbandonTests_Child_AbruptExit(void *arg = nullptr) m.Abandon(); // don't close the mutex } - UninitializeChild(childRunningEvent, parentEvents, childEvents); + TestAssert(UninitializeChild(childRunningEvent, parentEvents, childEvents)); } TestAssert(test_kill(currentPid) == 0); // abandon the mutex abruptly @@ -894,7 +897,7 @@ DWORD AbandonTests_Child_FileLocksNotInherited_Child_AbruptExit(void *arg = null m.Close(); // close mutex without releasing lock (root parent expects the mutex to be abandoned) } - UninitializeChild(childRunningEvent, parentEvents, childEvents); + TestAssert(UninitializeChild(childRunningEvent, parentEvents, childEvents)); return 0; } @@ -917,14 +920,12 @@ DWORD PALAPI AbandonTests_Child_TryLock(void *arg) TestAssert(WaitForSingleObject(m, g_expectedTimeoutMilliseconds) == WAIT_TIMEOUT); } - UninitializeChild(childRunningEvent, parentEvents, childEvents); + TestAssert(UninitializeChild(childRunningEvent, parentEvents, childEvents)); return 0; } bool AbandonTests() { - const char *testName = "AbandonTests"; - // Abandon by graceful exit where the lock owner closes the mutex before releasing it, unblocks a waiter TestAssert(StartThread(AbandonTests_Child_GracefulExit_Close)); TestAssert(AbandonTests_Parent()); @@ -945,6 +946,151 @@ bool AbandonTests() return true; } +bool LockAndCloseWithoutThreadExitTests_Parent_CloseOnSameThread() +{ + const char *testName = "LockAndCloseWithoutThreadExitTests"; + + AutoCloseMutexHandle parentEvents[2], childEvents[2]; + TestAssert(InitializeParent(testName, parentEvents, childEvents)); + int ei = 0; + char name[MaxPathSize]; + AutoCloseMutexHandle m; + + TestCreateMutex(m, BuildName(testName, name, GlobalPrefix, NamePrefix)); + TestAssert(m != nullptr); + + TestAssert(YieldToChild(parentEvents, childEvents, ei)); // child locks mutex and closes second reference to mutex on lock-owner thread + TestAssert(WaitForSingleObject(m, 0) == WAIT_TIMEOUT); // attempt to lock and fail + + TestAssert(YieldToChild(parentEvents, childEvents, ei)); // child closes last reference to mutex on lock-owner thread + TestAssert(WaitForSingleObject(m, 0) == WAIT_ABANDONED_0); // attempt to lock and see abandoned mutex + TestAssert(m.Release()); + + TestAssert(YieldToChild(parentEvents, childEvents, ei)); // child exits + TestAssert(TestFileExists(BuildGlobalShmFilePath(testName, name, NamePrefix))); + m.Close(); + TestAssert(!TestFileExists(BuildGlobalShmFilePath(testName, name, NamePrefix))); + + TestAssert(UninitializeParent(testName, parentEvents)); + return true; +} + +DWORD PALAPI LockAndCloseWithoutThreadExitTests_Child_CloseOnSameThread(void *arg = nullptr) +{ + const char *testName = "LockAndCloseWithoutThreadExitTests"; + + TestAssert(test_getpid() != g_parentPid); // this test needs to run in a separate process + + AutoCloseMutexHandle childRunningEvent, parentEvents[2], childEvents[2]; + TestAssert(InitializeChild(testName, childRunningEvent, parentEvents, childEvents)); + int ei = 0; + char name[MaxPathSize]; + + // ... parent waits for child to lock and close second reference to mutex + AutoCloseMutexHandle m(TestOpenMutex(BuildName(testName, name, GlobalPrefix, NamePrefix))); + TestAssert(m != nullptr); + TestAssert(WaitForSingleObject(m, 0) == WAIT_OBJECT_0); + TestAssert(AutoCloseMutexHandle(TestOpenMutex(BuildName(testName, name, GlobalPrefix, NamePrefix))) != nullptr); + TestAssert(YieldToParent(parentEvents, childEvents, ei)); // parent waits for child to close last reference to mutex + + m.Close(); // close mutex on lock-owner thread without releasing lock + TestAssert(YieldToParent(parentEvents, childEvents, ei)); // parent verifies while this thread is still active + + TestAssert(UninitializeChild(childRunningEvent, parentEvents, childEvents)); + return 0; +} + +DWORD PALAPI LockAndCloseWithoutThreadExitTests_ChildThread_CloseMutex(void *arg); + +bool LockAndCloseWithoutThreadExitTests_Parent_CloseOnDifferentThread() +{ + const char *testName = "LockAndCloseWithoutThreadExitTests"; + + AutoCloseMutexHandle parentEvents[2], childEvents[2]; + TestAssert(InitializeParent(testName, parentEvents, childEvents)); + int ei = 0; + char name[MaxPathSize]; + AutoCloseMutexHandle m; + + TestCreateMutex(m, BuildName(testName, name, GlobalPrefix, NamePrefix)); + TestAssert(m != nullptr); + + TestAssert(YieldToChild(parentEvents, childEvents, ei)); // child locks mutex and closes second reference to mutex on lock-owner thread + TestAssert(WaitForSingleObject(m, 0) == WAIT_TIMEOUT); // attempt to lock and fail + + TestAssert(YieldToChild(parentEvents, childEvents, ei)); // child closes last reference to mutex on non-lock-owner thread + TestAssert(WaitForSingleObject(m, 0) == WAIT_TIMEOUT); // attempt to lock and fail + m.Close(); + m = TestOpenMutex(BuildName(testName, name, GlobalPrefix, NamePrefix)); + TestAssert(m != nullptr); // child has implicit reference to mutex + + TestAssert(YieldToChild(parentEvents, childEvents, ei)); // child closes new reference to mutex on lock-owner thread + TestAssert(WaitForSingleObject(m, 0) == WAIT_ABANDONED_0); // attempt to lock and see abandoned mutex + TestAssert(m.Release()); + + TestAssert(YieldToChild(parentEvents, childEvents, ei)); // child exits + TestAssert(TestFileExists(BuildGlobalShmFilePath(testName, name, NamePrefix))); + m.Close(); + TestAssert(!TestFileExists(BuildGlobalShmFilePath(testName, name, NamePrefix))); + + TestAssert(UninitializeParent(testName, parentEvents)); + return true; +} + +DWORD PALAPI LockAndCloseWithoutThreadExitTests_Child_CloseOnDifferentThread(void *arg = nullptr) +{ + const char *testName = "LockAndCloseWithoutThreadExitTests"; + + TestAssert(test_getpid() != g_parentPid); // this test needs to run in a separate process + + AutoCloseMutexHandle childRunningEvent, parentEvents[2], childEvents[2]; + TestAssert(InitializeChild(testName, childRunningEvent, parentEvents, childEvents)); + int ei = 0; + char name[MaxPathSize]; + + // ... parent waits for child to lock and close second reference to mutex + AutoCloseMutexHandle m(TestOpenMutex(BuildName(testName, name, GlobalPrefix, NamePrefix))); + TestAssert(m != nullptr); + TestAssert(WaitForSingleObject(m, 0) == WAIT_OBJECT_0); + TestAssert(AutoCloseMutexHandle(TestOpenMutex(BuildName(testName, name, GlobalPrefix, NamePrefix))) != nullptr); + TestAssert(YieldToParent(parentEvents, childEvents, ei)); // parent waits for child to close last reference to mutex + + // Close the mutex on a thread that is not the lock-owner thread, without releasing the lock + HANDLE closeMutexThread = nullptr; + TestAssert(StartThread(LockAndCloseWithoutThreadExitTests_ChildThread_CloseMutex, (HANDLE)m, &closeMutexThread)); + TestAssert(closeMutexThread != nullptr); + TestAssert(WaitForSingleObject(closeMutexThread, FailTimeoutMilliseconds) == WAIT_OBJECT_0); + TestAssert(CloseHandle(closeMutexThread)); + m.Abandon(); // mutex is already closed, don't close it again + TestAssert(YieldToParent(parentEvents, childEvents, ei)); // parent verifies while this lock-owner thread is still active + + m = TestOpenMutex(BuildName(testName, name, GlobalPrefix, NamePrefix)); + TestAssert(m != nullptr); + m.Close(); // close mutex on lock-owner thread without releasing lock + TestAssert(YieldToParent(parentEvents, childEvents, ei)); // parent verifies while this thread is still active + + TestAssert(UninitializeChild(childRunningEvent, parentEvents, childEvents)); + return 0; +} + +DWORD PALAPI LockAndCloseWithoutThreadExitTests_ChildThread_CloseMutex(void *arg) +{ + TestAssert(arg != nullptr); + AutoCloseMutexHandle((HANDLE)arg).Close(); + return 0; +} + +bool LockAndCloseWithoutThreadExitTests() +{ + TestAssert(StartProcess("LockAndCloseWithoutThreadExitTests_Child_CloseOnSameThread")); + TestAssert(LockAndCloseWithoutThreadExitTests_Parent_CloseOnSameThread()); + + TestAssert(StartProcess("LockAndCloseWithoutThreadExitTests_Child_CloseOnDifferentThread")); + TestAssert(LockAndCloseWithoutThreadExitTests_Parent_CloseOnDifferentThread()); + + return true; +} + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // Test harness @@ -954,7 +1100,8 @@ bool (*const (TestList[]))() = HeaderMismatchTests, MutualExclusionTests, LifetimeTests, - AbandonTests + AbandonTests, + LockAndCloseWithoutThreadExitTests }; bool RunTests() @@ -1125,6 +1272,14 @@ int __cdecl main(int argc, char **argv) { AbandonTests_Child_TryLock(); } + else if (test_strcmp(argv[2], "LockAndCloseWithoutThreadExitTests_Child_CloseOnSameThread") == 0) + { + LockAndCloseWithoutThreadExitTests_Child_CloseOnSameThread(); + } + else if (test_strcmp(argv[2], "LockAndCloseWithoutThreadExitTests_Child_CloseOnDifferentThread") == 0) + { + LockAndCloseWithoutThreadExitTests_Child_CloseOnDifferentThread(); + } ExitProcess(PASS); return PASS; } diff --git a/src/libraries/System.Threading/tests/MutexTests.cs b/src/libraries/System.Threading/tests/MutexTests.cs index 79de3ee2be4f9..e5143a4484446 100644 --- a/src/libraries/System.Threading/tests/MutexTests.cs +++ b/src/libraries/System.Threading/tests/MutexTests.cs @@ -401,39 +401,53 @@ public static IEnumerable CrossProcess_NamedMutex_ProtectedFileAccessA } } - [ActiveIssue("https://github.com/dotnet/runtime/issues/28449")] [Theory] [MemberData(nameof(CrossProcess_NamedMutex_ProtectedFileAccessAtomic_MemberData))] public void CrossProcess_NamedMutex_ProtectedFileAccessAtomic(string prefix) { - ThreadTestHelpers.RunTestInBackgroundThread(() => + string fileName = GetTestFilePath(); + try { - string mutexName = prefix + Guid.NewGuid().ToString("N"); - string fileName = GetTestFilePath(); - - Action otherProcess = (m, f) => + ThreadTestHelpers.RunTestInBackgroundThread(() => { - using (var mutex = Mutex.OpenExisting(m)) - { - mutex.CheckedWait(); - try - { File.WriteAllText(f, "0"); } - finally { mutex.ReleaseMutex(); } + string mutexName = prefix + Guid.NewGuid().ToString("N"); - IncrementValueInFileNTimes(mutex, f, 10); - } - }; + Action otherProcess = (m, f) => + { + using (var mutex = Mutex.OpenExisting(m)) + { + mutex.CheckedWait(); + try + { File.WriteAllText(f, "0"); } + finally { mutex.ReleaseMutex(); } - using (var mutex = new Mutex(false, mutexName)) - using (var remote = RemoteExecutor.Invoke(otherProcess, mutexName, fileName)) - { - SpinWait.SpinUntil(() => File.Exists(fileName), ThreadTestHelpers.UnexpectedTimeoutMilliseconds); + IncrementValueInFileNTimes(mutex, f, 10); + } + }; - IncrementValueInFileNTimes(mutex, fileName, 10); - } + using (var mutex = new Mutex(false, mutexName)) + using (var remote = RemoteExecutor.Invoke(otherProcess, mutexName, fileName)) + { + SpinWait.SpinUntil( + () => + { + mutex.CheckedWait(); + try + { return File.Exists(fileName) && int.TryParse(File.ReadAllText(fileName), out _); } + finally { mutex.ReleaseMutex(); } + }, + ThreadTestHelpers.UnexpectedTimeoutMilliseconds); + + IncrementValueInFileNTimes(mutex, fileName, 10); + } - Assert.Equal(20, int.Parse(File.ReadAllText(fileName))); - }); + Assert.Equal(20, int.Parse(File.ReadAllText(fileName))); + }); + } + catch (Exception ex) when (File.Exists(fileName)) + { + throw new AggregateException($"File contents: {File.ReadAllText(fileName)}", ex); + } } private static void IncrementValueInFileNTimes(Mutex mutex, string fileName, int n) @@ -451,6 +465,103 @@ private static void IncrementValueInFileNTimes(Mutex mutex, string fileName, int } } + [Fact] + public void NamedMutex_ThreadExitDisposeRaceTest() + { + var mutexName = Guid.NewGuid().ToString("N"); + + for (int i = 0; i < 1000; ++i) + { + var m = new Mutex(false, mutexName); + var startParallelTest = new ManualResetEvent(false); + + var t0Ready = new AutoResetEvent(false); + Thread t0 = ThreadTestHelpers.CreateGuardedThread(out Action waitForT0, () => + { + m.CheckedWait(); + t0Ready.Set(); + startParallelTest.CheckedWait(); // after this, exit T0 + }); + t0.IsBackground = true; + + var t1Ready = new AutoResetEvent(false); + Thread t1 = ThreadTestHelpers.CreateGuardedThread(out Action waitForT1, () => + { + using (var m2 = Mutex.OpenExisting(mutexName)) + { + m.Dispose(); + t1Ready.Set(); + startParallelTest.CheckedWait(); // after this, close last handle to named mutex, exit T1 + } + }); + t1.IsBackground = true; + + t0.Start(); + t0Ready.CheckedWait(); // wait for T0 to acquire the mutex + t1.Start(); + t1Ready.CheckedWait(); // wait for T1 to open the existing mutex in a new mutex object and dispose one of the two + + // Release both threads at the same time. T0 will be exiting the thread, perhaps trying to abandon the mutex + // that is still locked by it. In parallel, T1 will be disposing the last mutex instance, which would try to + // destroy the mutex. + startParallelTest.Set(); + waitForT0(); + waitForT1(); + + // Create a new mutex object with the same name and acquire it. There can be a delay between Thread.Join() above + // returning and for T0 to abandon its mutex, keep trying to also verify that the mutex object is actually + // destroyed and created new again. + SpinWait.SpinUntil(() => + { + using (m = new Mutex(true, mutexName, out bool createdNew)) + { + if (createdNew) + { + m.ReleaseMutex(); + } + return createdNew; + } + }); + } + } + + [Fact] + public void NamedMutex_DisposeWhenLockedRaceTest() + { + var mutexName = Guid.NewGuid().ToString("N"); + var mutex2Name = mutexName + "_2"; + + var waitsForThread = new Action[Environment.ProcessorCount]; + for (int i = 0; i < waitsForThread.Length; ++i) + { + var t = ThreadTestHelpers.CreateGuardedThread(out waitsForThread[i], () => + { + for (int i = 0; i < 1000; ++i) + { + // Create or open two mutexes with different names, acquire the lock if created, and dispose without + // releasing the lock. What may occasionally happen is, one thread T0 will acquire the lock, another + // thread T1 will open the same mutex, T0 will dispose its mutex while the lock is held, and T1 will + // then release the last reference to the mutex. On some implementations T1 may not be able to destroy + // the mutex when it is still locked by T0, or there may be potential for races in the sequence. This + // test only looks for errors from race conditions. + using (var mutex = new Mutex(true, mutexName)) + { + } + using (var mutex = new Mutex(true, mutex2Name)) + { + } + } + }); + t.IsBackground = true; + t.Start(); + } + + foreach (var waitForThread in waitsForThread) + { + waitForThread(); + } + } + public static TheoryData GetValidNames() { var names = new TheoryData() { Guid.NewGuid().ToString("N") };