From 943f3bbcd5e612ee1214c7f4a6f9d2ace1fa563a Mon Sep 17 00:00:00 2001 From: Raphael Maenle Date: Tue, 9 Jun 2026 15:32:02 +0200 Subject: [PATCH] ci: ThreadSanitizer lane + fix use-after-free TSan flagged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a -DSECSGEM_TSAN=ON CMake option that builds every target with -fsanitize=thread + debug symbols + -O1 + frame pointers. Wires a dedicated thread-sanitizer job into .gitea/workflows/ci.yml that builds and runs the full test suite under TSan with TSAN_OPTIONS=halt_on_error=1 (any flagged race fails the job, not just warns). Result against the full 426-case / 2557-assertion suite: 0 warnings, all green. That converts the existing test_thread_safety.cpp (which exercised the asio::post-onto-strand pattern) and test_concurrency (in-flight transaction interleaving) and test_robustness_fuzz (28 random action types × thousands of ticks) from "pattern smoke-tests" into actual race detection. The first TSan run caught a real bug in test_robustness_fuzz's act_exception_complete: it held a pointer to an ExceptionStore entry across fire_internal(RecoveryComplete), which deletes the entry. The subsequent state() read was a use-after-free. TSan flagged it 8 times (4 reads × 2 stack-frame variants). Fix is scoped lookup + re-check via has() after the mutation; matches the contract any reasonable caller would follow. The asio std_fenced_block atomic_thread_fence path generates TSan "not supported" warnings during compile — those are asio's, not ours, and don't affect runtime detection. Co-Authored-By: Claude Opus 4.7 --- .gitea/workflows/ci.yml | 45 +++++++++++++++++++++++++++++++--- CMakeLists.txt | 14 +++++++++++ tests/test_robustness_fuzz.cpp | 28 ++++++++++++--------- 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 6167ceb..58915ff 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -38,11 +38,50 @@ jobs: python3 \ python3-yaml - - name: Configure + - name: Configure (Release) run: cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Release - - name: Build + - name: Build (Release) run: cmake --build build - - name: Unit tests + - name: Unit tests (Release) run: build/secsgem_tests + + thread-sanitizer: + runs-on: ubuntu-latest + container: + image: ubuntu:24.04 + steps: + - name: Bootstrap (node + git for actions/checkout) + run: | + export DEBIAN_FRONTEND=noninteractive + apt-get update + apt-get install -y --no-install-recommends \ + git ca-certificates nodejs + + - uses: actions/checkout@v4 + + - name: Install C++ toolchain + run: | + export DEBIAN_FRONTEND=noninteractive + apt-get install -y --no-install-recommends \ + build-essential cmake ninja-build \ + libasio-dev libyaml-cpp-dev \ + python3 python3-yaml + + # Debug + -fsanitize=thread. Catches data races in the + # io_context strand contract documented in INTEGRATION.md §3. + # halt_on_error=1 makes TSan-flagged races fail the job, not + # just print a warning. + - name: Configure (TSan) + run: cmake -S . -B build-tsan -G Ninja + -DCMAKE_BUILD_TYPE=Debug + -DSECSGEM_TSAN=ON + + - name: Build (TSan) + run: cmake --build build-tsan + + - name: Unit tests (TSan) + env: + TSAN_OPTIONS: halt_on_error=1 + run: build-tsan/secsgem_tests diff --git a/CMakeLists.txt b/CMakeLists.txt index a4500d5..2a894f6 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -11,6 +11,20 @@ endif() add_compile_options(-Wall -Wextra -Wpedantic) +# --- ThreadSanitizer (opt-in) --------------------------------------------- +# Build with -DSECSGEM_TSAN=ON to instrument every target with TSan. +# Catches data races in EquipmentDataModel access from the io_context +# thread vs. application threads — the contract documented in +# INTEGRATION.md §3. CI runs the threading-relevant tests under this +# flag in a separate lane; the default (release, no sanitizer) is what +# customers build. +option(SECSGEM_TSAN "Build with ThreadSanitizer" OFF) +if(SECSGEM_TSAN) + message(STATUS "ThreadSanitizer enabled — debug symbols + -fsanitize=thread") + add_compile_options(-fsanitize=thread -g -O1 -fno-omit-frame-pointer) + add_link_options(-fsanitize=thread) +endif() + find_package(Threads REQUIRED) find_package(yaml-cpp REQUIRED) diff --git a/tests/test_robustness_fuzz.cpp b/tests/test_robustness_fuzz.cpp index d4236b1..6d786a3 100644 --- a/tests/test_robustness_fuzz.cpp +++ b/tests/test_robustness_fuzz.cpp @@ -483,21 +483,27 @@ bool act_exception_recover(World& w) { bool act_exception_complete(World& w) { if (w.exceptions.empty()) return false; const uint32_t exid = w.exceptions[w.rng() % w.exceptions.size()]; - const auto* ex = w.model.exceptions.get(exid); - if (!ex) return false; - if (ex->fsm->state() != ExceptionState::Recovering) return false; + { + // Scoped check: don't hold `ex` across fire_internal because a + // successful RecoveryComplete erases the entry from the store + // and invalidates the pointer (TSan flagged this as a real UAF). + const auto* ex = w.model.exceptions.get(exid); + if (!ex) return false; + if (ex->fsm->state() != ExceptionState::Recovering) return false; + } const auto event = w.roll(0.7) ? ExceptionEvent::RecoveryComplete : ExceptionEvent::RecoveryFailed; const bool ok = w.model.exceptions.fire_internal(exid, event); - if (ok) { - if (ex->fsm->state() == ExceptionState::Cleared || - !w.model.exceptions.has(exid)) { - auto it = std::find(w.exceptions.begin(), w.exceptions.end(), exid); - if (it != w.exceptions.end()) w.exceptions.erase(it); - } - w.log_action("exception_complete " + std::to_string(exid)); + if (!ok) return false; + + // After fire_internal returns, look the entry up again — if Cleared + // landed, the store has already removed it. + if (!w.model.exceptions.has(exid)) { + auto it = std::find(w.exceptions.begin(), w.exceptions.end(), exid); + if (it != w.exceptions.end()) w.exceptions.erase(it); } - return ok; + w.log_action("exception_complete " + std::to_string(exid)); + return true; } bool act_module_event(World& w) {