From 1149f02fad6436851cfed067c7e43daf04c67939 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Mon, 27 Apr 2020 16:30:06 -0700 Subject: [PATCH] Fix Unix named mutex crash during some race conditions Below when I refer to "mutex" I'm referring to the underlying mutex object, not an instance of the `Mutex` class. - When the last reference to a mutex is closed while the lock is held by some thread and a pthread mutex is used, the mutex was attempted to be destroyed but that has undefined behavior - There doesn't seem to be a way to behave exactly like on Windows for this corner case, where the mutex is destroyed when the last reference to it is released, regardless of which process has the mutex locked and which process releases the last reference to it (they could be two different processes), including in cases of abrupt shutdown - For this corner case I settled on what seems like a decent solution and compatible with older runtimes: - When a process releases its last reference to the mutex - If that mutex is locked by the same thread, the lock is abandoned and the process no longer references the mutex - If that mutex is locked by a different thread, the lifetime of the mutex is extended with an implicit ref. The implicit ref prevents this or other processes from attempting to destroy the mutex while it is locked. The implicit ref is removed in either of these cases: - The mutex gets another reference from within the same process - The thread that owns the lock exits and abandons the mutex, at which point that would be the last reference to the mutex and the process would not reference the mutex anymore - The implementation based on file locks is less restricted, but for consistency that implementation also follows the same behavior - There was also a race between an exiting thread abandoning one of its locked named mutexes and another thread releasing the last reference to it, fixed by using the creation/deletion process lock to synchronize Fix for https://github.com/dotnet/runtime/issues/34271 in master Closes https://github.com/dotnet/runtime/issues/28449 - probably doesn't fix the issue, but trying to enable it to see if it continues to fail --- src/coreclr/src/pal/src/include/pal/mutex.hpp | 13 +- .../src/pal/src/include/pal/sharedmemory.h | 7 +- .../src/pal/src/include/pal/synchobjects.hpp | 2 +- .../src/pal/src/sharedmemory/sharedmemory.cpp | 23 ++- .../src/pal/src/synchmgr/synchmanager.cpp | 65 ++++-- src/coreclr/src/pal/src/synchobj/mutex.cpp | 91 +++++++-- .../threading/NamedMutex/test1/namedmutex.cpp | 193 ++++++++++++++++-- .../System.Threading/tests/MutexTests.cs | 157 +++++++++++--- 8 files changed, 462 insertions(+), 89 deletions(-) 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") };