From f711cfa162bf8164608dbbfd04d5a886019ba1bb Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:33:00 +0200 Subject: [PATCH 01/13] build: pin bzip2 and jansson instead of tracking master bzip2 is pinned to the current master commit (1.0.8 has no CMake build); jansson moves to the v2.15.1 release, which builds with CMake 4. --- cmake/FetchDependencies.cmake | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/cmake/FetchDependencies.cmake b/cmake/FetchDependencies.cmake index e3219e01d..4c82a8c12 100644 --- a/cmake/FetchDependencies.cmake +++ b/cmake/FetchDependencies.cmake @@ -12,23 +12,24 @@ message(STATUS "Fetching MafiaNet dependencies...") # ----------------------------------------------------------------------------- # bzip2 - compression library for Autopatcher -# Note: Using master branch as it has CMake support (1.0.8 tag does not) +# Note: the 1.0.8 release tag has no CMake build, so this pins a post-1.0.8 +# master commit. Shallow clones cannot fetch an arbitrary SHA from every +# server, hence GIT_SHALLOW FALSE. # ----------------------------------------------------------------------------- FetchContent_Declare( bzip2 GIT_REPOSITORY https://gitlab.com/bzip2/bzip2.git - GIT_TAG master - GIT_SHALLOW TRUE + GIT_TAG 66c46b8c9436613fd81bc5d03f63a61933a4dcc3 # master, 2023-05-31 + GIT_SHALLOW FALSE ) # ----------------------------------------------------------------------------- # jansson - JSON library for ComprehensivePCGame master server communication -# Note: Using master branch as v2.14.1 has CMake compatibility issues with cmake_minimum_required # ----------------------------------------------------------------------------- FetchContent_Declare( jansson GIT_REPOSITORY https://github.com/akheron/jansson.git - GIT_TAG master + GIT_TAG v2.15.1 GIT_SHALLOW TRUE ) From 66db7ab041de089a822a98fa04c974fe8531b930 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:33:00 +0200 Subject: [PATCH 02/13] build: add sanitizer option, compiler cache detection and presets - MAFIANET_SANITIZER builds every target with -fsanitize=. - sccache/ccache is used as the compiler launcher when found on PATH (MAFIANET_USE_COMPILER_CACHE, on by default). - Presets: minimal, asan, tsan, plus test presets (unit, integration, per-config) and workflow presets, so 'cmake --workflow --preset asan' configures, builds and tests in one command. Presets move to schema 6 (CMake 3.25); the project itself still needs only 3.21. --- CLAUDE.md | 8 ++ CMakeLists.txt | 31 ++++++++ CMakePresets.json | 124 ++++++++++++++++++++++++++++-- docs/getting-started/building.rst | 58 ++++++++++++++ 4 files changed, 216 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b72aa8abf..cdfe8c4c1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -65,6 +65,14 @@ Available generators (run `cmake --help` for full list): | `MAFIANET_BUILD_EXTRAS` | OFF | Build `MafiaNetExtras` (EmailSender, HTTPConnection*, Telnet/RakNet transports, ConsoleServer, command parsers, PacketConsoleLogger); forced ON by `MAFIANET_BUILD_SAMPLES` | | `MAFIANET_DISABLED_FEATURES` | (empty) | Semicolon list of plugins to compile out (`_RAKNET_SUPPORT_=0`); names validated against `MAFIANET_OPTIONAL_FEATURES` in `Source/CMakeLists.txt` | | `MAFIANET_MINIMAL` | OFF | Compile out every optional plugin (core transport only); built by the `linux-minimal` CI job | +| `MAFIANET_SANITIZER` | (empty) | `address+undefined` or `thread`; applied to every target. Prefer the `asan` / `tsan` presets | +| `MAFIANET_USE_COMPILER_CACHE` | ON | Use sccache/ccache as compiler launcher when found on `PATH` | + +Presets (`CMakePresets.json`, CMake 3.25+): `debug`, `release`, `ci`, `minimal`, `asan`, `tsan`. +`cmake --workflow --preset ` configures, builds and tests in one go; run +sanitizer tests through `ctest --preset asan|tsan` so the sanitizer options and +`ci/tsan.supp` are applied. Known by-design TSan races live in `ci/tsan.supp`; add an entry only +for a race that is part of RakNet's lock-free getter model, never to silence a new bug. When adding or removing a plugin guarded by a `_RAKNET_SUPPORT_` flag, update `MAFIANET_OPTIONAL_FEATURES` in `Source/CMakeLists.txt` as well, or the CMake diff --git a/CMakeLists.txt b/CMakeLists.txt index b03827493..26a79392e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -54,11 +54,42 @@ set(MAFIANET_DISABLED_FEATURES "" CACHE STRING "Semicolon-separated plugin names to compile out (see MAFIANET_OPTIONAL_FEATURES in Source/CMakeLists.txt)") option(MAFIANET_MINIMAL "Compile out every optional plugin (core transport only)" OFF) +# Sanitizers (Clang/GCC only): a "+"- or ","-separated list such as +# "address+undefined" or "thread" ("thread" cannot be combined with "address"). +# Recoverable UBSan findings are made fatal so they fail the test that hit them. +set(MAFIANET_SANITIZER "" CACHE STRING "Build with sanitizers (e.g. address+undefined, thread)") + +# Compiler cache. sccache or ccache is picked up automatically when installed, +# unless the caller already chose a launcher (CI passes one explicitly). +option(MAFIANET_USE_COMPILER_CACHE "Use sccache/ccache when available" ON) + # Ensure at least one library type is built if(NOT MAFIANET_BUILD_SHARED AND NOT MAFIANET_BUILD_STATIC) message(FATAL_ERROR "At least one of MAFIANET_BUILD_SHARED or MAFIANET_BUILD_STATIC must be ON") endif() +if(MAFIANET_USE_COMPILER_CACHE AND NOT CMAKE_CXX_COMPILER_LAUNCHER AND NOT CMAKE_C_COMPILER_LAUNCHER) + find_program(MAFIANET_COMPILER_CACHE NAMES sccache ccache) + if(MAFIANET_COMPILER_CACHE) + message(STATUS "MafiaNet: compiler cache ${MAFIANET_COMPILER_CACHE}") + set(CMAKE_C_COMPILER_LAUNCHER "${MAFIANET_COMPILER_CACHE}") + set(CMAKE_CXX_COMPILER_LAUNCHER "${MAFIANET_COMPILER_CACHE}") + endif() +endif() + +# Applied globally, so FetchContent dependencies are instrumented too: a +# sanitizer runtime with half the program uninstrumented reports false positives +# (notably TSan) and misses overflows that cross into third-party code. +if(MAFIANET_SANITIZER) + if(MSVC) + message(FATAL_ERROR "MAFIANET_SANITIZER is not supported with MSVC") + endif() + string(REPLACE "+" "," _mafianet_san "${MAFIANET_SANITIZER}") + message(STATUS "MafiaNet: sanitizers ${_mafianet_san}") + add_compile_options(-fsanitize=${_mafianet_san} -fno-sanitize-recover=all -fno-omit-frame-pointer) + add_link_options(-fsanitize=${_mafianet_san}) +endif() + # Dependencies find_package(OpenSSL 3.0.0 REQUIRED) find_package(Threads REQUIRED) diff --git a/CMakePresets.json b/CMakePresets.json index ae052994e..583f15329 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -1,8 +1,8 @@ { - "version": 4, + "version": 6, "cmakeMinimumRequired": { "major": 3, - "minor": 21, + "minor": 25, "patch": 0 }, "configurePresets": [ @@ -41,20 +41,134 @@ "MAFIANET_BUILD_SAMPLES": "ON", "MAFIANET_BUILD_TESTS": "ON" } + }, + { + "name": "minimal", + "inherits": "base", + "displayName": "Minimal (core transport only)", + "cacheVariables": { + "CMAKE_BUILD_TYPE": "Release", + "MAFIANET_MINIMAL": "ON" + } + }, + { + "name": "sanitizer-base", + "hidden": true, + "inherits": "base", + "description": "RelWithDebInfo, not Debug: _DEBUG enables RakAsserts that abort before the sanitizer can report, and NDEBUG is what ships.", + "condition": { + "type": "notEquals", + "lhs": "${hostSystemName}", + "rhs": "Windows" + }, + "cacheVariables": { + "CMAKE_BUILD_TYPE": "RelWithDebInfo", + "MAFIANET_BUILD_TESTS": "ON" + } + }, + { + "name": "asan", + "inherits": "sanitizer-base", + "displayName": "AddressSanitizer + UBSan", + "cacheVariables": { + "MAFIANET_SANITIZER": "address+undefined" + } + }, + { + "name": "tsan", + "inherits": "sanitizer-base", + "displayName": "ThreadSanitizer", + "cacheVariables": { + "MAFIANET_SANITIZER": "thread" + } } ], "buildPresets": [ + { "name": "debug", "configurePreset": "debug" }, + { "name": "release", "configurePreset": "release" }, + { "name": "ci", "configurePreset": "ci" }, + { "name": "minimal", "configurePreset": "minimal" }, + { "name": "asan", "configurePreset": "asan" }, + { "name": "tsan", "configurePreset": "tsan" } + ], + "testPresets": [ + { + "name": "test-base", + "hidden": true, + "output": { "outputOnFailure": true }, + "execution": { "timeout": 600 } + }, { "name": "debug", + "inherits": "test-base", "configurePreset": "debug" }, { - "name": "release", - "configurePreset": "release" + "name": "unit", + "inherits": "test-base", + "displayName": "Unit tests (debug)", + "configurePreset": "debug", + "filter": { "include": { "label": "unit" } }, + "execution": { "jobs": 0 } + }, + { + "name": "integration", + "inherits": "test-base", + "displayName": "Integration tests (debug)", + "configurePreset": "debug", + "filter": { "include": { "label": "integration" } } }, { "name": "ci", - "configurePreset": "ci" + "inherits": "test-base", + "configurePreset": "ci", + "configuration": "Release" + }, + { + "name": "asan", + "inherits": "test-base", + "configurePreset": "asan", + "environment": { + "ASAN_OPTIONS": "detect_leaks=0:abort_on_error=1:halt_on_error=1", + "UBSAN_OPTIONS": "print_stacktrace=1:halt_on_error=1" + } + }, + { + "name": "tsan", + "inherits": "test-base", + "configurePreset": "tsan", + "environment": { + "TSAN_OPTIONS": "halt_on_error=1:second_deadlock_stack=1:suppressions=${sourceDir}/ci/tsan.supp" + } + } + ], + "workflowPresets": [ + { + "name": "debug", + "displayName": "Configure, build and test (Debug)", + "steps": [ + { "type": "configure", "name": "debug" }, + { "type": "build", "name": "debug" }, + { "type": "test", "name": "debug" } + ] + }, + { + "name": "asan", + "displayName": "Configure, build and test under ASan + UBSan", + "steps": [ + { "type": "configure", "name": "asan" }, + { "type": "build", "name": "asan" }, + { "type": "test", "name": "asan" } + ] + }, + { + "name": "tsan", + "displayName": "Configure, build and test under TSan", + "steps": [ + { "type": "configure", "name": "tsan" }, + { "type": "build", "name": "tsan" }, + { "type": "test", "name": "tsan" } + ] } ] } diff --git a/docs/getting-started/building.rst b/docs/getting-started/building.rst index f16365f2b..3d29b976d 100644 --- a/docs/getting-started/building.rst +++ b/docs/getting-started/building.rst @@ -62,6 +62,12 @@ Configure build options with CMake: * - ``MAFIANET_MINIMAL`` - OFF - Compile out every optional plugin, leaving the core transport only. See `Minimal builds`_. + * - ``MAFIANET_SANITIZER`` + - (empty) + - Clang/GCC sanitizers, ``+``-separated: ``address+undefined`` or ``thread``. Applied to every target, dependencies included. See `Sanitizers`_. + * - ``MAFIANET_USE_COMPILER_CACHE`` + - ON + - Use ``sccache`` or ``ccache`` as the compiler launcher when one is on ``PATH``. An explicit ``CMAKE__COMPILER_LAUNCHER`` takes precedence. Example with options: @@ -197,6 +203,58 @@ on Linux, flip the ``#if defined(__linux__)`` batching guards in ``socket2.cpp``, ``socket2_berkley.cpp``, ``reliability_layer.cpp`` and ``RakNetSocket2.h`` to ``#if 0``. +CMake presets +------------- + +``CMakePresets.json`` (CMake 3.25+) covers the common configurations. Each +preset builds into ``build/``: + +.. list-table:: + :header-rows: 1 + :widths: 20 80 + + * - Preset + - Configuration + * - ``debug`` + - Debug, samples and tests. ``.clangd`` reads this tree's ``compile_commands.json``. + * - ``release`` + - Release, libraries only. + * - ``ci`` + - Release, samples and tests. + * - ``minimal`` + - Release with ``MAFIANET_MINIMAL=ON``. + * - ``asan`` / ``tsan`` + - RelWithDebInfo and tests under AddressSanitizer + UBSan, or ThreadSanitizer. Not available on Windows. + +Workflow presets configure, build and test in one command: + +.. code-block:: bash + + cmake --workflow --preset debug + cmake --workflow --preset asan + + # Or step by step; test presets "unit" and "integration" filter by label + cmake --preset debug && cmake --build --preset debug + ctest --preset unit + +Sanitizers +---------- + +The ``asan`` and ``tsan`` presets build with ``RelWithDebInfo`` rather than +``Debug``: ``_DEBUG`` enables ``RakAssert`` checks that abort before the +sanitizer can report, and ``NDEBUG`` is what ships. Their test presets set +``ASAN_OPTIONS`` / ``UBSAN_OPTIONS`` / ``TSAN_OPTIONS``, so run them through +``ctest --preset`` rather than plain ``ctest``. + +ThreadSanitizer reports RakNet's by-design unlocked reads of connection state +from the public getters. Those are listed in ``ci/tsan.supp``; the CI TSan job +is advisory and exists to catch new races. On Linux hosts with +``vm.mmap_rnd_bits`` above 28, TSan may fail to start; lower it with +``sudo sysctl vm.mmap_rnd_bits=28``. + +On macOS, the ``.devcontainer`` gives a Linux environment for running the +sanitizers and the Linux-only batched datagram paths. + Running Tests ------------- From c6a382ae26a47a381b79b07eaa019998bfa6308b Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:33:00 +0200 Subject: [PATCH 03/13] fix(core): do not memcpy from a null password SendConnectionRequest copied passwordDataLength bytes from passwordData even when no password is set, passing a null source to memcpy. That is undefined behaviour even for a length of 0 (glibc declares the argument nonnull), and UBSan aborts on it in every connecting integration test. --- Source/src/peer.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Source/src/peer.cpp b/Source/src/peer.cpp index dd6df7ecd..5bdce011f 100644 --- a/Source/src/peer.cpp +++ b/Source/src/peer.cpp @@ -3430,7 +3430,10 @@ ConnectionAttemptResult RakPeer::SendConnectionRequest( const char* host, unsign rcs->actionToTake=RequestedConnectionStruct::CONNECT; rcs->sendConnectionAttemptCount=sendConnectionAttemptCount; rcs->timeBetweenSendConnectionAttemptsMS=timeBetweenSendConnectionAttemptsMS; - memcpy(rcs->outgoingPassword, passwordData, passwordDataLength); + // No password means passwordData is null. memcpy's source is declared nonnull, + // so a null source is undefined behaviour even with a length of 0. + if (passwordDataLength > 0) + memcpy(rcs->outgoingPassword, passwordData, passwordDataLength); rcs->outgoingPasswordLength=(unsigned char) passwordDataLength; rcs->timeoutTime=timeoutTime; @@ -3484,7 +3487,10 @@ ConnectionAttemptResult RakPeer::SendConnectionRequest( const char* host, unsign rcs->actionToTake=RequestedConnectionStruct::CONNECT; rcs->sendConnectionAttemptCount=sendConnectionAttemptCount; rcs->timeBetweenSendConnectionAttemptsMS=timeBetweenSendConnectionAttemptsMS; - memcpy(rcs->outgoingPassword, passwordData, passwordDataLength); + // No password means passwordData is null. memcpy's source is declared nonnull, + // so a null source is undefined behaviour even with a length of 0. + if (passwordDataLength > 0) + memcpy(rcs->outgoingPassword, passwordData, passwordDataLength); rcs->outgoingPasswordLength=(unsigned char) passwordDataLength; rcs->timeoutTime=timeoutTime; rcs->socket=socket; From c8c7d4df334e9007f5da9a57a497d7668cfa289c Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:33:00 +0200 Subject: [PATCH 04/13] fix(core): make thread lifecycle flags and the lockless counter atomic ThreadSanitizer reports races on all of these: - RakPeer::endThreads / isMainLoopThreadActive and RNS2_Berkley::endThreads were volatile bool spin-wait flags. volatile gives no happens-before edge between the network thread and Startup/Shutdown; std::atomic does. - LocklessUint32_t::GetValue read the counter without the barrier the writers used. It is now std::atomic on every platform, which also fixes Increment/Decrement returning the old value on Linux, contrary to the documented contract (no caller used the return value). - GetTimeUS_Linux lazily initialised its time base through unsynchronised globals; it now uses a function-local static. --- Source/include/mafianet/lockless_types.h | 17 ++---- Source/include/mafianet/peer.h | 11 ++-- Source/include/mafianet/socket2.h | 4 +- Source/src/get_time.cpp | 17 +++--- Source/src/lockless_types.cpp | 35 +++--------- Tests/Unit/LocklessTypesTests.cpp | 69 ++++++++++++++++++++++++ 6 files changed, 99 insertions(+), 54 deletions(-) create mode 100644 Tests/Unit/LocklessTypesTests.cpp diff --git a/Source/include/mafianet/lockless_types.h b/Source/include/mafianet/lockless_types.h index 680cf8496..f472cb69c 100644 --- a/Source/include/mafianet/lockless_types.h +++ b/Source/include/mafianet/lockless_types.h @@ -19,10 +19,7 @@ #include "mafianet/export.h" #include "mafianet/native_types.h" #include "mafianet/windows_includes.h" -#if defined(ANDROID) || defined(__S3E__) || defined(__APPLE__) -// __sync_fetch_and_add not supported apparently -#include "mafianet/simple_mutex.h" -#endif +#include namespace MafiaNet { @@ -36,18 +33,10 @@ class RAK_DLL_EXPORT LocklessUint32_t uint32_t Increment(void); // Returns variable value after changing it uint32_t Decrement(void); - uint32_t GetValue(void) const {return value;} + uint32_t GetValue(void) const {return value.load();} protected: -#ifdef _WIN32 - volatile LONG value; -#elif defined(ANDROID) || defined(__S3E__) || defined(__APPLE__) - // __sync_fetch_and_add not supported apparently - SimpleMutex mutex; - uint32_t value; -#else - volatile uint32_t value; -#endif + std::atomic value; }; } diff --git a/Source/include/mafianet/peer.h b/Source/include/mafianet/peer.h index fb4322882..37173f912 100644 --- a/Source/include/mafianet/peer.h +++ b/Source/include/mafianet/peer.h @@ -41,6 +41,7 @@ #include "mafianet/secure_handshake.h" #include "mafianet/lockless_types.h" #include "mafianet/ds_queue.h" +#include namespace MafiaNet { /// Forward declarations @@ -847,10 +848,12 @@ class RAK_DLL_EXPORT RakPeer : public RakPeerInterface, public RNS2EventHandler bool IsLoopbackAddress(const AddressOrGUID &systemIdentifier, bool matchPort) const; SystemAddress GetLoopbackAddress(void) const; - ///Set this to true to terminate the Peer thread execution - volatile bool endThreads; - ///true if the peer thread is active. - volatile bool isMainLoopThreadActive; + /// Set this to true to terminate the Peer thread execution. + /// Atomic, not volatile: the update thread and Startup/Shutdown hand off + /// through these flags, and volatile gives no happens-before edge. + std::atomic endThreads; + /// true if the peer thread is active. + std::atomic isMainLoopThreadActive; // MafiaNet::LocklessUint32_t isRecvFromLoopThreadActive; diff --git a/Source/include/mafianet/socket2.h b/Source/include/mafianet/socket2.h index f7cecf248..625937ae2 100644 --- a/Source/include/mafianet/socket2.h +++ b/Source/include/mafianet/socket2.h @@ -22,6 +22,7 @@ #include "mafianet/thread.h" #include "mafianet/ds_threadsafe_allocating_queue.h" #include "mafianet/export.h" +#include // Batched datagram I/O (recvmmsg / sendmmsg) is guarded by a plain // `#if defined(__linux__)` wherever it appears. There is no macro and no build @@ -262,7 +263,8 @@ class RNS2_Berkley : public IRNS2_Berkley // same guard, and only defined on that platform. void RecvFromBatchedLoop(void); MafiaNet::LocklessUint32_t isRecvFromLoopThreadActive; - volatile bool endThreads; + // Atomic, not volatile: set by the owning thread, polled by the recv thread. + std::atomic endThreads; // Constructor not called! #if defined(__APPLE__) diff --git a/Source/src/get_time.cpp b/Source/src/get_time.cpp index 810254599..a48eca7e7 100644 --- a/Source/src/get_time.cpp +++ b/Source/src/get_time.cpp @@ -46,10 +46,11 @@ #else #include #include -MafiaNet::TimeUS initialTime; #endif +#if defined(_WIN32) static bool initialized=false; +#endif #if defined(GET_TIME_SPIKE_LIMIT) && GET_TIME_SPIKE_LIMIT>0 #include "mafianet/simple_mutex.h" @@ -180,13 +181,15 @@ MafiaNet::TimeUS GetTimeUS_Windows( void ) MafiaNet::TimeUS GetTimeUS_Linux( void ) { timeval tp; - if ( initialized == false) + // Time is reported relative to the first call, otherwise MafiaNet::Time in + // milliseconds underflows when dividing by 1000. A function-local static is + // initialised exactly once even when the network and user threads race here. + static const MafiaNet::TimeUS initialTime = [] { - gettimeofday( &tp, 0 ); - initialized=true; - // I do this because otherwise MafiaNet::Time in milliseconds won't work as it will underflow when dividing by 1000 to do the conversion - initialTime = ( tp.tv_sec ) * (MafiaNet::TimeUS) 1000000 + ( tp.tv_usec ); - } + timeval t; + gettimeofday( &t, 0 ); + return ( t.tv_sec ) * (MafiaNet::TimeUS) 1000000 + ( t.tv_usec ); + }(); // GCC MafiaNet::TimeUS curTime; diff --git a/Source/src/lockless_types.cpp b/Source/src/lockless_types.cpp index 0f109ff12..663513949 100644 --- a/Source/src/lockless_types.cpp +++ b/Source/src/lockless_types.cpp @@ -17,41 +17,20 @@ using namespace MafiaNet; -LocklessUint32_t::LocklessUint32_t() +LocklessUint32_t::LocklessUint32_t() : value(0) { - value=0; } -LocklessUint32_t::LocklessUint32_t(uint32_t initial) +LocklessUint32_t::LocklessUint32_t(uint32_t initial) : value(initial) { - value=initial; } +// GetValue() used to read the counter without the lock or barrier the writers +// used, and the Linux path returned the value *before* the change, contrary to +// the header. std::atomic fixes both on every platform. uint32_t LocklessUint32_t::Increment(void) { -#ifdef _WIN32 - return (uint32_t) InterlockedIncrement(&value); -#elif defined(ANDROID) || defined(__S3E__) || defined(__APPLE__) - uint32_t v; - mutex.Lock(); - ++value; - v=value; - mutex.Unlock(); - return v; -#else - return __sync_fetch_and_add (&value, (uint32_t) 1); -#endif + return value.fetch_add(1) + 1; } uint32_t LocklessUint32_t::Decrement(void) { -#ifdef _WIN32 - return (uint32_t) InterlockedDecrement(&value); -#elif defined(ANDROID) || defined(__S3E__) || defined(__APPLE__) - uint32_t v; - mutex.Lock(); - --value; - v=value; - mutex.Unlock(); - return v; -#else - return __sync_fetch_and_add (&value, (uint32_t) -1); -#endif + return value.fetch_sub(1) - 1; } diff --git a/Tests/Unit/LocklessTypesTests.cpp b/Tests/Unit/LocklessTypesTests.cpp new file mode 100644 index 000000000..e90cc1b9a --- /dev/null +++ b/Tests/Unit/LocklessTypesTests.cpp @@ -0,0 +1,69 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + */ + +#include + +#include "mafianet/lockless_types.h" + +#include +#include + +using namespace MafiaNet; + +TEST(LocklessUint32, DefaultsToZero) +{ + const LocklessUint32_t counter; + EXPECT_EQ(counter.GetValue(), 0u); +} + +TEST(LocklessUint32, IncrementReturnsTheValueAfterTheChange) +{ + LocklessUint32_t counter(5); + EXPECT_EQ(counter.Increment(), 6u); + EXPECT_EQ(counter.GetValue(), 6u); +} + +TEST(LocklessUint32, DecrementReturnsTheValueAfterTheChange) +{ + LocklessUint32_t counter(5); + EXPECT_EQ(counter.Decrement(), 4u); + EXPECT_EQ(counter.GetValue(), 4u); +} + +TEST(LocklessUint32, DecrementWrapsBelowZero) +{ + LocklessUint32_t counter; + EXPECT_EQ(counter.Decrement(), UINT32_MAX); +} + +// No update is lost when threads contend; under TSan this also proves the +// counter is race-free. +TEST(LocklessUint32, ConcurrentIncrementsAndDecrementsBalance) +{ + constexpr int kThreads = 8; + constexpr int kIterations = 10000; + LocklessUint32_t counter(1000); + + std::vector threads; + for (int t = 0; t < kThreads; ++t) + { + threads.emplace_back([&counter, t] + { + for (int i = 0; i < kIterations; ++i) + { + if (t % 2 == 0) + counter.Increment(); + else + counter.Decrement(); + } + }); + } + for (std::thread &thread : threads) + thread.join(); + + EXPECT_EQ(counter.GetValue(), 1000u); +} From b5c4dbf12c729f90a626ec12e67d5715737e6140 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:33:05 +0200 Subject: [PATCH 05/13] chore: add editorconfig, clangd/clang-tidy config, devcontainer and Dependabot - .editorconfig mirrors the tab/Allman source style. It sets no end_of_line: the tree mixes CRLF and LF files. - .clangd points at the debug preset's compile_commands.json; .clang-tidy is a focused correctness/modernize set, diagnostics only. - .devcontainer gives a Linux environment (ptrace and TSan-friendly seccomp) for the Linux-only batched I/O paths and the sanitizer presets. - Dependabot bumps GitHub Actions, the docs requirements and the Dockerfile base image monthly. --- .clang-tidy | 34 +++++++++++++++++++++++++++++++++ .clangd | 5 +++++ .devcontainer/devcontainer.json | 25 ++++++++++++++++++++++++ .editorconfig | 26 +++++++++++++++++++++++++ .github/dependabot.yml | 25 ++++++++++++++++++++++++ 5 files changed, 115 insertions(+) create mode 100644 .clang-tidy create mode 100644 .clangd create mode 100644 .devcontainer/devcontainer.json create mode 100644 .editorconfig create mode 100644 .github/dependabot.yml diff --git a/.clang-tidy b/.clang-tidy new file mode 100644 index 000000000..a298c4cae --- /dev/null +++ b/.clang-tidy @@ -0,0 +1,34 @@ +# clang-tidy checks for MafiaNet, read by clangd (inline diagnostics) and by +# `run-clang-tidy -p build/`. Focused on correctness plus the +# mechanical modernizations clang-tidy can fix on its own. Not a CI gate: on +# the legacy tree it would drown the signal. +--- +Checks: > + -*, + bugprone-*, + performance-*, + modernize-use-override, + modernize-use-nullptr, + modernize-use-using, + modernize-use-equals-default, + modernize-use-equals-delete, + modernize-use-bool-literals, + modernize-loop-convert, + modernize-use-emplace, + cppcoreguidelines-pro-type-member-init, + cppcoreguidelines-virtual-class-destructor, + readability-redundant-control-flow, + readability-redundant-string-init, + -bugprone-easily-swappable-parameters, + -bugprone-narrowing-conversions, + -bugprone-reserved-identifier, + -performance-no-int-to-ptr, + -performance-enum-size + +# Diagnostics only. +WarningsAsErrors: '' + +# Only diagnose project headers, not fetched third-party dependencies. +HeaderFilterRegex: 'Source/(include|src)/.*' + +FormatStyle: none diff --git a/.clangd b/.clangd new file mode 100644 index 000000000..58c375e09 --- /dev/null +++ b/.clangd @@ -0,0 +1,5 @@ +# clangd reads compile_commands.json from the `debug` preset's build tree +# (cmake --preset debug). Point it elsewhere locally with a user config if you +# work from another preset. +CompileFlags: + CompilationDatabase: build/debug diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json new file mode 100644 index 000000000..283494850 --- /dev/null +++ b/.devcontainer/devcontainer.json @@ -0,0 +1,25 @@ +// Linux development container. Mainly for working on the Linux-only batched +// datagram paths (recvmmsg/sendmmsg) and running the sanitizer presets from a +// macOS or Windows host: open the folder in VS Code / Codespaces, then +// `cmake --workflow --preset debug` (or asan / tsan). +{ + "name": "MafiaNet", + "image": "mcr.microsoft.com/devcontainers/cpp:1-ubuntu-24.04", + // ptrace for gdb; the default seccomp profile also blocks the personality() + // call TSan uses to disable ASLR. + "runArgs": ["--cap-add=SYS_PTRACE", "--security-opt", "seccomp=unconfined"], + "postCreateCommand": "sudo apt-get update && sudo apt-get install -y --no-install-recommends libssl-dev clang clangd clang-tidy ccache", + "customizations": { + "vscode": { + "extensions": [ + "llvm-vs-code-extensions.vscode-clangd", + "ms-vscode.cmake-tools", + "vadimcn.vscode-lldb" + ], + "settings": { + "cmake.useCMakePresets": "always", + "C_Cpp.intelliSenseEngine": "disabled" + } + } + } +} diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 000000000..8c777e351 --- /dev/null +++ b/.editorconfig @@ -0,0 +1,26 @@ +# EditorConfig -- https://editorconfig.org +# Mirrors .clang-format. No end_of_line: the tree mixes CRLF and LF files, and +# forcing either would rewrite whole files on save. +root = true + +[*] +charset = utf-8 +insert_final_newline = true +trim_trailing_whitespace = true + +[*.{c,cpp,cc,cxx,h,hpp,hxx,inl}] +indent_style = tab +indent_size = 4 +tab_width = 4 + +[{CMakeLists.txt,*.cmake,CMakePresets.json}] +indent_style = space +indent_size = 4 + +[*.{yml,yaml,json}] +indent_style = space +indent_size = 2 + +[*.{md,rst}] +indent_style = space +trim_trailing_whitespace = false diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 000000000..5e5a87010 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,25 @@ +# GitHub Actions and the docs toolchain. The CMake FetchContent pins +# (cmake/Fetch*.cmake, Tests/CMakeLists.txt) are not covered by Dependabot and +# are bumped by hand. +version: 2 +updates: + - package-ecosystem: github-actions + directory: / + schedule: + interval: monthly + groups: + actions: + patterns: ["*"] + + - package-ecosystem: pip + directory: /docs + schedule: + interval: monthly + groups: + docs: + patterns: ["*"] + + - package-ecosystem: docker + directory: / + schedule: + interval: monthly From 17fe6f4c740d757a33e811d7ed71a09a1159efe2 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:34:18 +0200 Subject: [PATCH 06/13] test: reach RakPeer internals through pointers-to-member, not downcasts SendGateBypass and SlotInspector downcast the RakPeer returned by GetInstance() to a test-only subclass it is not an instance of. That is undefined behaviour, and UBSan's vptr check aborts four SessionConfigLive tests on it. Naming the protected member through the derived class and applying the pointer-to-member to the real RakPeer gives the same access legally. --- Tests/Integration/SessionConfigLiveTests.cpp | 24 ++++++++++++-------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/Tests/Integration/SessionConfigLiveTests.cpp b/Tests/Integration/SessionConfigLiveTests.cpp index 8af8da8c8..4de0c7d64 100644 --- a/Tests/Integration/SessionConfigLiveTests.cpp +++ b/Tests/Integration/SessionConfigLiveTests.cpp @@ -242,6 +242,10 @@ namespace // in this translation unit would size it with THIS unit's view of the class, and a mismatch there is // silent memory corruption rather than a compile error. // + // The derived type only names the member; the call goes through a pointer-to-member on the real + // RakPeer. Downcasting the peer to this type instead would be undefined behaviour (it is not one), + // and UBSan's vptr check aborts on it. + // // SendBuffered rather than SendImmediate: it is the layer the public Send() drops into once its own // checks pass, so injected traffic still crosses the network thread exactly like real traffic. class SendGateBypass : public RakPeer @@ -249,7 +253,7 @@ namespace public: static void Inject(RakPeerInterface *peer, const MafiaNet::BitStream &bs, const AddressOrGUID &target) { - static_cast(static_cast(peer))->SendBuffered( + (static_cast(peer)->*&SendGateBypass::SendBuffered)( (const char *)bs.GetData(), bs.GetNumberOfBitsUsed(), MafiaNet::Priority::Immediate, MafiaNet::Reliability::ReliableOrdered, 0, target, false, RemoteSystemStruct::NO_ACTION, 0); @@ -265,8 +269,8 @@ namespace // invariant only holds still on the slot itself. // // Never instantiated, and the peer must still come from RakPeerInterface::GetInstance() -- see - // SendGateBypass above for why constructing a RakPeer-derived object here would be silent memory - // corruption rather than a compile error. + // SendGateBypass above for why, and for why members are read through pointers-to-member rather than + // by downcasting. class SlotInspector : public RakPeer { public: @@ -280,20 +284,22 @@ namespace // The tests using this have exactly one remote, so the first active slot is it. static Snapshot FirstActiveSlot(RakPeerInterface *peer) { - SlotInspector *self = static_cast(static_cast(peer)); + const RakPeer *rakPeer = static_cast(peer); + const RemoteSystemStruct *slots = rakPeer->*&SlotInspector::remoteSystemList; + const unsigned int slotCount = rakPeer->*&SlotInspector::maximumNumberOfPeers; Snapshot out; out.sawSlot = false; out.reported = false; out.payloadLength = 0; - if (self->remoteSystemList == 0) + if (slots == 0) return out; - for (unsigned int i = 0; i < self->maximumNumberOfPeers; ++i) + for (unsigned int i = 0; i < slotCount; ++i) { - if (self->remoteSystemList[i].isActive == false) + if (slots[i].isActive == false) continue; out.sawSlot = true; - out.reported = self->remoteSystemList[i].connectionReportedToApplication; - out.payloadLength = self->remoteSystemList[i].remoteSessionConfigLength; + out.reported = slots[i].connectionReportedToApplication; + out.payloadLength = slots[i].remoteSessionConfigLength; return out; } return out; From fcb1765858222e4eb0c5a8a914d29f4330769fe5 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:41:53 +0200 Subject: [PATCH 07/13] fix(core): release runThreadsMutex in ThreadPool::Clear when not running Clear() on a pool whose threads were never started (or already stopped) returned with runThreadsMutex still locked. The destructor calls Clear(), so every FileListTransfer destroyed a locked mutex, and any later call taking the lock from another thread (WasStarted, StartThreads) blocked forever. Found by ThreadSanitizer. --- Source/include/mafianet/thread_pool.h | 1 + Tests/Unit/ThreadPoolTests.cpp | 37 +++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 Tests/Unit/ThreadPoolTests.cpp diff --git a/Source/include/mafianet/thread_pool.h b/Source/include/mafianet/thread_pool.h index 448b82bba..648d1f386 100644 --- a/Source/include/mafianet/thread_pool.h +++ b/Source/include/mafianet/thread_pool.h @@ -495,6 +495,7 @@ void ThreadPool::Clear(void) inputFunctionQueue.Clear(_FILE_AND_LINE_); inputQueue.Clear(_FILE_AND_LINE_); outputQueue.Clear(_FILE_AND_LINE_); + runThreadsMutex.Unlock(); } } template diff --git a/Tests/Unit/ThreadPoolTests.cpp b/Tests/Unit/ThreadPoolTests.cpp new file mode 100644 index 000000000..78ffeaecb --- /dev/null +++ b/Tests/Unit/ThreadPoolTests.cpp @@ -0,0 +1,37 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + */ + +#include + +#include "mafianet/thread_pool.h" + +#include +#include +#include +#include + +// Clear() on a pool whose threads were never started used to return with +// runThreadsMutex still held. The destructor calls Clear(), so every +// FileListTransfer destroyed a locked mutex, and any later call that takes the +// lock from another thread blocked forever. +// +// The deadline only bounds the failure: with the bug, WasStarted() never +// returns. The blocked thread is detached so a failure reports instead of +// hanging the test binary. +TEST(ThreadPool, ClearOnAnUnstartedPoolReleasesItsLock) +{ + auto pool = std::make_shared>(); + pool->Clear(); + + auto done = std::make_shared>(); + std::future wasStarted = done->get_future(); + std::thread([pool, done] { done->set_value(pool->WasStarted()); }).detach(); + + ASSERT_EQ(wasStarted.wait_for(std::chrono::seconds(10)), std::future_status::ready) + << "WasStarted() blocked: Clear() left runThreadsMutex locked"; + EXPECT_FALSE(wasStarted.get()); +} From 867f7cab2573d34096eed9c969e800135f9a9c14 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:47:41 +0200 Subject: [PATCH 08/13] ci: add sanitizer jobs and sccache - sanitizers (asan): ASan + UBSan over both suites, blocking. - sanitizers (tsan): TSan over both suites, advisory. RakNet's public getters read network-thread state without locking by design; those races are baselined in ci/tsan.supp so the job reports new ones. - sccache backs the Linux, minimal, macOS and sanitizer builds with the GitHub Actions cache. Windows (Visual Studio generator) has no launcher support and is unchanged. --- .github/workflows/build.yml | 90 +++++++++++++++++++++++++++++++++++++ ci/tsan.supp | 50 +++++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 ci/tsan.supp diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 89f715612..306a25e17 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -16,6 +16,12 @@ concurrency: permissions: contents: read +# sccache keeps compiled objects in the GitHub Actions cache, so a PR rebuilds +# only the translation units it touched. Jobs opt in by running +# mozilla-actions/sccache-action and passing it as the compiler launcher. +env: + SCCACHE_GHA_ENABLED: "true" + jobs: # Linux: Build and test in Docker container (full network access). # Tests run through CTest inside the container: one process per test, @@ -86,10 +92,15 @@ jobs: [ -e "Source/include/mafianet/$n" ] && echo "$f:$l: $rest"; done || true) if [ -n "$bad" ]; then echo "Bare includes of library headers (use \"mafianet/.h\"):"; echo "$bad"; exit 1; fi + - name: Set up sccache + uses: mozilla-actions/sccache-action@v0.0.11 + - name: Configure CMake run: | cmake -B build \ -DCMAKE_BUILD_TYPE=${{ matrix.build_type }} \ + -DCMAKE_C_COMPILER_LAUNCHER=sccache \ + -DCMAKE_CXX_COMPILER_LAUNCHER=sccache \ -DMAFIANET_BUILD_TESTS=ON \ -DMAFIANET_BUILD_EXTRAS=ON @@ -139,10 +150,15 @@ jobs: - name: Install OpenSSL run: sudo apt-get update && sudo apt-get install -y libssl-dev + - name: Set up sccache + uses: mozilla-actions/sccache-action@v0.0.11 + - name: Configure CMake (minimal) run: | cmake -B build \ -DCMAKE_BUILD_TYPE=Release \ + -DCMAKE_C_COMPILER_LAUNCHER=sccache \ + -DCMAKE_CXX_COMPILER_LAUNCHER=sccache \ -DMAFIANET_MINIMAL=ON - name: Build @@ -151,6 +167,75 @@ jobs: - name: Report library size run: ls -l build/Source/libMafiaNetStatic.a build/Source/libmafianet.so* + # Sanitizers, via the asan / tsan presets (RelWithDebInfo + tests; see + # CMakePresets.json for why not Debug). Every target is instrumented, + # third-party dependencies included, so TSan sees both sides of each access. + # + # TSan is advisory: RakNet's public getters read connection state owned by the + # network thread without locking, by design. Those races are baselined in + # ci/tsan.supp so the job flags new ones; removing them needs a + # concurrency-model change, not a CI tweak. + sanitizers: + runs-on: ubuntu-latest + timeout-minutes: 60 + continue-on-error: ${{ matrix.preset == 'tsan' }} + strategy: + fail-fast: false + matrix: + preset: [asan, tsan] + name: sanitizers (${{ matrix.preset }}) + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + persist-credentials: false + + - name: Install dependencies + run: sudo apt-get update && sudo apt-get install -y libssl-dev ninja-build clang + + # Ubuntu 24.04 runners use 32 bits of mmap randomisation, which older TSan + # runtimes cannot map their shadow memory around. + - name: Reduce ASLR entropy for TSan + if: matrix.preset == 'tsan' + run: sudo sysctl vm.mmap_rnd_bits=28 + + - name: Set up sccache + uses: mozilla-actions/sccache-action@v0.0.11 + + - name: Configure CMake + run: | + cmake --preset ${{ matrix.preset }} -G Ninja \ + -DCMAKE_C_COMPILER=clang \ + -DCMAKE_CXX_COMPILER=clang++ \ + -DCMAKE_C_COMPILER_LAUNCHER=sccache \ + -DCMAKE_CXX_COMPILER_LAUNCHER=sccache + + - name: Build + run: cmake --build --preset ${{ matrix.preset }} + + - name: Run unit tests + run: | + ctest --preset ${{ matrix.preset }} -L unit \ + --parallel 4 \ + --output-junit junit-unit.xml + + # Retries absorb loopback timing misses, which sanitizer slowdown makes + # more frequent. A report that only fires on some attempts can still pass + # here, so check the log of any test that needed a retry. + - name: Run integration tests + run: | + ctest --preset ${{ matrix.preset }} -L integration \ + --repeat until-pass:3 \ + --output-junit junit-integration.xml + + - name: Upload test results + if: always() + uses: actions/upload-artifact@v4 + with: + name: test-results-sanitizers-${{ matrix.preset }} + path: build/${{ matrix.preset }}/junit-*.xml + if-no-files-found: ignore + # macOS: Build and run both suites (unit + integration). macos: runs-on: macos-latest @@ -163,10 +248,15 @@ jobs: - name: Install OpenSSL run: brew install openssl@3 + - name: Set up sccache + uses: mozilla-actions/sccache-action@v0.0.11 + - name: Configure CMake run: | cmake -B build \ -DOPENSSL_ROOT_DIR=$(brew --prefix openssl@3) \ + -DCMAKE_C_COMPILER_LAUNCHER=sccache \ + -DCMAKE_CXX_COMPILER_LAUNCHER=sccache \ -DMAFIANET_BUILD_SAMPLES=ON \ -DMAFIANET_BUILD_TESTS=ON diff --git a/ci/tsan.supp b/ci/tsan.supp new file mode 100644 index 000000000..48660695d --- /dev/null +++ b/ci/tsan.supp @@ -0,0 +1,50 @@ +# ThreadSanitizer suppressions for MafiaNet. +# +# These baseline KNOWN, by-design data races so the TSAN CI job can act as a +# regression guard that surfaces NEW races. They are NOT a statement that these +# accesses are correct — they document a deliberate concurrency-model trade-off +# inherited from RakNet: the network update thread owns remoteSystemList[] and +# each connection's ReliabilityLayer, while public, user-thread getters read that +# state WITHOUT locking, accepting eventual consistency in exchange for avoiding +# locks on the hot path. +# +# Properly resolving these requires a deliberate concurrency-model redesign +# (fine-grained synchronization or a snapshot/double-buffer for reads), tracked +# separately. Genuine race bugs (thread-lifecycle synchronization, lazy-init, +# the lockless counter) have been FIXED rather than suppressed. +# +# Format: https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions +# A `race:` entry suppresses any race report whose stacks contain the substring. + +# --- Lock-free reads of connection state through public API getters --- +race:MafiaNet::RakPeer::GetSystemList +race:MafiaNet::RakPeer::GetConnectionState +race:MafiaNet::RakPeer::GetIndexFromSystemAddress +race:MafiaNet::RakPeer::GetSystemAddressFromIndex +race:MafiaNet::RakPeer::GetAveragePing +race:MafiaNet::RakPeer::GetLastPing +race:MafiaNet::ReliabilityLayer::GetStatistics +# Send()'s session-handshake gate reads connectMode the same way +race:MafiaNet::RakPeer::IsExchangingSessionData + +# --- Config get/set on the user thread, read lock-free by the network thread --- +race:MafiaNet::RakPeer::SetMaximumIncomingConnections +race:MafiaNet::RakPeer::AddToBanList +race:MafiaNet::ReliabilityLayer::SetSplitMessageProgressInterval + +# --- Copying connection identifiers read lock-free from shared state --- +race:MafiaNet::SystemAddress::operator= + +# --- remoteSystemList membership churn observed concurrently with the readers +# above; part of the same lock-free-read trade-off --- +race:MafiaNet::RakPeer::CloseConnectionInternal2 +race:MafiaNet::RakPeer::RemoveFromActiveSystemList +race:MafiaNet::RakPeer::AssignSystemAddressToRemoteSystemList +race:MafiaNet::RakPeer::ParseConnectionRequestPacket +race:MafiaNet::RakPeer::SetOccasionalPing + +# --- Thread-handoff command queues: producers on the user thread, consumer on +# the network thread. The queue access pattern predates this work; baselined +# pending a review of the intended locking discipline --- +race:DataStructures::Queue::Push +race:DataStructures::Queue::Push From 996e010c02fb18763421b7ba7b7c2dba46f88f74 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:38:20 +0200 Subject: [PATCH 09/13] fix: MSVC lambda capture in LocklessUint32 test; baseline SlotInspector TSan read MSVC requires a local constexpr used in a lambda to be captured; make the constants static. SessionConfigLive's SlotInspector reads the server's slot state without a lock on purpose, which TSan reports on CI runners. --- Tests/Unit/LocklessTypesTests.cpp | 4 ++-- ci/tsan.supp | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/Tests/Unit/LocklessTypesTests.cpp b/Tests/Unit/LocklessTypesTests.cpp index e90cc1b9a..c33afc0b9 100644 --- a/Tests/Unit/LocklessTypesTests.cpp +++ b/Tests/Unit/LocklessTypesTests.cpp @@ -44,8 +44,8 @@ TEST(LocklessUint32, DecrementWrapsBelowZero) // counter is race-free. TEST(LocklessUint32, ConcurrentIncrementsAndDecrementsBalance) { - constexpr int kThreads = 8; - constexpr int kIterations = 10000; + static constexpr int kThreads = 8; + static constexpr int kIterations = 10000; LocklessUint32_t counter(1000); std::vector threads; diff --git a/ci/tsan.supp b/ci/tsan.supp index 48660695d..b5a1e9a37 100644 --- a/ci/tsan.supp +++ b/ci/tsan.supp @@ -48,3 +48,8 @@ race:MafiaNet::RakPeer::SetOccasionalPing # pending a review of the intended locking discipline --- race:DataStructures::Queue::Push race:DataStructures::Queue::Push + +# --- Test-only: SessionConfigLive's SlotInspector samples the server's slot +# state from the test thread without a lock, deliberately, to observe the +# static-mode gating window (see the comment on it in the test) --- +race:SlotInspector::FirstActiveSlot From a42944bd73e7c270aa7bfcebe420ebb0b5a03dd8 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:49:27 +0200 Subject: [PATCH 10/13] test: give each FileListDirectory test its own temp directory Both tests used one fixed path and removed it on setup and teardown, so running them in parallel (the sanitizer jobs run the unit suite with --parallel) let one delete the other's tree mid-walk. --- Tests/Unit/FileListDirectoryTests.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Tests/Unit/FileListDirectoryTests.cpp b/Tests/Unit/FileListDirectoryTests.cpp index 9b450ff7f..b08477662 100644 --- a/Tests/Unit/FileListDirectoryTests.cpp +++ b/Tests/Unit/FileListDirectoryTests.cpp @@ -32,7 +32,11 @@ namespace std::filesystem::path root; TempTree() { - root = std::filesystem::temp_directory_path() / "mafianet_filelist_unit"; + // One directory per test: ctest may run these in parallel, and a + // shared path lets one test delete another's tree mid-walk. + root = std::filesystem::temp_directory_path() / + (std::string("mafianet_filelist_unit_") + + ::testing::UnitTest::GetInstance()->current_test_info()->name()); std::filesystem::remove_all(root); std::filesystem::create_directories(root / "sub" / "deeper"); Write("a.bin", 10); From f567b16c1ff8f292c8e608c189e04912b818cfa2 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 22:29:38 +0200 Subject: [PATCH 11/13] test: check ThreadPool's lock with TryLock instead of a deadline Adds SimpleMutex::TryLock (pthread_mutex_trylock / TryEnterCriticalSection) so the ThreadPool::Clear regression test checks the lock state directly from a second thread instead of waiting up to ten seconds for a blocked call. A pool that kept its lock is leaked rather than destroyed, since its destructor would deadlock. --- Source/include/mafianet/simple_mutex.h | 5 ++ Source/src/simple_mutex.cpp | 9 ++++ Tests/Unit/ThreadPoolTests.cpp | 65 ++++++++++++++++++++------ 3 files changed, 64 insertions(+), 15 deletions(-) diff --git a/Source/include/mafianet/simple_mutex.h b/Source/include/mafianet/simple_mutex.h index 2f77f8337..f26daa5a7 100644 --- a/Source/include/mafianet/simple_mutex.h +++ b/Source/include/mafianet/simple_mutex.h @@ -58,6 +58,11 @@ class RAK_DLL_EXPORT SimpleMutex // Unlocks the mutex. void Unlock(void); + // Locks the mutex if no other thread holds it, without blocking. Returns + // true if the lock was taken; the caller must then Unlock(). On Windows the + // lock is re-entrant, so this succeeds for the thread already holding it. + bool TryLock(void); + diff --git a/Source/src/simple_mutex.cpp b/Source/src/simple_mutex.cpp index f14755fa1..333134834 100644 --- a/Source/src/simple_mutex.cpp +++ b/Source/src/simple_mutex.cpp @@ -165,6 +165,15 @@ void SimpleMutex::Unlock(void) #endif } +bool SimpleMutex::TryLock(void) +{ +#ifdef _WIN32 + return TryEnterCriticalSection(&criticalSection) != 0; +#else + return pthread_mutex_trylock(&hMutex) == 0; +#endif +} + void SimpleMutex::Init(void) { #if defined(_WIN32) diff --git a/Tests/Unit/ThreadPoolTests.cpp b/Tests/Unit/ThreadPoolTests.cpp index 78ffeaecb..6cc17fc21 100644 --- a/Tests/Unit/ThreadPoolTests.cpp +++ b/Tests/Unit/ThreadPoolTests.cpp @@ -7,31 +7,66 @@ #include +#include "mafianet/simple_mutex.h" #include "mafianet/thread_pool.h" -#include -#include -#include #include +using namespace MafiaNet; + +namespace +{ + // TryLock from a second thread: it never blocks, and it is not fooled by + // the re-entrant Windows lock the way a same-thread attempt would be. + bool FreeFromAnotherThread(SimpleMutex &mutex) + { + bool acquired = false; + std::thread([&mutex, &acquired] + { + acquired = mutex.TryLock(); + if (acquired) + mutex.Unlock(); + }).join(); + return acquired; + } + + // ThreadPool is a header-only template, so constructing a derived pool in + // this translation unit is safe; the subclass only exposes the lock. + class InspectablePool : public ThreadPool + { + public: + SimpleMutex &RunThreadsMutex() { return runThreadsMutex; } + }; +} + +TEST(SimpleMutex, TryLockTakesAFreeMutex) +{ + SimpleMutex mutex; + EXPECT_TRUE(FreeFromAnotherThread(mutex)); +} + +TEST(SimpleMutex, TryLockFailsWhileAnotherThreadHoldsIt) +{ + SimpleMutex mutex; + mutex.Lock(); + EXPECT_FALSE(FreeFromAnotherThread(mutex)); + mutex.Unlock(); + EXPECT_TRUE(FreeFromAnotherThread(mutex)); +} + // Clear() on a pool whose threads were never started used to return with // runThreadsMutex still held. The destructor calls Clear(), so every // FileListTransfer destroyed a locked mutex, and any later call that takes the // lock from another thread blocked forever. // -// The deadline only bounds the failure: with the bug, WasStarted() never -// returns. The blocked thread is detached so a failure reports instead of -// hanging the test binary. +// A pool that kept its lock is leaked on purpose: its destructor calls Clear() +// again, which would deadlock re-locking the mutex and hang instead of failing. TEST(ThreadPool, ClearOnAnUnstartedPoolReleasesItsLock) { - auto pool = std::make_shared>(); + InspectablePool *pool = new InspectablePool; pool->Clear(); - - auto done = std::make_shared>(); - std::future wasStarted = done->get_future(); - std::thread([pool, done] { done->set_value(pool->WasStarted()); }).detach(); - - ASSERT_EQ(wasStarted.wait_for(std::chrono::seconds(10)), std::future_status::ready) - << "WasStarted() blocked: Clear() left runThreadsMutex locked"; - EXPECT_FALSE(wasStarted.get()); + const bool released = FreeFromAnotherThread(pool->RunThreadsMutex()); + EXPECT_TRUE(released) << "Clear() left runThreadsMutex locked"; + if (released) + delete pool; } From b43abf21fea38aa47b1c80248733a4030d4a6985 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 22:29:38 +0200 Subject: [PATCH 12/13] ci: fail sanitizer jobs on a report from any retried attempt; bound unit preset jobs Integration retries absorb loopback timing misses, but a sanitizer report in a failed attempt could be hidden by a passing retry. ctest now writes every attempt to a log, and a follow-up step fails on any sanitizer report in it. Drops "jobs": 0 from the unit test preset: CTest 3.29+ reads it as unbounded parallelism, older versions as a single job. --- .github/workflows/build.yml | 17 ++++++++++++++--- CMakePresets.json | 3 +-- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 306a25e17..b8fd94f4c 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -220,20 +220,31 @@ jobs: --output-junit junit-unit.xml # Retries absorb loopback timing misses, which sanitizer slowdown makes - # more frequent. A report that only fires on some attempts can still pass - # here, so check the log of any test that needed a retry. + # more frequent. They must not absorb a sanitizer report, so the log of + # every attempt is kept and the next step fails on any report in it. - name: Run integration tests run: | ctest --preset ${{ matrix.preset }} -L integration \ --repeat until-pass:3 \ + --output-log build/${{ matrix.preset }}/integration.log \ --output-junit junit-integration.xml + - name: Fail on sanitizer reports from any attempt + run: | + if grep -nE 'ERROR: AddressSanitizer|WARNING: ThreadSanitizer|runtime error:' \ + build/${{ matrix.preset }}/integration.log; then + echo "::error::A sanitizer reported an error in an attempt that a retry later passed" + exit 1 + fi + - name: Upload test results if: always() uses: actions/upload-artifact@v4 with: name: test-results-sanitizers-${{ matrix.preset }} - path: build/${{ matrix.preset }}/junit-*.xml + path: | + build/${{ matrix.preset }}/junit-*.xml + build/${{ matrix.preset }}/integration.log if-no-files-found: ignore # macOS: Build and run both suites (unit + integration). diff --git a/CMakePresets.json b/CMakePresets.json index 583f15329..f088a4990 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -108,8 +108,7 @@ "inherits": "test-base", "displayName": "Unit tests (debug)", "configurePreset": "debug", - "filter": { "include": { "label": "unit" } }, - "execution": { "jobs": 0 } + "filter": { "include": { "label": "unit" } } }, { "name": "integration", From 99fc7a00f9ab07d73d3bd455788ac75f572fded7 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Fri, 16 Oct 2026 22:40:06 +0200 Subject: [PATCH 13/13] ci: fail the sanitizer report check when the ctest log is missing --- .github/workflows/build.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b8fd94f4c..79f27459a 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -231,6 +231,8 @@ jobs: - name: Fail on sanitizer reports from any attempt run: | + # A missing log would make grep exit 2, which the if reads as "clean". + test -f build/${{ matrix.preset }}/integration.log || { echo "::error::integration.log missing"; exit 1; } if grep -nE 'ERROR: AddressSanitizer|WARNING: ThreadSanitizer|runtime error:' \ build/${{ matrix.preset }}/integration.log; then echo "::error::A sanitizer reported an error in an attempt that a retry later passed"