From a4b56d105a76708c5d995ddebdef06a18a92dca8 Mon Sep 17 00:00:00 2001 From: Gene Cooperman Date: Wed, 24 Dec 2025 21:49:48 -0500 Subject: [PATCH 1/2] Add tests: mutex lock/unlock with test for owner --- .../mutex-lock-unlock-two-owners.cpp | 29 +++++++++++++++++++ .../mutex-recursive-lock.cpp | 16 ++++++++++ 2 files changed, 45 insertions(+) create mode 100644 test/invalid_thread_op_arg_program/mutex-lock-unlock-two-owners.cpp create mode 100644 test/invalid_thread_op_arg_program/mutex-recursive-lock.cpp diff --git a/test/invalid_thread_op_arg_program/mutex-lock-unlock-two-owners.cpp b/test/invalid_thread_op_arg_program/mutex-lock-unlock-two-owners.cpp new file mode 100644 index 00000000..de964eb1 --- /dev/null +++ b/test/invalid_thread_op_arg_program/mutex-lock-unlock-two-owners.cpp @@ -0,0 +1,29 @@ +// BUG: One thread locks a mutex, and a second thread unlocks the mutex. +#include +#include + +pthread_mutex_t lock; +sem_t signal_sem; + +void* lock_thread(void* arg) { + pthread_mutex_lock(&lock); + sem_post(&signal_sem); + return NULL; +} +void* unlock_thread(void* arg) { + sem_wait(&signal_sem); + pthread_mutex_unlock(&lock); + return NULL; +} +int main() { + pthread_t t1, t2; + pthread_mutex_init(&lock, NULL); + sem_init(&signal_sem, 0, 0); + pthread_create(&t1, NULL, lock_thread, NULL); + pthread_create(&t2, NULL, unlock_thread, NULL); + pthread_join(t1, NULL); + pthread_join(t2, NULL); + sem_destroy(&signal_sem); + pthread_mutex_destroy(&lock); + return 0; +} diff --git a/test/invalid_thread_op_arg_program/mutex-recursive-lock.cpp b/test/invalid_thread_op_arg_program/mutex-recursive-lock.cpp new file mode 100644 index 00000000..a88f9e3b --- /dev/null +++ b/test/invalid_thread_op_arg_program/mutex-recursive-lock.cpp @@ -0,0 +1,16 @@ +// BUG: the thread tries to recursively lock a mutex that it owns. +#include + +void* lock_thread(void* arg) { + pthread_mutex_lock(&lock); + pthread_mutex_lock(&lock); + return NULL; +} +int main() { + pthread_t t1, t2; + pthread_mutex_init(&lock, NULL); + pthread_create(&t1, NULL, lock_thread, NULL); + pthread_join(t1, NULL); + pthread_mutex_destroy(&lock); + return 0; +} From 8d3ecf50a5ab08d0062e0790740f137033dd7248 Mon Sep 17 00:00:00 2001 From: Gene Cooperman Date: Wed, 24 Dec 2025 21:21:28 -0500 Subject: [PATCH 2/2] Add MCMutex::owner; check error: lock/unlock * Check for recursive pthread_mutex_lock by same owner thread * Check for pthread_mutex_unlock by different thread from mutex owner * And tests converted to do setNextTransition before fail, to correctly display "PENDING TRANSITIONS". --- NEWS | 4 +++ include/objects/MCMutex.h | 2 ++ src/objects/MCMutex.cpp | 9 +++++++ src/transitions/mutex/MCMutexLock.cpp | 33 ++++++++++++++++++----- src/transitions/mutex/MCMutexUnlock.cpp | 36 ++++++++++++++++++++----- 5 files changed, 70 insertions(+), 14 deletions(-) diff --git a/NEWS b/NEWS index 3697ba6c..3c4c6717 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,7 @@ +Version 1.2.0 release notes (DATE: TBD) +========================================== +* Check for recursive pthread_mutex_lock by a thread already holding that mutex lock. + Version 1.1.0 release notes (Oct. 7, 2025) ========================================== diff --git a/include/objects/MCMutex.h b/include/objects/MCMutex.h index 32d2c346..76b51b76 100644 --- a/include/objects/MCMutex.h +++ b/include/objects/MCMutex.h @@ -7,6 +7,7 @@ struct MCMutexShadow { pthread_mutex_t *systemIdentity; enum State { undefined, unlocked, locked, destroyed } state; + tid_t owner; explicit MCMutexShadow(pthread_mutex_t *systemIdentity) : systemIdentity(systemIdentity), state(undefined) {} @@ -45,6 +46,7 @@ struct MCMutex : public MCVisibleObject { void unlock(); void init(); void deinit(); + tid_t ownerTid() const; }; #endif // INCLUDE_MCMINI_OBJECTS_MCMUTEX_HPP diff --git a/src/objects/MCMutex.cpp b/src/objects/MCMutex.cpp index 26324884..f3f57d4a 100644 --- a/src/objects/MCMutex.cpp +++ b/src/objects/MCMutex.cpp @@ -48,18 +48,21 @@ void MCMutex::lock(tid_t newOwner) { this->mutexShadow.state = MCMutexShadow::locked; + this->mutexShadow.owner = newOwner; } void MCMutex::unlock() { this->mutexShadow.state = MCMutexShadow::unlocked; + this->mutexShadow.owner = TID_INVALID; } void MCMutex::init() { this->mutexShadow.state = MCMutexShadow::unlocked; + this->mutexShadow.owner = TID_INVALID; } void @@ -68,6 +71,12 @@ MCMutex::deinit() this->mutexShadow.state = MCMutexShadow::undefined; } +tid_t +MCMutex::ownerTid() const +{ + return this->mutexShadow.owner; +} + bool MCMutex::canAcquire(tid_t thread) const { diff --git a/src/transitions/mutex/MCMutexLock.cpp b/src/transitions/mutex/MCMutexLock.cpp index 47ab2ae6..57f622aa 100644 --- a/src/transitions/mutex/MCMutexLock.cpp +++ b/src/transitions/mutex/MCMutexLock.cpp @@ -80,17 +80,36 @@ MCReadMutexLock(const MCSharedTransition *shmTransition, assert(mutexThatExists -> mutexShadow.state == MCMutexShadow::unlocked || mutexThatExists -> mutexShadow.state == MCMutexShadow::locked); - MC_REPORT_UNDEFINED_BEHAVIOR_ON_FAIL( - mutexThatExists != nullptr, - "Attempting to lock an uninitialized mutex"); + // 1. We've now computed mutexThatExists (i.e., the state of the + // mutex arguments to pthread_mutex_lock()). Compute the nextTransition. + tid_t threadThatRanId = shmTransition->executor; + auto threadThatRan = state->getThreadWithId(threadThatRanId); + MCTransition *nextTransition = + new MCMutexLock(threadThatRan, mutexThatExists); + + // 2. Now test for undefined behaviors or invalid arguments. + // If fail, then call setNextTransition on nextTransition, in order to + // display correct results for programState->printNextTransitions(). + if (mutexThatExists == nullptr) { + programState->setNextTransitionForThread(threadThatRanId, std::shared_ptr(nextTransition)); + mc_report_undefined_behavior( + "Attempting to lock an uninitialized mutex"); + } if (mutexThatExists->isDestroyed()) { - MC_REPORT_UNDEFINED_BEHAVIOR( + programState->setNextTransitionForThread(threadThatRanId, std::shared_ptr(nextTransition)); + mc_report_undefined_behavior( "Attempting to lock a mutex that has been destroyed"); } - tid_t threadThatRanId = shmTransition->executor; - auto threadThatRan = state->getThreadWithId(threadThatRanId); - return new MCMutexLock(threadThatRan, mutexThatExists); + if (mutexThatExists->isLocked() && + mutexThatExists->ownerTid() == threadThatRanId) { + programState->setNextTransitionForThread(threadThatRanId, std::shared_ptr(nextTransition)); + mc_report_undefined_behavior( + "Attempting to recursively lock a mutex already owned by this thread."); + } + + // 3. Success (no undefined behavior or invalid args). Return nextTransition. + return nextTransition; } std::shared_ptr diff --git a/src/transitions/mutex/MCMutexUnlock.cpp b/src/transitions/mutex/MCMutexUnlock.cpp index ee635349..72384597 100644 --- a/src/transitions/mutex/MCMutexUnlock.cpp +++ b/src/transitions/mutex/MCMutexUnlock.cpp @@ -14,17 +14,39 @@ MCReadMutexUnlock(const MCSharedTransition *shmTransition, mutexThatExists != nullptr, "Attempting to unlock an uninitialized mutex"); + // 1. We've now computed mutexThatExists (i.e., the state of the + // mutex arguments to pthread_mutex_lock()). Compute the nextTransition. + tid_t threadThatRanId = shmTransition->executor; + auto threadThatRan = state->getThreadWithId(threadThatRanId); + MCTransition *nextTransition = + new MCMutexUnlock(threadThatRan, mutexThatExists); + + // 2. Now test for undefined behaviors or invalid arguments. + // If fail, then call setNextTransition on nextTransition, in order to + // display correct results for programState->printNextTransitions(). + if (mutexThatExists == nullptr) { + programState->setNextTransitionForThread(threadThatRanId, std::shared_ptr(nextTransition)); + mc_report_undefined_behavior( + "Attempting to unlock an uninitialized mutex"); + } + if (mutexThatExists->isDestroyed()) { + programState->setNextTransitionForThread(threadThatRanId, std::shared_ptr(nextTransition)); + mc_report_undefined_behavior( + "Attempting to unlock a mutex that has been destroyed"); + } if (mutexThatExists->isUnlocked()) { - MC_REPORT_UNDEFINED_BEHAVIOR( + programState->setNextTransitionForThread(threadThatRanId, std::shared_ptr(nextTransition)); + mc_report_undefined_behavior( "Attempting to unlock a mutex that is already unlocked"); - } else if (mutexThatExists->isDestroyed()) { - MC_REPORT_UNDEFINED_BEHAVIOR( - "Attempting to unlock a mutex that has been destroyed"); + } + if (mutexThatExists->ownerTid() != threadThatRanId) { + programState->setNextTransitionForThread(threadThatRanId, std::shared_ptr(nextTransition)); + mc_report_undefined_behavior( + "Attempting to unlock a mutex that was locked by a different thread."); } - tid_t threadThatRanId = shmTransition->executor; - auto threadThatRan = state->getThreadWithId(threadThatRanId); - return new MCMutexUnlock(threadThatRan, mutexThatExists); + // 3. Success (no undefined behavior or invalid args). Return nextTransition. + return nextTransition; } std::shared_ptr