ci: ThreadSanitizer lane + fix use-after-free TSan flagged
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 <noreply@anthropic.com>
This commit is contained in:
+42
-3
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user