From 9db483402ea2fd4e0a5ff3bdd2b1fc50a7518ec9 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sat, 19 Sep 2026 22:24:11 +0200 Subject: [PATCH] fix(mem,spill): close two data races the worker threads exposed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TSan failed on all three platforms in PR CI. Both races are in code this branch added, and both were invisible to the local ladder because it has no TSan leg — that lane is CI-only, so "macOS and Linux green" said nothing about either. 1. system_info.c: the cached system info cbm_system_info() published its cache with a plain flag: if (!info_cached) { cached_info = detect_...(); info_cached = 1; } Fine while only the main thread asked. The memory-relief work started calling cbm_mem_system_under_pressure() from the per-file extract loop, so several workers now race on the flag AND on the struct — and a struct assignment is not atomic, so a reader could see half of one. Reported at system_info.c:284 against :274, from extract_worker, on macOS, ubuntu-arm and ubuntu-latest alike. Now a three-state atomic: whichever thread wins the compare-exchange is the only writer of cached_info, a loser returns the copy it detected itself (the same answer — detection only reads the OS), and later readers see the published struct through the acquire/release pair. No mutex, so nothing has to be initialised before first use. 2. pass_parallel.c: the spill pointer pp_spill_active() tested ec->pctx->spill BEFORE its atomic spill_mode gate, so it read an 8-byte pointer with no synchronisation while pp_spill_enter was publishing that pointer under spill_mu. pp_spill_enter assigns the pointer and only then release-stores spill_mode, so gating on the atomic first makes the pointer read ordered by that release. Short-circuit order is load bearing in that expression now, and the comment says so. Found by running the TSan lane locally with halt_on_error=0 rather than CI's halt_on_error=1: the first setting stops at the first race, so each fix would have revealed the next one a CI round trip later. Enumerating instead showed the second race was the last one in that suite. Signed-off-by: Martin Vogel --- src/foundation/system_info.c | 51 ++++++++++++++++++++++++++++-------- src/pipeline/pass_parallel.c | 12 +++++++-- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/src/foundation/system_info.c b/src/foundation/system_info.c index ad182f150..84a91cdf1 100644 --- a/src/foundation/system_info.c +++ b/src/foundation/system_info.c @@ -17,8 +17,9 @@ enum { DEFAULT_CORES = 1, MIN_WORKERS = 1, CBM_WORKERS_MAX = 256 }; #include "foundation/mem_core.h" /* cbm_alloc: one accounted allocation path */ #include "foundation/platform.h" #include "foundation/system_info_internal.h" -#include // uint64_t -#include // strtol +#include // the system-info cache is published across worker threads +#include // uint64_t +#include // strtol #include #ifndef _WIN32 @@ -267,23 +268,51 @@ static cbm_system_info_t detect_system_windows(void) { /* ── Public API ──────────────────────────────────────────────────── */ -static int info_cached = 0; +/* The cache is published once, by whichever thread gets there first. + * + * This used to be a plain `if (!info_cached) { cached_info = detect(); }`, which + * was fine while only the main thread asked. It stopped being fine when the + * memory-relief path started calling cbm_mem_system_under_pressure() from the + * extract workers: several threads then raced on the flag AND on the struct, + * and a struct assignment is not atomic, so a reader could see half of one. + * TSan caught it on all three platforms (system_info.c:284 against :274, from + * extract_worker). + * + * Three states rather than a flag, so the winner of the compare-exchange is the + * only writer of cached_info: a loser returns the copy it detected itself, + * which is the same answer, and a later reader sees the published struct + * through the acquire/release pair. No mutex, so nothing has to be initialised + * before first use, and no reader can observe a half-written struct. */ +enum { INFO_EMPTY = 0, INFO_CLAIMED = 1, INFO_READY = 2 }; +static _Atomic int info_state = INFO_EMPTY; static cbm_system_info_t cached_info; -cbm_system_info_t cbm_system_info(void) { - if (!info_cached) { +static cbm_system_info_t detect_system_now(void) { #ifdef _WIN32 - cached_info = detect_system_windows(); + return detect_system_windows(); #elif defined(__APPLE__) - cached_info = detect_system_macos(); + return detect_system_macos(); #elif defined(__NetBSD__) || defined(__FreeBSD__) || defined(__OpenBSD__) - cached_info = detect_system_bsd(); + return detect_system_bsd(); #else - cached_info = detect_system_linux(); + return detect_system_linux(); #endif - info_cached = SKIP_ONE; +} + +cbm_system_info_t cbm_system_info(void) { + if (atomic_load_explicit(&info_state, memory_order_acquire) == INFO_READY) { + return cached_info; } - return cached_info; + /* Detection only reads the OS (sysconf/sysctl/GetSystemInfo), so racing + * threads doing it twice during startup costs a little and changes nothing. */ + cbm_system_info_t local = detect_system_now(); + int expected = INFO_EMPTY; + if (atomic_compare_exchange_strong_explicit(&info_state, &expected, INFO_CLAIMED, + memory_order_acq_rel, memory_order_relaxed)) { + cached_info = local; + atomic_store_explicit(&info_state, INFO_READY, memory_order_release); + } + return local; } int cbm_default_worker_count(bool initial) { diff --git a/src/pipeline/pass_parallel.c b/src/pipeline/pass_parallel.c index aba55fb20..dbf459814 100644 --- a/src/pipeline/pass_parallel.c +++ b/src/pipeline/pass_parallel.c @@ -832,8 +832,16 @@ static void pp_spill_enter(extract_ctx_t *ec, const char *reason) { } static bool pp_spill_active(const extract_ctx_t *ec) { - return ec->pctx && ec->pctx->spill && - atomic_load_explicit(&ec->pctx->spill_mode, memory_order_acquire) != 0; + /* The atomic gates the pointer, not the other way round. pp_spill_enter + * assigns ec->pctx->spill under spill_mu and only THEN release-stores + * spill_mode, so a reader that has acquired a non-zero spill_mode is + * guaranteed to see the finished pointer. Testing the pointer first read it + * with no synchronisation at all while another worker was publishing it — + * a genuine data race on an 8-byte write, which TSan caught at + * pass_parallel.c:817 against this line. Short-circuit order is load + * bearing here; do not reorder these terms. */ + return ec->pctx && atomic_load_explicit(&ec->pctx->spill_mode, memory_order_acquire) != 0 && + ec->pctx->spill; } CBMFileResult *cbm_pipeline_result_acquire(const cbm_pipeline_ctx_t *ctx, CBMFileResult **cache,