fix(spec): bound attention's position and seq_len by CONTEXT_LEN - #3433
Merged
Conversation
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: Claude Opus 5 <noreply@anthropic.com>
gHashTag
enabled auto-merge (squash)
September 7, 2026 23:14
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
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 #3432 · Refs #3430
One pass ago I listed
cache_kvas needing "either a new parameter or.len, and that is a decision". It was not. The bound was declared eleven lines above it:Three functions, one file, the same shape
apply_rope_qkrope_tables.cos[position * ROPE_PAIRS + pair_idx][CONTEXT_LEN * ROPE_PAIRS]f64cache_kvcache_k[position * EMBED_DIM + i]// [CONTEXT_LEN][EMBED_DIM]compute_scoresscores[h * CONTEXT_LEN + j],j < seq_len[NUM_HEADS * CONTEXT_LEN]f64Two of the three were not in the #3430 audit at all. I found them by reading the neighbours of the one I had flagged.
In the generated Rust each panics. In C they do not —
void cache_kv(AttentionBuffers* buffers, size_t position, double* cache_k, double* cache_v)is a bare pointer, so the write simply happens.The intent was already written down twice
Stated in a comment on the declaration, allocated to by the specs own test, enforced by nothing.
The guard uses the constant, not
.lenSame reasoning as #3431: 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 a constant the identical check reaches all three:compute_scoresclampsseq_lenrather than returning: 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, all pre-existing and from other causes. All three guards green —
STALE 0,936/936,80/80,CONVERGED pairs run: 2— and both seals refreshed.What this says about my own audit
I read the function and not the file. Twice now the thing I called a decision was a constant already in scope — this, and the CI cost two passes ago.