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 diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 89f715612..79f27459a 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,88 @@ 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. 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: | + # 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" + 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 + build/${{ matrix.preset }}/integration.log + if-no-files-found: ignore + # macOS: Build and run both suites (unit + integration). macos: runs-on: macos-latest @@ -163,10 +261,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/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..f088a4990 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,133 @@ "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" } } + }, + { + "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/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/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/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/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/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/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; 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/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; 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); diff --git a/Tests/Unit/LocklessTypesTests.cpp b/Tests/Unit/LocklessTypesTests.cpp new file mode 100644 index 000000000..c33afc0b9 --- /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) +{ + static constexpr int kThreads = 8; + static 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); +} diff --git a/Tests/Unit/ThreadPoolTests.cpp b/Tests/Unit/ThreadPoolTests.cpp new file mode 100644 index 000000000..6cc17fc21 --- /dev/null +++ b/Tests/Unit/ThreadPoolTests.cpp @@ -0,0 +1,72 @@ +/* + * 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/simple_mutex.h" +#include "mafianet/thread_pool.h" + +#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. +// +// 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) +{ + InspectablePool *pool = new InspectablePool; + pool->Clear(); + const bool released = FreeFromAnotherThread(pool->RunThreadsMutex()); + EXPECT_TRUE(released) << "Clear() left runThreadsMutex locked"; + if (released) + delete pool; +} diff --git a/ci/tsan.supp b/ci/tsan.supp new file mode 100644 index 000000000..b5a1e9a37 --- /dev/null +++ b/ci/tsan.supp @@ -0,0 +1,55 @@ +# 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 + +# --- 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 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 ) 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 -------------