fix(spec): bound buffer_write by size, and audit what bounds a slice write - #3431
Merged
Conversation
…write Closes #3430 Refs #3428 I closed #3428 saying I had not measured how widespread that shape was. Of 4692 emitted functions, 67 take a slice parameter and 59 index into one. Classifying the bound took three matcher attempts and each earlier answer was wrong: * "34 write without bounding by .len()" -- true and useless. Most bound by an explicit length PARAMETER, which is the only thing that can work in C. * "3 have a length parameter and do not use it as the bound" -- two of the three were false positives. ternary_shift_right starts its loop at len - 1 and my regex looked for `while (... len ...)`. * "24 bounded by a compile-time constant" -- inflated. TRIT_NEG, TRIT_POS and TRIT_ZERO are VALUES, and matching "a declared constant appears in the body" swept in byte_to_trits, already hand-verified as correct. Hand-verified ten functions by eye: two matcher false positives, eight genuinely bound by something other than the buffer. I did not read all 59, so the class is between 8 and 24 and this publishes no single number for it. Four bounds are in use across the corpus: the buffer's own .len() (which C cannot honour, having no length); an explicit length parameter; a compile-time constant; and nothing at all. cache_kv writes cache_k[position * EMBED_DIM + i], so its index grows with `position` and nothing bounds `position`. Fixed here because it is unambiguous: buffer_write never compared `head` to `size`. The evidence that this is an oversight rather than a convention is in the same file -- the sibling buffer_read guards its index, and its guard was `tail == size`, which catches exactly one-past-the-end and lets every larger index through. Both now use `>=`. The bound uses `size`, not buf_in.len, deliberately: a []T loses its length at the C ABI, so a .len-based guard would exist in the Rust and Zig outputs and be absent from C. With the parameter, `if ((head >= size))` appears identically in both. Left as a decision: the constant-bounded set -- hash_insert, hash_remove, mem_store, mem_store_block, set_insert, cache_kv and the two gla_* -- is a convention with an unstated precondition, enforced by nothing, and changing it means adding length parameters and changing the ABI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gHashTag
enabled auto-merge (squash)
September 7, 2026 23:01
Contributor
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
gHashTag
added a commit
that referenced
this pull request
Sep 7, 2026
Closes #3432 Refs #3430 One pass ago I listed cache_kv as needing "either a new parameter or .len, and that is a decision". It was not: `const CONTEXT_LEN : usize = 81; // Max sequence length` is declared eleven lines above it in the same file. Three functions share the shape, and two of them were not in that audit at all -- they were found by reading the neighbours of the one I had flagged: apply_rope_qk rope_tables.cos[position * ROPE_PAIRS + pair_idx] against [CONTEXT_LEN * ROPE_PAIRS] -- out-of-bounds READ cache_kv cache_k[position * EMBED_DIM + i] against // [CONTEXT_LEN][EMBED_DIM] -- out-of-bounds WRITE compute_scores scores[h * CONTEXT_LEN + j], j < seq_len against [NUM_HEADS * CONTEXT_LEN] -- runs off every row Nothing bounded `position` or `seq_len`. In the generated Rust each panics; in C it does not, the parameter being a bare pointer. The intent was already written down twice. The declaration carries the shape in a comment -- `cache_k: []f32, cache_v: []f32, // [CONTEXT_LEN][EMBED_DIM]` -- and the spec's own test allocates exactly `[0.0; EMBED_DIM * CONTEXT_LEN]`. Stated in a comment, exercised by a test, enforced by nothing. The guard uses the CONSTANT, not `.len`, for the same reason as #3431: a []T loses its length at the C ABI, so a .len-based guard would exist in the Rust and Zig outputs and be absent from C. With a constant the identical check appears in all three. compute_scores clamps seq_len rather than returning, because a caller asking for more rows than the buffer holds still wants the ones that exist. rustc errors on the file are unchanged at 23; those are pre-existing and from other causes. The lesson is that I read the function and not the file. Twice now the thing I called a decision was a constant already in scope. Co-authored-by: lab <lab@example.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Closes #3430 · Refs #3428
I closed #3428 saying I had not measured how widespread that shape was. This measures it — and most of what my matchers first told me was wrong.
The population
Of 4692 emitted functions: 67 take a slice parameter, 59 index into one.
Three matcher attempts, each earlier answer wrong
.len()"ternary_shift_rightstarts its loop atlen - 1, and my regex looked forwhile (... len ...)TRIT_NEG/TRIT_POS/TRIT_ZEROare values, and "a declared constant appears in the body" swept inbyte_to_trits, already hand-verified as correctHand-verified ten functions by eye: two matcher false positives, eight genuinely bound by something other than the buffer. I did not read all 59, so the class is somewhere between 8 and 24, and this publishes no single number for it.
Four different bounds are in use
.len()pipeline_runafter #3429byte_to_trits,mac_parallel_multiplyhash_insert/TABLE_SIZE,mem_store/MEM_SIZE,set_insert/SET_MAXbuffer_writecache_kvearns its own line: it writescache_k[position * EMBED_DIM + i], so the index grows withpositionand nothing boundsposition.The one that is unambiguous, fixed here
buffer_writenever comparedheadtosize. The evidence that this is an oversight and not a convention is inside the same file: the siblingbuffer_readguards its index — and its guard wastail == size, catching exactly one-past-the-end and letting every larger index through. Both now use>=.The bound uses
size, notbuf_in.len, deliberately: a[]Tloses its length at the C ABI, so a.len-based guard would live in the Rust and Zig outputs and be absent from C. With the parameter, the identical check appears in both:Left as a decision
The constant-bounded set is a convention with an unstated precondition — "your buffer must be at least CONST long". Coherent, enforced by nothing in any backend, and changing it means adding length parameters and changing the ABI.
All four backends generate; all three guards green —
STALE 0,936/936,80/80,CONVERGED pairs run: 2— and both seals for the edited spec refreshed.