From c335978c3484febbac731aebe4fd5d6cbfcf6920 Mon Sep 17 00:00:00 2001 From: Cel Andromeda Skeggs Date: Tue, 28 Jan 2025 21:53:29 +0000 Subject: [PATCH 1/3] Improve error checking in mutex stub The default mutex stub is intended to be usable on any platform, even platforms that do not have threads and cannot block. This stub is inappropriate for applications that need to contend over mutexes. However, if inadvertently used in applications in a way that would result in mutex contention, they would silently allow incorrect and dangerous behavior. (Notably, they allowed multiple threads to enter the same critical section.) The new implementation still works on all platforms, and never blocks. However, it ensures that only one thread enters each critical section at a time. Attempting to acquire a mutex that is already taken will result in a failure to acquire the mutex, and likely an assertion. Because this should only occur in the case of a coding defect, this is an improvement over the previous implementation. --- Os/Stub/Mutex.cpp | 15 +++++++++++++++ Os/Stub/Mutex.hpp | 16 +++++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/Os/Stub/Mutex.cpp b/Os/Stub/Mutex.cpp index c430a19b0f7..ebdd0fed0cf 100644 --- a/Os/Stub/Mutex.cpp +++ b/Os/Stub/Mutex.cpp @@ -9,10 +9,25 @@ namespace Stub { namespace Mutex { StubMutex::Status StubMutex::take() { + // Attempt to mark the mutex as taken. + if (this->m_handle.m_mutex_taken.exchange(true)) { + // The mutex was already taken, so fail the operation. + // (This stub is for platforms without the ability to block.) + return Status::ERROR_BUSY; + } + // The mutex was not already taken. + // Now that it has been marked as taken, we have successfully entered the critical section. return Status::OP_OK; } StubMutex::Status StubMutex::release() { + // Attempt to mark the mutex as not taken. + if (!this->m_handle.m_mutex_taken.exchange(false)) { + // The mutex was already not taken, which indicates a coding defect. + return Status::ERROR_OTHER; + } + // The mutex was taken. + // Now that it has been marked as not taken, we have successfully exited the critical section. return Status::OP_OK; } diff --git a/Os/Stub/Mutex.hpp b/Os/Stub/Mutex.hpp index 0b7fa708626..1c0db3cfa16 100644 --- a/Os/Stub/Mutex.hpp +++ b/Os/Stub/Mutex.hpp @@ -4,17 +4,27 @@ // ====================================================================== #include "Os/Mutex.hpp" +#include + #ifndef OS_STUB_MUTEX_HPP #define OS_STUB_MUTEX_HPP namespace Os { namespace Stub { namespace Mutex { -struct StubMutexHandle : public MutexHandle {}; +struct StubMutexHandle : public MutexHandle { + //! True if the mutex has been acquired without being released. + std::atomic m_mutex_taken = {false}; +}; -//! \brief stub implementation of Os::Mutex +//! \brief Nonblocking stub implementation of Os::Mutex +//! +//! Stub implementation of `MutexInterface` for use as a delegate class. //! -//! Stub implementation of `MutexInterface` for use as a delegate class handling error-only file operations. +//! This mutex will never block, which allows it to be used on any platform without OS dependencies. +//! It is unsuitable for use in environments where threads need to contend over mutexes. +//! However, it is appropriate for use in environments with multiple threads that are not intended to +//! contend over mutexes, and where contention would indicate a coding defect worthy of an assertion. //! class StubMutex : public MutexInterface { public: From 720e203bed49cf0d7701484ddbd3506a71452c86 Mon Sep 17 00:00:00 2001 From: Cel Andromeda Skeggs Date: Thu, 13 Feb 2025 21:40:14 +0000 Subject: [PATCH 2/3] Note mutex identity in lock/unlock status assert When a mutex fails to be taken or released, and it causes an assertion to trip, this change makes sure that enough information is provided to uniquely identify which mutex was at fault. --- Os/Mutex.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Os/Mutex.cpp b/Os/Mutex.cpp index 27ff04d7c88..3d63ceb2f8a 100644 --- a/Os/Mutex.cpp +++ b/Os/Mutex.cpp @@ -34,13 +34,13 @@ Mutex::Status Mutex::release() { void Mutex::lock() { FW_ASSERT(&this->m_delegate == reinterpret_cast(&this->m_handle_storage[0])); Mutex::Status status = this->take(); - FW_ASSERT(status == Mutex::Status::OP_OK, status); + FW_ASSERT(status == Mutex::Status::OP_OK, static_cast(reinterpret_cast(this)), status); } void Mutex::unLock() { FW_ASSERT(&this->m_delegate == reinterpret_cast(&this->m_handle_storage[0])); Mutex::Status status = this->release(); - FW_ASSERT(status == Mutex::Status::OP_OK, status); + FW_ASSERT(status == Mutex::Status::OP_OK, static_cast(reinterpret_cast(this)), status); } ScopeLock::ScopeLock(Mutex& mutex) : m_mutex(mutex) { From 3707174fc3691e54bfd810ceeb361eca108f38b6 Mon Sep 17 00:00:00 2001 From: Cel Andromeda Skeggs Date: Fri, 14 Feb 2025 01:53:08 +0000 Subject: [PATCH 3/3] Fix assertion cast argument types to be generic --- Os/Mutex.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Os/Mutex.cpp b/Os/Mutex.cpp index 3d63ceb2f8a..f3d140149de 100644 --- a/Os/Mutex.cpp +++ b/Os/Mutex.cpp @@ -34,13 +34,13 @@ Mutex::Status Mutex::release() { void Mutex::lock() { FW_ASSERT(&this->m_delegate == reinterpret_cast(&this->m_handle_storage[0])); Mutex::Status status = this->take(); - FW_ASSERT(status == Mutex::Status::OP_OK, static_cast(reinterpret_cast(this)), status); + FW_ASSERT(status == Mutex::Status::OP_OK, static_cast(reinterpret_cast(this)), status); } void Mutex::unLock() { FW_ASSERT(&this->m_delegate == reinterpret_cast(&this->m_handle_storage[0])); Mutex::Status status = this->release(); - FW_ASSERT(status == Mutex::Status::OP_OK, static_cast(reinterpret_cast(this)), status); + FW_ASSERT(status == Mutex::Status::OP_OK, static_cast(reinterpret_cast(this)), status); } ScopeLock::ScopeLock(Mutex& mutex) : m_mutex(mutex) {