Skip to content

Commit 475db1e

Browse files
authored
Merge pull request #2596 from DeusData/fix/msan-hidden-uninit-reads
fix(msan): five "stack-overflow" suites were hiding two uninitialized reads
2 parents 879c11e + dbc6ac4 commit 475db1e

7 files changed

Lines changed: 90 additions & 101 deletions

File tree

‎.github/workflows/_test.yml‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -370,16 +370,19 @@ jobs:
370370
# artifact — the local ladder cannot answer that, having no faithful x86-64
371371
# emulation. It has now answered it, and the answer SPLIT the list:
372372
#
373-
# - the five deep-recursion suites overflow their thread stacks on x86-64
374-
# too, so that limit is NOT architectural;
373+
# - five suites aborted with "stack-overflow ... nested bug" on both
374+
# architectures. That was not deep recursion: it was MSan crashing while
375+
# it reported real uninitialized reads (reporting with the slow unwinder
376+
# exhausted the main thread's stack). scripts/msan.sh now unwinds with
377+
# frame pointers, the
378+
# reports print, the reads are fixed, and the five suites run again;
375379
# - `cli` does not overflow here at all — it fails in the install path for
376380
# an unrelated, still-undiagnosed reason;
377381
# - `incremental` was a shadow-memory RSS artifact, now fixed in the test
378382
# itself rather than skipped, so it stays in the lane.
379383
#
380384
# Per-cause detail lives in scripts/msan.sh, which is the single authoritative
381-
# list both venues now share. Zero use-of-uninitialized-value findings were
382-
# reported across the lane on either architecture.
385+
# list both venues now share.
383386
test-msan:
384387
if: ${{ inputs.lanes == 'all' || contains(fromJSON(inputs.lanes), 'msan') }}
385388
runs-on: ubuntu-latest

‎internal/cbm/vendored/grammars/MANIFEST.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ row instead.
111111
| purescript | `purescript/scanner.c`, serialize | guard `memcpy(buffer, indents->data, to_copy)` with `if (to_copy > 0)` | UBSan: zero-length `memcpy` with a NULL/0-size source when the indent vector is empty (formal UB, harmless) |
112112
| plsql | `plsql/parser.c`, include | `#include <tree_sitter/parser.h>` → `#include "tree_sitter/parser.h"` | The older ABI-14 generator emits angle brackets; every other vendored grammar uses the quoted form, which resolves the per-grammar `tree_sitter/` header from the including file's directory |
113113
| properties | `properties/scanner.c`, whole file | move the `reached_eof` flag out of a file-scope `static bool` and into a per-parser payload allocated by `..._external_scanner_create()` | **Data race, and the only one of its kind in 103 vendored scanners.** That flag is how the scanner refuses to emit a second end-of-input `FAKE_EOL`, and upstream keeps it in ONE process-wide object. With several worker threads indexing `.properties` files at once, whichever reached EOF first set the flag, so another thread's `!reached_eof` was false and its parse never received the `FAKE_EOL` it needed to finish — it sat at end-of-input asking for a token that would never arrive. Measured 2026-09-19: ~106 M parse operations on a 253-byte file before the per-file budget stopped it, after which the file was dropped from the graph and java/kotlin indexes differed run to run. Interleaved A/B under identical load, 40 runs each: unpatched 6 stalls, patched 0. The serialize/deserialize wire format is unchanged (the state is still the returned length). Upstream `6310671b24d4` still carries the static, so a re-vendor must re-apply this |
114+
| wit | `wit/scanner.c`, `..._external_scanner_create` | `{}` → `{ return NULL; }` | **Uninitialized read (MSan).** The function returns `void *` but had no `return`, and tree-sitter's runtime stores the result as the scanner payload, so every WIT parse used an indeterminate value (C11 6.9.1p12 makes that undefined). The scanner never reads its payload, so ordinary builds were unaffected, and the vendored build's `-w` hid `-Wreturn-type`. Under MSan the report of this read crashed and posed as a "stack-overflow", which is why five grammar/pipeline suites were excluded from the MSan lane. Upstream bytecodealliance/tree-sitter-wit still carries it at `059bfdd1af72` (2025-06-23, the last change to `src/scanner.c`) |
114115
| swift | `swift/scanner.c`, `OP_SYMBOL_SUPPRESSOR` + `eat_operators` | `1UL <<` / `1 <<` → `1ULL <<` | UBSan: `1 << suppressor` shifts an `int` by up to `TOKEN_COUNT` bits, undefined once the index reaches 31, while the mask it feeds is `uint64_t`. `1UL << FAKE_TRY_BANG` is the same defect on Windows, where `unsigned long` is 32 bits and `FAKE_TRY_BANG` is 32; the CLANGARM64 leg runs UBSan in trap mode, so there it is an illegal instruction rather than a log line. Upstream already carries both changes: `fb63a7004f07` (2026-04-06, upstream #558) for `eat_operators`, `6ab8d1d74ebd` (2026-08-10) for the `OP_SYMBOL_SUPPRESSOR` entry. Our pin `8abb3e8b3325` (2026-03-20) predates both, so this is a backport rather than a local invention — a re-vendor past 2026-08-10 should delete this row, not re-apply it |
115116

116117
## Vendored from verified upstream

‎internal/cbm/vendored/grammars/wit/scanner.c‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎scripts/ci/msan-lane.sh‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,13 +38,14 @@ run_suite() {
3838
# excluded suite and the cause it is excluded for).
3939
#
4040
# This leg used to force it empty to settle whether the local arm64
41-
# exclusions were an aarch64 artifact. It answered that: the five
42-
# deep-recursion suites overflow on x86-64 too, `cli` fails for an unrelated
43-
# install-path reason, and `incremental` was a shadow-memory RSS artifact
44-
# that is now fixed in the test rather than skipped. Keeping the override
45-
# would re-red the gate for causes already recorded, so the question is
46-
# closed and the venues share one list. scripts/msan.sh still warns loudly
47-
# that the lane is partial, which is the honest signal to keep.
41+
# exclusions were an aarch64 artifact. They were not: `cli` fails for an
42+
# install-path reason, `incremental` was a shadow-memory RSS artifact (now
43+
# fixed in the test), and the five suites that "overflowed" on both
44+
# architectures were hiding real uninitialized reads whose reports crashed
45+
# under the slow unwinder. With fast unwinding (scripts/msan.sh) those reports
46+
# print, the reads are fixed, and the five suites are back in the lane.
47+
# Only `cli` is still excluded, and scripts/msan.sh warns that the lane is
48+
# partial while anything is.
4849
echo "=== MSan suite (exclusions per scripts/msan.sh) ==="
4950
docker run --rm -v "$PWD:/src" -w /src "$IMAGE"
5051
}

‎scripts/msan.sh‎

Lines changed: 68 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,9 @@
99
# the clang-analyzer lane do not cover dynamically. halt_on_error stays ON:
1010
# a finding is a bug (or an interceptor gap to triage), never board data.
1111
#
12-
# VENUES: the CI test-msan job (x86-64) is AUTHORITATIVE and runs with no
13-
# exclusions. The local container is arm64, where deep-recursion suites
14-
# overflow their thread stacks under instrumentation — MSan's shadow/stack
15-
# handling is materially better supported on x86-64, so that limitation may
16-
# be architectural, and the local ladder has no faithful x86-64 emulation to
17-
# decide it. An accepted venue divergence for this lane specifically: the
18-
# exclusions below are the LOCAL default only, and CI overrides them away.
12+
# VENUES: the CI test-msan job (x86-64, authoritative) and the local arm64
13+
# container (test-infrastructure/docker-compose.yml, service test-msan) both
14+
# run this script with the same exclusion list below.
1915
#
2016
# Usage: scripts/msan.sh [suite ...] (default: full suite)
2117
set -euo pipefail
@@ -33,12 +29,10 @@ fi
3329
# one cannot be shadowed); its headers live under the MSan prefix.
3430
# MSAN_ORIGINS: 2 = full origin chains (best reports, heaviest frames),
3531
# 1 = immediate origin only, 0 = none. Detection is IDENTICAL at every level;
36-
# only report quality differs, so lowering it is the first lever to try when
37-
# frame inflation overflows a deep-recursion suite.
38-
# Default 1 on this lane: identical DETECTION at every level (only report
39-
# depth differs), and the frame savings are what lets the deep-recursion
40-
# grammar suites fit their stacks under instrumentation. Export
41-
# MSAN_ORIGINS=2 locally when chasing a specific report's origin chain.
32+
# only report quality differs.
33+
# Default 1 on this lane: the immediate origin is enough to locate a read, and
34+
# the lighter frames keep the lane cheaper. Export MSAN_ORIGINS=2 locally when
35+
# chasing a specific report's origin chain.
4236
MSAN_ORIGINS="${MSAN_ORIGINS:-1}"
4337
if [ "$MSAN_ORIGINS" = "0" ]; then
4438
MSAN_ORIGIN_FLAG=""
@@ -70,11 +64,20 @@ make -j"$(nproc)" -f Makefile.cbm build/msan/test-runner \
7064
CXX_STDLIB_FLAGS="-stdlib=libc++ -nostdinc++ -isystem $MSAN_PREFIX/include/c++/v1" \
7165
CXX_STDLIB="-L$MSAN_PREFIX/lib -Wl,-rpath,$MSAN_PREFIX/lib -lc++ -lc++abi"
7266

73-
export MSAN_OPTIONS="${MSAN_OPTIONS:-halt_on_error=1:print_stats=0}"
74-
# Origin tracking inflates every frame; the grammar-corpus suites drive the
75-
# deepest parser recursion in the tree. Raise what can be raised (main-thread
76-
# stack via RLIMIT_STACK, worker stacks via the sanitized-build-only knob in
77-
# cbm_thread_create) — see the exclusion note below for what this does NOT fix.
67+
# fast_unwind_on_fatal=1: a report's stack is walked with frame pointers (this
68+
# build keeps them, -fno-omit-frame-pointer) instead of the default slow
69+
# unwinder, which in this image is the MSan-instrumented libunwind. With the
70+
# slow unwinder, reporting an uninitialized read exhausted the main thread's
71+
# stack, so the report never printed and MSan aborted with "stack-overflow"
72+
# followed by "nested bug in the same thread" instead. That disguise kept five
73+
# suites excluded as deep recursion while they hid a real read (the WIT
74+
# scanner's create function returned nothing; see the grammar MANIFEST.md).
75+
# Measured on the same tree and test: slow unwind aborts at the 256 MiB stack
76+
# limit, fast unwind prints the use-of-uninitialized-value report.
77+
export MSAN_OPTIONS="${MSAN_OPTIONS:-halt_on_error=1:print_stats=0:fast_unwind_on_fatal=1}"
78+
# Instrumented frames are several times larger than plain ones, so give the
79+
# stacks headroom: the main thread via RLIMIT_STACK, worker threads via the
80+
# sanitized-build-only knob in cbm_thread_create.
7881
ulimit -s 262144 2>/dev/null || true
7982
export CBM_THREAD_STACK_MB="${CBM_THREAD_STACK_MB:-256}"
8083
export LD_LIBRARY_PATH="$MSAN_PREFIX/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}"
@@ -102,22 +105,22 @@ fi
102105

103106
echo "=== MSan lane: $(clang --version | head -1) ==="
104107

105-
# KNOWN RED — THREE DISTINCT CAUSES, whitelisted per cause (O10)
108+
# KNOWN RED — whitelisted per cause (O10). Only (B) is still excluded.
106109
#
107-
# The x86-64 CI leg deliberately runs this lane WITHOUT these exclusions, to
108-
# settle which limits are architectural. It has now done so, and it SPLIT the
109-
# seven suites an earlier version of this block lumped together as one cause.
110-
# That earlier claim -- "seven suites abort with stack-overflow" -- was wrong
111-
# for two of them and is corrected here.
112-
#
113-
# (A) STACK-OVERFLOW under instrumentation — FIVE suites:
114-
# grammar_regression grammar_labels pipeline lang_contract grammar_probe_e
115-
# Each aborts with "MemorySanitizer: stack-overflow" followed by "nested bug
116-
# in the same thread, aborting". The fault is always the same pc (inside
117-
# MSan's memset interceptor -- the instruction that happens to touch the
118-
# guard page, not the recursion source) on a pipeline worker thread. It
119-
# reproduces on BOTH arm64 (local) and x86-64 (CI logged 5 overflows), so it
120-
# is NOT the aarch64 shadow-mapping artifact an earlier note here claimed.
110+
# (A) "STACK-OVERFLOW" — FIXED, no longer excluded. Five suites
111+
# (grammar_regression grammar_labels pipeline lang_contract
112+
# grammar_probe_e) aborted on both arm64 and x86-64 with "MemorySanitizer:
113+
# stack-overflow" followed by "nested bug in the same thread, aborting".
114+
# It was never deep recursion. MSan crashed while REPORTING real
115+
# uninitialized reads (see fast_unwind_on_fatal above): the WIT scanner's
116+
# create function returned nothing (the four grammar suites), and
117+
# githistory_coupling_limits_output left its commits' timestamps
118+
# uninitialized (pipeline). Both are fixed and the suites are back in the
119+
# lane. The crashing thread was the MAIN thread: MSan's "T<n>" is the OS
120+
# thread id, and every abort showed T<n> equal to the pid. An earlier
121+
# version of this block read it as a worker-thread ordinal, which is why
122+
# the worker-stack knob, MSAN_ORIGINS and CBM_WORKERS=1 never changed
123+
# anything and why it proposed a byte-measuring recursion guard.
121124
#
122125
# (B) INSTALL/ACTIVATION failures — cli. NOT an overflow. On x86-64 the suite
123126
# runs to completion: 253 passed, 5 failed, every failure in the install or
@@ -126,10 +129,9 @@ echo "=== MSan lane: $(clang --version | head -1) ==="
126129
# suite is green on every other venue. MSan reported ZERO
127130
# use-of-uninitialized-value here, so excluding it costs no uninit coverage
128131
# -- which is the only thing this lane exists to provide.
129-
# NOT DIAGNOSED, and honestly so: the local lane is arm64, where these
130-
# suites hit (A) long before reaching this code, so there is no faithful
131-
# venue to iterate in and each attempt costs a ~30min CI round trip.
132-
# Recorded as a follow-up rather than guessed at from a log.
132+
# NOT DIAGNOSED yet. Recorded as a follow-up rather than guessed at from a
133+
# log; with (A) understood, the local arm64 container is a usable venue
134+
# for it (scripts/msan.sh cli).
133135
#
134136
# (C) RSS BUDGET — incremental. FIXED rather than excluded, so it is no longer
135137
# in the list below: the budget assertion is now skipped under
@@ -139,51 +141,45 @@ echo "=== MSan lane: $(clang --version | head -1) ==="
139141
# instead would blind the guard on the platforms where it does work. The
140142
# suite stays IN this lane and keeps its uninitialized-read coverage.
141143
#
142-
# WHAT WAS TRIED for (A) — each disproven by measurement, do not repeat:
143-
# 1. RLIMIT_STACK 8 -> 64 -> 256 MiB, and `ulimit -s unlimited`. No effect;
144-
# the crashing thread is not the main thread.
145-
# 2. CBM_THREAD_STACK_MB at 256 MiB and at 1024 MiB (the cap). The knob is
146-
# verified compiled in (CBM_SANITIZED_BUILD is defined for this lane) and
147-
# the floor is applied in cbm_thread_create for both the default and
148-
# explicit-size paths. The fault address did not move by a single byte
149-
# between 256 MiB and 1024 MiB -- a 4x stack increase changing nothing is
150-
# what rules out "the stack is merely too small".
151-
# 3. MSAN_ORIGINS 2 -> 1 -> 0. Detection is identical at every level and 0
152-
# gives the smallest frames; same thread, same address. Frame inflation is
153-
# not the trigger.
154-
# 4. One suite per process (the sharding below). It removed the cumulative
155-
# thread-ordinal effect an earlier note suspected, and thousands of tests
156-
# now run before the wall, but the deep suites still abort.
157-
# 5. CBM_WORKERS=1, to push the recursion onto the main thread where the
158-
# rlimit does apply. Still a worker thread, still overflows.
159-
#
160-
# WHAT THIS POINTS AT (the follow-up, not a guess to act on blindly): the
161-
# recursion guards this tree does have -- see the stack_overflow_a/b/c suites,
162-
# which pass -- bound DEPTH, while the resource actually exhausted is BYTES.
163-
# Instrumented frames are several times larger, so the byte budget is gone
164-
# before the depth counter trips. If that is right, the fix is a guard that
165-
# measures remaining stack rather than counted depth, and it would make these
166-
# suites pass under every sanitizer rather than papering over one lane.
144+
# WHY (B) IS EXCLUDED RATHER THAN LEFT RED: the lane is gating. A permanently
145+
# red gate teaches everyone to ignore it and hides the findings the other
146+
# suites produce, which is the entire reason this lane exists. The exclusion is
147+
# narrow, named, and expires when the install failure is diagnosed. It is not a
148+
# claim that cli is covered, and the lane says so when it runs (below).
167149
#
168-
# WHY (A) AND (B) ARE EXCLUDED RATHER THAN LEFT RED: the lane is gating. A
169-
# permanently red gate teaches everyone to ignore it, and it hides the
170-
# uninitialized-read findings the other ~130 suites DO produce -- which is the
171-
# entire reason this lane exists. Each exclusion is narrow, named, and expires
172-
# with its own fix: (A) when the recursion guard measures bytes, (B) when the
173-
# install failure is diagnosed on a venue that can run it. Neither is a claim
174-
# that those suites are covered.
175-
MSAN_EXCLUDE="${MSAN_EXCLUDE-grammar_regression grammar_labels pipeline cli lang_contract grammar_probe_e}"
150+
# LESSON FROM (A): an MSan abort that is not a use-of-uninitialized-value
151+
# report is a reporting failure until proven otherwise. Look for the read
152+
# first (fast unwinding prints it), not for a stack limit.
153+
MSAN_EXCLUDE="${MSAN_EXCLUDE-cli}"
176154

177155
if [ "$#" -gt 0 ]; then
178156
./build/msan/test-runner "$@"
179-
elif true; then
157+
else
180158
# One suite per process: a fresh process per suite keeps one suite's
181159
# thread/allocator state from reaching the next, and makes a failure name
182160
# exactly one suite. Suite enumeration comes from --list-suites, whose
183161
# completeness the sharding union guard already proves; the known-red set
184162
# documented above is skipped by name and nothing else is.
163+
all_suites="$(./build/msan/test-runner --list-suites)"
164+
# Fail loudly if an entry matches no suite: a typo (or a TEST name given
165+
# where a SUITE name is required) would otherwise exclude nothing silently.
166+
# A case match, not `printf | grep -q`, which pipefail can turn into a
167+
# false failure when grep exits early.
168+
for e in $MSAN_EXCLUDE; do
169+
case $'\n'"$all_suites"$'\n' in
170+
*$'\n'"$e"$'\n'*) ;;
171+
*)
172+
echo "FATAL: MSAN_EXCLUDE entry '$e' is not a suite name" >&2
173+
exit 1
174+
;;
175+
esac
176+
done
177+
if [ -n "$MSAN_EXCLUDE" ]; then
178+
echo "WARNING: running a PARTIAL sanitizer lane — excluded: $MSAN_EXCLUDE" >&2
179+
echo "WARNING: a green result here does NOT mean the tree is MSan-clean." >&2
180+
fi
185181
fails=""
186-
for suite in $(./build/msan/test-runner --list-suites); do
182+
for suite in $all_suites; do
187183
case " $MSAN_EXCLUDE " in
188184
*" $suite "*)
189185
echo "=== msan: $suite SKIPPED (known-red, see the block above) ==="
@@ -197,18 +193,4 @@ elif true; then
197193
echo "=== MSan lane FAILED suites:$fails ===" >&2
198194
exit 1
199195
fi
200-
else
201-
echo "WARNING: running a PARTIAL sanitizer lane — excluded: $MSAN_EXCLUDE" >&2
202-
echo "WARNING: a green result here does NOT mean the tree is MSan-clean." >&2
203-
excl_pattern="$(printf '%s\n' $MSAN_EXCLUDE | tr '\n' '|' | sed 's/|$//')"
204-
suites="$(./build/msan/test-runner --list-suites | grep -Evx "$excl_pattern")"
205-
# Fail loudly if an entry matched no suite: a typo (or a TEST name given
206-
# where a SUITE name is required) would otherwise exclude nothing silently.
207-
for e in $MSAN_EXCLUDE; do
208-
./build/msan/test-runner --list-suites | grep -qx "$e" || {
209-
echo "FATAL: MSAN_EXCLUDE entry '$e' is not a suite name" >&2; exit 1; }
210-
done
211-
echo "--- excluded: $MSAN_EXCLUDE (see the comment in $0) ---"
212-
# shellcheck disable=SC2086 # deliberate word-splitting of the suite list
213-
./build/msan/test-runner $suites
214196
fi

0 commit comments

Comments
 (0)