From 8fba8f88e1f19f585da71417db5fa12aa97a0609 Mon Sep 17 00:00:00 2001 From: igor-krechetov <58586005+igor-krechetov@users.noreply.github.com> Date: Mon, 6 Apr 2026 13:09:43 +0100 Subject: [PATCH] [1.0.4][r] fix potential multithreading issues Fix - Fixed potential race-conditions in dispatchers - Replaced copy with move operations in hsm --- CHANGELOG.md | 9 ++++ CMakeLists.txt | 2 +- README.md | 2 +- include/hsmcpp/hsm.hpp | 13 +++--- include/hsmcpp/os/common/UniqueLock.hpp | 12 +++++- scripts/local/validate.sh | 9 +--- src/HsmEventDispatcherBase.cpp | 1 + src/HsmEventDispatcherQt.cpp | 5 ++- src/HsmEventDispatcherSTD.cpp | 8 +++- src/HsmImpl.cpp | 2 +- src/os/common/UniqueLock.cpp | 55 +++++++++++++++++-------- 11 files changed, 78 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3271272..3df3628 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,15 @@ # Changelog All notable changes to project will be documented in this file. +## [1.0.4] - 2026-04-06 +### Fixed +- Fixed potential race-conditions in dispatchers +- Replaced copy with move operations in hsm + +## [1.0.3] - 2026-04-06 +### Fixed +- Fixed unused parameter compiler warnings/errors in generated code + ## [1.0.2] - 2024-05-31 ### Fixed - fixed handling of tags during code generation (transition targets were getting double prefix which was breaking the build) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3811c17..e437a02 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -3,7 +3,7 @@ cmake_minimum_required(VERSION 3.16) project(hsmcpp) -set(PROJECT_VERSION "1.0.2") +set(PROJECT_VERSION "1.0.4") set(PROJECT_DESCRIPTION "C++ library for hierarchical state machines / finite state machines. Provides a code-free visual approach for defining state machine logic using GUI editors with automatic code and diagram generation. Check out https://hsmcpp.readthedocs.io for detailed documentation.") set(CMAKE_VERBOSE_MAKEFILE OFF) diff --git a/README.md b/README.md index 4de4825..5e8b101 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,5 @@ [![MIT License](https://img.shields.io/badge/license-MIT-blue.svg)](https://github.com/igor-krechetov/hsmcpp/blob/main/LICENSE) -[![Changelog](https://img.shields.io/badge/changelog-v1.0.2-green.svg)](https://github.com/igor-krechetov/hsmcpp/blob/main/CHANGELOG.md) +[![Changelog](https://img.shields.io/badge/changelog-v1.0.4-green.svg)](https://github.com/igor-krechetov/hsmcpp/blob/main/CHANGELOG.md) [![Documentation Status](https://readthedocs.org/projects/hsmcpp/badge/?version=latest)](https://hsmcpp.readthedocs.io/en/latest/?badge=latest) # Releases diff --git a/include/hsmcpp/hsm.hpp b/include/hsmcpp/hsm.hpp index d12d771..62717ab 100644 --- a/include/hsmcpp/hsm.hpp +++ b/include/hsmcpp/hsm.hpp @@ -7,6 +7,7 @@ #include #include #include +#include #include "HsmTypes.hpp" #include "variant.hpp" @@ -734,7 +735,7 @@ void HierarchicalStateMachine::registerState(const StateID_t state, } } - registerState(state, funcStateChanged, funcEntering, funcExiting); + registerState(state, std::move(funcStateChanged), std::move(funcEntering), std::move(funcExiting)); } template @@ -762,7 +763,7 @@ void HierarchicalStateMachine::registerFinalState(const StateID_t state, } } - registerFinalState(state, event, funcStateChanged, funcEntering, funcExiting); + registerFinalState(state, event, std::move(funcStateChanged), std::move(funcEntering), std::move(funcExiting)); } template @@ -780,7 +781,7 @@ void HierarchicalStateMachine::registerHistory(const StateID_t parent, } } - registerHistory(parent, historyState, type, defaultTarget, funcTransitionCallback); + registerHistory(parent, historyState, type, defaultTarget, std::move(funcTransitionCallback)); } template @@ -797,7 +798,7 @@ bool HierarchicalStateMachine::registerSubstateEntryPoint(const StateID_t parent condition = std::bind(conditionCallback, handler, std::placeholders::_1); } - return registerSubstateEntryPoint(parent, substate, onEvent, condition, expectedConditionValue); + return registerSubstateEntryPoint(parent, substate, onEvent, std::move(condition), expectedConditionValue); } template @@ -832,7 +833,7 @@ void HierarchicalStateMachine::registerTransition(const StateID_t fromState, } } - registerTransition(fromState, toState, onEvent, funcTransitionCallback, funcConditionCallback, expectedConditionValue); + registerTransition(fromState, toState, onEvent, std::move(funcTransitionCallback), std::move(funcConditionCallback), expectedConditionValue); } template @@ -856,7 +857,7 @@ void HierarchicalStateMachine::registerSelfTransition(const StateID_t state, } } - registerSelfTransition(state, onEvent, type, funcTransitionCallback, funcConditionCallback, expectedConditionValue); + registerSelfTransition(state, onEvent, type, std::move(funcTransitionCallback), std::move(funcConditionCallback), expectedConditionValue); } template diff --git a/include/hsmcpp/os/common/UniqueLock.hpp b/include/hsmcpp/os/common/UniqueLock.hpp index 10a8b05..0c237bd 100644 --- a/include/hsmcpp/os/common/UniqueLock.hpp +++ b/include/hsmcpp/os/common/UniqueLock.hpp @@ -3,6 +3,8 @@ #ifndef HSMCPP_OS_COMMON_UNIQUELOCK_HPP #define HSMCPP_OS_COMMON_UNIQUELOCK_HPP +#include + namespace hsmcpp { @@ -24,7 +26,13 @@ class UniqueLock inline bool owns_lock() const noexcept { - return mOwnsLock; + const bool isLocked = const_cast(mOwnsLock).test_and_set(std::memory_order_acquire); + + if (false == isLocked) { + const_cast(mOwnsLock).clear(std::memory_order_release); + } + + return isLocked; } inline explicit operator bool() const noexcept @@ -44,7 +52,7 @@ class UniqueLock private: Mutex* mSync = nullptr; - bool mOwnsLock = false; + mutable std::atomic_flag mOwnsLock = ATOMIC_FLAG_INIT; }; } // namespace hsmcpp diff --git a/scripts/local/validate.sh b/scripts/local/validate.sh index 64dd244..9c4bc2d 100755 --- a/scripts/local/validate.sh +++ b/scripts/local/validate.sh @@ -1,11 +1,5 @@ #!/bin/sh -# python3 ./scripts/local/validate_metadata.py $1 ./ -# if [ $? != 0 ] -# then -# exit 1 -# fi - mkdir ./build cd ./build @@ -67,8 +61,7 @@ then -DHSMBUILD_CLANGTIDY=OFF \ .. - cppcheck --addon=../scripts/sca/cppcheck/misra.json \ - --enable=warning,performance,portability,information \ + cppcheck --enable=warning,performance,portability,information \ --inline-suppr \ --xml \ --suppressions-list=../scripts/sca/cppcheck/cppcheck_suppress.txt --project=./compile_commands.json \ diff --git a/src/HsmEventDispatcherBase.cpp b/src/HsmEventDispatcherBase.cpp index b1c8812..496ee9d 100644 --- a/src/HsmEventDispatcherBase.cpp +++ b/src/HsmEventDispatcherBase.cpp @@ -207,6 +207,7 @@ void HsmEventDispatcherBase::stopTimer(const TimerID_t timerID) { } bool HsmEventDispatcherBase::isTimerRunning(const TimerID_t timerID) { + LockGuard lck(mHandlersSync); return (mActiveTimers.find(timerID) != mActiveTimers.end()); } diff --git a/src/HsmEventDispatcherQt.cpp b/src/HsmEventDispatcherQt.cpp index 4b79444..24a05c6 100644 --- a/src/HsmEventDispatcherQt.cpp +++ b/src/HsmEventDispatcherQt.cpp @@ -95,9 +95,12 @@ void HsmEventDispatcherQt::startTimerImpl(const TimerID_t timerID, const unsigne SC2INT(timerID), intervalMs, BOOL2INT(isSingleShot)); + mRunningTimersSync.lock(); auto it = mNativeTimerHandlers.find(timerID); + const bool timerDoesntExist = (mNativeTimerHandlers.end() == it); + mRunningTimersSync.unlock(); - if (mNativeTimerHandlers.end() == it) { + if (timerDoesntExist) { auto funcCreateTimer = [=]() { QTimer* newTimer = new QTimer(this); diff --git a/src/HsmEventDispatcherSTD.cpp b/src/HsmEventDispatcherSTD.cpp index 2f216a7..1b6d1cb 100644 --- a/src/HsmEventDispatcherSTD.cpp +++ b/src/HsmEventDispatcherSTD.cpp @@ -82,7 +82,6 @@ void HsmEventDispatcherSTD::join() { void HsmEventDispatcherSTD::startTimerImpl(const TimerID_t timerID, const unsigned int intervalMs, const bool isSingleShot) { HSM_TRACE_CALL_ARGS("timerID=%d, intervalMs=%d, isSingleShot=%d", SC2INT(timerID), intervalMs, BOOL2INT(isSingleShot)); - auto it = mRunningTimers.end(); // lazy initialization of timers thread if (false == mTimersThread.joinable()) { @@ -91,7 +90,7 @@ void HsmEventDispatcherSTD::startTimerImpl(const TimerID_t timerID, const unsign { CriticalSection cs(mRunningTimersSync); - it = mRunningTimers.find(timerID); + auto it = mRunningTimers.find(timerID); // new timer if (mRunningTimers.end() == it) { @@ -144,6 +143,8 @@ void HsmEventDispatcherSTD::doDispatching() { // cppcheck-suppress misra-c2012-15.5 mEmitEvent.wait(lck, [=]() { // cppcheck-suppress misra-c2012-15.5 ; false-positive. "return" statement belongs to lambda function + // NOTE: it's assumed that calling empty() is thread-safe. Even if due to a race condition we get + // wrong value it will only cause a small delay in event processing, but won't cause any critical issues return (false == mPendingEvents.empty()) || (false == mEnqueuedEvents.empty()) || (true == mStopDispatcher); }); HSM_TRACE_DEBUG("woke up. pending events=%lu", mPendingEvents.size()); @@ -168,6 +169,9 @@ void HsmEventDispatcherSTD::handleTimers() { while (false == mStopDispatcher) { mRunningTimersSync.lock(); + // NOTE: it's assumed that calling empty() is thread-safe. Even if due to + // a race condition we get wrong value it will only cause a small delay in timer processing, + // but won't cause any critical issues if (false == mRunningTimers.empty()) { auto itTimeout = mRunningTimers.begin(); diff --git a/src/HsmImpl.cpp b/src/HsmImpl.cpp index 481fa81..fab37c4 100644 --- a/src/HsmImpl.cpp +++ b/src/HsmImpl.cpp @@ -1490,7 +1490,7 @@ bool HierarchicalStateMachine::Impl::processFinalStateTransition(const PendingEv { HSM_SYNC_EVENTS_QUEUE(); - mPendingEvents.push_front(finalStateEvent); + mPendingEvents.push_front(std::move(finalStateEvent)); } } } diff --git a/src/os/common/UniqueLock.cpp b/src/os/common/UniqueLock.cpp index de7f336..ead8bbb 100644 --- a/src/os/common/UniqueLock.cpp +++ b/src/os/common/UniqueLock.cpp @@ -7,8 +7,8 @@ namespace hsmcpp { UniqueLock::UniqueLock(Mutex& sync) -// NOTE: false-positive. thinks that ':' is arithmetic operation -// cppcheck-suppress misra-c2012-10.4 + // NOTE: false-positive. thinks that ':' is arithmetic operation + // cppcheck-suppress misra-c2012-10.4 : mSync(&sync) { lock(); } @@ -18,37 +18,56 @@ UniqueLock::~UniqueLock() { } UniqueLock::UniqueLock(UniqueLock&& src) noexcept - : mSync(src.mSync) - , mOwnsLock(src.mOwnsLock) { + : mSync(src.mSync) { + if (true == src.mOwnsLock.test_and_set(std::memory_order_acquire)) { + mOwnsLock.test_and_set(std::memory_order_release); + } else { + mOwnsLock.clear(std::memory_order_release); + } + src.mSync = nullptr; - src.mOwnsLock = false; + src.mOwnsLock.clear(std::memory_order_release); } UniqueLock& UniqueLock::operator=(UniqueLock&& src) noexcept { - if (mOwnsLock) { - unlock(); - } + if (this != &src) { + if (nullptr != mSync) { + if (true == mOwnsLock.test_and_set(std::memory_order_acquire)) { + mSync->unlock(); + } else { + mOwnsLock.clear(std::memory_order_release); + } + } - mSync = src.mSync; - mOwnsLock = src.mOwnsLock; + mSync = src.mSync; - src.mSync = nullptr; - src.mOwnsLock = false; + if (true == src.mOwnsLock.test_and_set(std::memory_order_acquire)) { + mOwnsLock.test_and_set(std::memory_order_release); + } else { + mOwnsLock.clear(std::memory_order_release); + } + + src.mSync = nullptr; + src.mOwnsLock.clear(std::memory_order_release); + } return *this; } void UniqueLock::lock() { - if ((nullptr != mSync) && (false == mOwnsLock)) { + if ((nullptr != mSync) && (false == mOwnsLock.test_and_set(std::memory_order_acq_rel))) { mSync->lock(); - mOwnsLock = true; } } void UniqueLock::unlock() { - if ((nullptr != mSync) && (true == mOwnsLock)) { - mSync->unlock(); - mOwnsLock = false; + if (nullptr != mSync) { + if (true == mOwnsLock.test_and_set(std::memory_order_acquire)) { + mSync->unlock(); + mOwnsLock.clear(std::memory_order_release); + } else { + mOwnsLock.clear(std::memory_order_release); + } } } @@ -56,7 +75,7 @@ Mutex* UniqueLock::release() noexcept { Mutex* res = mSync; mSync = nullptr; - mOwnsLock = false; + mOwnsLock.clear(std::memory_order_release); return res; }