Re-audit degenerate-container UB, and put the class under CI - #178
Merged
Conversation
added 2 commits
August 7, 2026 20:10
The July sign-off on this class was wrong twice in a week (#124, #151), both times because it argued reachability transitively rather than checking. Redo it properly, and — the part that lasts — stop relying on a human pass at all. The class needs two detectors that are blind to each other: UBSan's nonnull check sees a null `.data()` reaching `memcpy`, and hardened libstdc++ sees `&v[0]` on an empty vector, which forms an out-of-range address without ever loading from it. Both legs now exist in CI, so coverage is bounded by which *inputs* reach them — not by code reading. Static pass over all 69 memcpy/memmove/memset sites plus the .front() / .back() / .data()+i siblings: every one is guarded or provably non-empty at the site. Dynamic pass: 1704 runs under a local -D_GLIBCXX_ASSERTIONS build, over degenerate shapes x entry points x weighting modes, the 37 TS_* alternative kernels, and the >=150-tip L3b regime that no test had ever entered. No aborts — and a positive control proves that statement means something: reverting the #151 guard aborts on the first call. Adds tests/testthat/test-ts-degenerate-shapes.R (Tier 2, ~5 s), whose job is to put these shapes in front of both sanitizer legs on every dispatch. It aborts against a build with the #151 guard removed, so it is not tautological. One code fix: a 0 x 2 `startEdge` matrix passed every existing shape check and reached `flat.data() + n_edge` on an empty vector — undefined before C++20, and reported by neither detector. Fixes #177
`l3b_active` needs more than the TS_L3B_INCREMENTAL knob -- also a null sector mask, no tabu list and no pool collection -- so "the knob buys the L3b sites" was a reachability claim of exactly the kind the July audit got wrong. Checked it: TS_L3B_STATS=1 reports patch_clips=36 on the forced 12-tip call, so the path does engage. Say so where the claim is made. Also note that the zero-word datasets in the same test are the other side of that guard -- L3b is correctly inert for them -- and hedge the audit note's env-knob row, where per-knob path engagement was not verified.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #177.
The July sign-off on degenerate-container UB was wrong twice in a week
(#124, #151), both times for the same reason: it argued reachability
transitively — "callers cannot produce zero words" — instead of checking,
and one of its false claims went into the memory archive, where it then hid
#124 from a later reader for a month. So this redoes the audit, and, more
usefully, stops the class depending on a human pass at all.
Why one detector was never going to be enough
nonnullparametermemcpy(v.data(), w.data(), 0),vemptynonnull-attribute(gcc-ASAN)_GLIBCXX_ASSERTIONS, plain builds&v[0],v.back()on emptyv— no load or store_GLIBCXX_ASSERTIONS(glibcxx-assertions, from #60)Neither leg sees the other's sub-class, so "the sanitizers are clean" says
something only once both have executed the shape. Coverage here is bounded
by which inputs reach the detectors — never by reading code. That is the
finding, and it is why the deliverable is a test file rather than a report.
Static pass — all 69
memcpy/memmove/memsetsitesEach classified as guarded, or provably non-empty at the site; a proof
that appeals to what callers can produce does not count.
MaddisonSlatkin.cpp,ts_data.cpp_padsizeof(data)cannot be zerots_tbr.cpp:930TbrSnapshot::save/restorets_bench_tbr_phasests_tree.cppload_tip_states,save_node_statetip_bytes > 0/total_words == 0ts_tree.cpprestore_prealloc_undowhile (u.count > 0);save_node_statereturns before incrementingcountts_prune_reinsert,ts_sector,ts_collapsedtw > 0/ early returnts_tbr.cppL3b, vroot, NA clipl3b_active/use_directional(both requiretotal_words > 0) orhas_na(implies ≥1 block)ts_splits.cppwps >= 1; emit-loop filters identical to the count loop'sNo unguarded site. The siblings
.front()/.back()/.data() + iwereswept too (45 + 11); every
.back()is inside awhile (!stack.empty())oron
postorder, which is never empty.Dynamic pass — 1704 runs under
-D_GLIBCXX_ASSERTIONSTS_*environment knobs, plus the ≥150-tip regime that activates L3bZero aborts. Battery 3 is the one that mattered:
l3b_activeneedsn_tip >= 150unlessTS_L3B_INCREMENTALis set, so eightts_tbr.cppsites had never been executed by any test at any point.
Positive control, because "no aborts" is uninterpretable without proof
the harness can abort: reverting the #151 guard and rebuilding gives
stl_vector.h:1130: Assertion '__n < this->size()' failed,rc=127, on thefirst degenerate call; restoring it returns the run to green.
What lands
tests/testthat/test-ts-degenerate-shapes.R— Tier 2, 329 expectations,~5 s. Its expectations are contract checks; its job is to put these shapes
in front of both sanitizer legs on every dispatch. It aborts against a
build with the #151 guard removed, so it is not tautological.
It forces L3b with
TS_L3B_INCREMENTALon a 12-tip tree instead of payingfor 150.
l3b_activeneeds more than that knob — also a null sector mask,no tabu list, no pool collection — so rather than assume, I checked: under
TS_L3B_STATS=1the forced call reportspatch_clips=36. The zero-worddatasets in the same test are the other side of that guard, where L3b is
correctly inert.
One code fix. A
0 x 2startEdgematrix satisfied every existingshape check, left
flatempty, and reachedflat.data() + n_edge—pointer arithmetic on a possibly-null pointer, undefined before C++20 and
reported by neither detector. Without the new guard the existing
test-ts-driven.Rcase fails withEach startEdge matrix must describe a binary tree, i.e. it demonstrably reached the site.What this does NOT establish
_GLIBCXX_ASSERTIONShardens libstdc++ containers only. Raw arrays, andraw pointers derived from a container (
const uint64_t* bits = &v[i];then
bits[w]), are unchecked — those reads are ASan's job.instance was a zero-Fitch-word dataset, so that axis is now covered hard
and others less so.
not verified individually. Only L3b was load-bearing, and only L3b was
confirmed.
Write-up:
dev/red-team/reviews/container-ub-class-audit/. A newdataset-taking entry point should join
test-ts-degenerate-shapes.Rin thecommit that introduces it.
CI
agent-check 31210301540
— green, all three jobs. The
libstdc++ hardened assertionsleg reports 34compiler invocations carrying the flag and zero assertion failures.
gcc-ASAN 31210334799
— 0
runtime error, 0nonnullreports, which is the result this PRturns on. The
testsjob is nonetheless red, for a reason that predatesthis branch:
test-Concordance.R:433callstools::Rd_db("TreeSearch"),and the ASan container installs without a help database, so it errors with
"installed help of package 'TreeSearch' is corrupt". The identical failure
is on run 31193721419
(
feature/consistency-fixes, three hours before this branch existed).Filed separately; nothing here touches Rd or roxygen.
Both legs demonstrably executed the new file rather than skipping it:
ts-degenerate-shapesreports 329 expectations in each, matching thelocal count exactly, and
ts-drivenreports 185 (184 + the one new case).Attribution:
ms609-agentis suspended, soghwrites — this PR, #177,and its comments — land under
ms609rather than the agent account.