Skip to content

Nothing verifies that a sanitized lane actually compiled with CBM_SANITIZED_BUILD (UBSan and trap-UBSan have no probe) #1648

Closed

Description

@moffermann

Summary

Nothing verifies that a sanitized lane actually compiled with CBM_SANITIZED_BUILD defined. #1615 gave
CBM_SANITIZED one spelling and a compiler-probe backstop, but the backstop only covers the three
sanitizers that announce themselves. For UBSan and trap-UBSan there is no probe and never can be, so the
build system is the sole source of truth — and a silent regression there is invisible: the suite still
passes, just against native timing budgets on an instrumented binary.

That is not hypothetical. It is exactly what happened twice:

Opening this per CONTRIBUTING.md ("Open an issue first — always"; build-system changes need prior
design discussion). Happy to implement once there is a decision on shape — nothing is written yet.

Why the existing mechanisms do not cover it

mechanism covers does not cover
SANITIZED_DEFINE (Makefile.cbm:93-96) any lane keyed off $(SANITIZE) a lane with its own sanitize variable — precisely the TSan gap
probes in sanitized.h (#1615) ASan, TSan, MSan UBSan, trap-UBSan — no macro, no __has_feature bit exists
the test suite passing nothing here a native budget on an instrumented binary is looser to violate, not tighter; it mostly still passes

The third row is the important one. The failure mode is a test that passes for months and then flakes on
a loaded runner, attributed to whatever PR happened to be in flight.

It is not only about budgets any more

scripts/msan.sh:120-129 reasons from this macro while ruling out a cause:

CBM_THREAD_STACK_MB at 256 MiB and at 1024 MiB (the cap). The knob is verified compiled in
(CBM_SANITIZED_BUILD is defined for this lane)
and the floor is applied in cbm_thread_create [...]
The fault address did not move by a single byte between 256 MiB and 1024 MiB — a 4x stack increase
changing nothing is what rules out "the stack is merely too small".

If the define were ever absent on that lane, cbm_thread_stack_floor would compile to return requested
(src/foundation/compat_thread.c:33-48), the 4x increase would have changed nothing because the knob was
inert
, and a documented "do not repeat" conclusion would be wrong. The premise is asserted in a comment;
nothing checks it.

Proposed shape

Three small pieces, and no changes under .github/workflows/ at all — which is the point of this shape.

1. The binary reports what it was compiled with. A --build-config early-exit in
tests/test_main.c, next to the existing --version (line ~796) and --list-suites (line ~861),
printing one line of key=value:

$ build/c/test-runner --build-config
sanitized=1 test_seams=1

sanitized comes straight from CBM_SANITIZED, so it answers the only question that matters: what did
the compiler actually receive
, not what the Makefile intended to send.

2. scripts/test.sh asserts it. This is the single choke point — every sanitized lane in
_test.yml already goes through it:

leg _test.yml invocation expected
test-unix :93 scripts/test.sh CC=… CXX=… (default ASan+UBSan) sanitized=1
test-diag :221 scripts/test.sh … SANITIZE="-fsanitize=address,undefined …" sanitized=1
test-lsan-macos :308 scripts/test.sh … (ASan+LSan) sanitized=1
test-tsan :340 scripts/test.sh --tsan … sanitized=1
Windows CLANGARM64 test.sh:173-177 trap-UBSan injected by the script itself sanitized=1
explicit plain build — scripts/test.sh SANITIZE= sanitized=0

test.sh already knows the answer: it tracks SANITIZE_GIVEN (:147), the --tsan mode (:98, :179-185)
and the ARM64 trap-UBSan default (:173-177). So it can compute the expectation from the flags it is about
to pass to make, run --build-config on the binary it just built, and fail loudly on mismatch, before the
suite runs:

ERROR: build config mismatch — this lane builds instrumented but the binary reports sanitized=0.
       Every sanitized-budget branch compiled to its NATIVE value.
       Check that this lane's flag set carries $(SANITIZED_DEFINE) or -DCBM_SANITIZED_BUILD=1.

Note the TSan mode must check $BUILD_DIR/test-runner-tsan, not test-runner.

3. scripts/msan.sh gets the same one-liner. It is the only lane that bypasses test.sh
(_test.yml:284 → Docker → msan.sh, running build/msan/test-runner). One check next to the build at
msan.sh:66-68 — and it would turn the comment quoted above into something enforced.

What this would have caught, and how fast

The TSan gap would have failed test-tsan in seconds, at the build step, with a message naming the cause
— instead of surfacing as an unrelated-looking flaky spawn test across three PRs.

Limits, stated up front

  • It proves the define reached the compiler, not that the sanitizer is instrumenting. That was never
    the failure mode, and -fsanitize= not working would fail far louder.
  • It cannot cover a future lane that routes around both scripts. Partial mitigation: the fix(build): give sanitized-build detection one spelling and a backstop #1615 probes
    still self-heal ASan/TSan/MSan, so a new lane would have to be UBSan-only to slip through.
  • It adds one flag to a test-only binary. No production surface.
  • test-windows (_test.yml:398) runs unsanitized, so it asserts sanitized=0 — which is also worth
    having: it catches the inverse mistake of leaking the define into a native lane.

Alternatives considered

  1. #error in sanitized.h when a probe fires without the define. Rejected in fix(build): give sanitized-build detection one spelling and a backstop #1615, for reasons
    that still hold: it breaks the binary instead of correcting it, and breaks an out-of-tree
    make CFLAGS_EXTRA=-fsanitize=address. It is also blind to UBSan, the case this issue is really about.
  2. Env var (CBM_EXPECT_SANITIZED=1) plus an in-suite test. A lane that forgets the variable skips
    the check silently — the same silence that caused the problem. Inverting the default (fail when unset)
    breaks every ad-hoc local run.
  3. Grep Makefile.cbm in CI for the define on each flag set. Verifies the intent, not the binary, and
    would have missed nothing only by accident — a flag set can also be assembled in a script.
  4. A make print-flags target compared in CI. Same weakness: it reports what make would pass, not
    what the compiler received, and it needs the CI workflow edits this shape avoids.

Scope

Three files: tests/test_main.c, scripts/test.sh, scripts/msan.sh. Roughly 40-60 lines including the
error text. One issue, one PR, per CONTRIBUTING.md.

Found while doing #1615 and #1618; not blocking either.

Activity

  1. DeusData commented on Aug 14, 2026

    @DeusData
    Owner

    This is the right generalisation, and it comes from a real miss on our side rather than a hypothetical.

    #1595 widened the spawn-retry budget for sanitized builds and it did nothing on the TSan leg, because CBM_SANITIZED_BUILD was never defined there — SANITIZED_DEFINE keys off $(SANITIZE), and TSan uses its own TSAN_SANITIZE. 169 thread-instrumented compile lines, not one carrying the define. The fix was a one-line Makefile change; the expensive part was that nothing could tell us the budget was inert, so the leg kept failing on a PR whose diff made causation impossible.

    Your #1615 then found a fourth spelling I had missed entirely (tests/test_cs_lsp_bench.c checking ASan only, so TSan and MSan measured instrumented parses against a native 200 ms budget). Two independent instances of the same class inside a week is the argument for asserting it rather than remembering it.

    A runtime or build-time assertion that a lane which is instrumented also knows it is instrumented would have caught both at the moment they were introduced instead of after a release cycle. The Makefile comment already warns about exactly this for trap-UBSan — the lesson was written down and then not enforced, which is precisely the gap a check closes.

    Fair warning on timing: our CI is badly backed up tonight behind a v0.10.5 that fixes a batch of install and startup blockers, so this will get proper attention after that lands. Thank you for filing it as an issue with the analysis rather than just patching around it — and for the three accurate reports on my code this week.

  2. astandrik commented on Aug 17, 2026

    @astandrik
    Contributor

    I’d like to take this one. I’ll keep the implementation to the agreed shape: a --build-config report from the test runner, fail-closed verification in scripts/test.sh, and the same verification in scripts/msan.sh, with no workflow or production-code changes. I’ll validate the default sanitized, explicit SANITIZE=, TSan, and MSan paths locally first and follow up here before opening a PR.

  3. added
    bugSomething isn't working
    maintainer-notesInternal observations and improvement notes
    priority/normalStandard review queue; useful PR with ordinary maintainer urgency.
    and removed
    windowsWindows-specific issues
    on Aug 18, 2026
  4. astandrik commented on Aug 18, 2026

    @astandrik
    Contributor

    The implementation is now in #1716.

    It changes only tests/test_main.c, scripts/test.sh, and scripts/msan.sh: 76 insertions and one deletion. The Makefile, workflows, dependencies, and production code are unchanged. The commit is signed and passes the DCO check.

    I checked the exact LF-terminated --build-config output and the early exit before test-home or cache setup. A plain runner reports sanitized=0 test_seams=1; the default sanitized and explicit UBSan runners report sanitized=1 test_seams=1. Empty and whitespace-only SANITIZE values take the plain path. A reused plain build also stops at the new check before a sanitized iteration can start a suite. The full scripts/test.sh run passed on Linux x86-64.

    Some platform checks still need CI. On Apple Silicon, ASan and TSan fail in the sanitizer runtime before main. The remote MSan path cannot reach its build command because the Compose service is pinned to linux/arm64 and the x86-64 builder has no binfmt/QEMU registration. Windows CLANG64/CLANGARM64 and the authoritative sanitizer lanes will run in PR CI.

  5. added
    priority/highNeeds near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker.
    github_actionsPull requests that update GitHub Actions code
    and removed
    priority/normalStandard review queue; useful PR with ordinary maintainer urgency.
    on Sep 5, 2026
  6. DeusData commented on Sep 27, 2026

    @DeusData
    Owner

    Fixed on main in 11b662f (PR #1716, thanks to @astandrik): the test binary now reports its sanitizer build define, and the sanitizer lanes refuse to run when the compiled configuration doesn't match the lane. Thank you for raising it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workinggithub_actionsPull requests that update GitHub Actions codemaintainer-notesInternal observations and improvement notespriority/highNeeds near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker.

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions