Validate eos_token_id against vocab_size to prevent OOB write in ApplyMinLength - #2266
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a security issue where untrusted eos_token_id values from genai_config.json could be used as unchecked indices into per-token score buffers, leading to out-of-bounds writes during ApplyMinLength (CPU and CUDA).
Changes:
- Add generator-construction validation to reject any
eos_token_idoutside[0, vocab_size). - Add defense-in-depth bounds checks at the
ApplyMinLengthsinks for CPU and CUDA implementations. - Add a regression test ensuring generator creation fails when
eos_token_id >= vocab_size.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/generators.cpp |
Validates eos_token_id against vocab_size during generator construction to prevent OOB writes. |
src/search.cpp |
Adds bounds check before indexing per-beam score rows with eos_token_id (CPU defense-in-depth). |
src/cuda/search_cuda.cpp |
Adds bounds check before launching CUDA score-masking kernel with eos_token_id (CUDA defense-in-depth). |
test/sampling_tests.cpp |
Adds regression test covering out-of-range eos_token_id rejection at generator creation. |
tianleiwu
reviewed
Jul 7, 2026
tianleiwu
reviewed
Jul 7, 2026
jiafatom
force-pushed
the
fix/eos-token-id-vocab-bounds
branch
from
July 7, 2026 23:06
d3d4773 to
473ef2f
Compare
kunal-vaishnavi
previously approved these changes
Jul 8, 2026
eos_token_id from genai_config.json is used directly as an index into the per-beam, vocab_size-sized score row in Search::ApplyMinLength. A config value >= vocab_size (or < 0) caused an out-of-bounds write of std::numeric_limits<float>::lowest() past the end of the row (heap-buffer-overflow, CWE-787 / CWE-129). Fix: - Reject out-of-range eos_token_id at generator creation, mirroring the existing top_k <= vocab_size validation. - Defensively skip out-of-range ids at the sink in both the CPU (Search_Cpu::ApplyMinLength) and CUDA (Search_Cuda::ApplyMinLength) backends. - Add a CPU regression test overlaying an out-of-range eos_token_id. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- eos_token_id test now catches std::runtime_error specifically and asserts the message mentions eos_token_id, so unrelated exceptions can't satisfy it. - Reword the validation error to a clear half-open interval: 'must be in range [0, <vocab_size>) (vocab_size)'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… at creation) Per reviewer feedback, remove the defense-in-depth bounds checks in Search_Cpu::ApplyMinLength and Search_Cuda::ApplyMinLength since an out-of-range eos_token_id is already rejected at generator creation and these checks are on the per-token inference hot path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The new creation-time check (eos_token_id must be in [0, vocab_size)) rejected
the sampling tests, which overlay a tiny vocab_size (5/13/17/21) onto the
tiny-random-gpt2-fp32 base config while leaving its eos_token_id=98 in place:
C++ exception "eos_token_id (98) must be in range [0, 5) (vocab_size)"
-> 6 CPU tests failed (Batched/Randomized TopP/TopK), plus the CUDA and
NvTensorRtRtx variants on their runners.
Overlay a valid eos_token_id (0) alongside every small vocab_size overlay
(fixed vocab-5 CPU/CUDA tests, the RunSamplingTest helper, and the
NvTensorRtRtx setup helper). This is behavior-neutral for these tests: none
set min_length, so eos_token_id is never used (it only indexes the score row
in Search::ApplyMinLength). The intentional out-of-range regression test
(EosTokenIdExceedsVocabSizeThrowsCpu, eos_token_id=5 vs vocab_size=5) is left
unchanged and still asserts the validation throws.
Verified: all 10 CPU SamplingTests pass, including the regression test.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jiafatom
force-pushed
the
fix/eos-token-id-vocab-bounds
branch
from
July 10, 2026 16:21
cb1eb80 to
d4ef5ea
Compare
kunal-vaishnavi
approved these changes
Jul 10, 2026
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.
Summary
Fixes an out-of-bounds write (CWE-787 / CWE-129) reachable from a malicious
genai_config.json.Search_Cpu::ApplyMinLengthsuppresses EOS tokens while the sequence is shorter thanmin_lengthby writingstd::numeric_limits<float>::lowest()into the per-beam score row at each configuredeos_token_id:eos_token_idcomes straight from config (src/config.cpp) with only astatic_cast<int>and is never bounded againstvocab_size. A model whoseeos_token_id >= vocab_size(or< 0) drives the store past the end of thevocab_size-sizedGetScores()subspan — a heap-buffer-overflow write on the first decode step (reached viaGenerator::GenerateNextToken->ApplyMinLength). The same pattern exists in the CUDA backend (Search_Cuda::ApplyMinLength).Changes
src/generators.cpp: reject anyeos_token_idoutside[0, vocab_size)in theGenerator::Generatorvalidator, mirroring the existingtop_k <= vocab_sizecheck (added in Fix security vulnerabilities #2224).src/search.cppandsrc/cuda/search_cuda.cpp: defense-in-depth — skip out-of-range ids at the sink in both backends.test/sampling_tests.cpp: addEosTokenIdExceedsVocabSizeThrowsCpu, which overlays an out-of-rangeeos_token_idand assertsOgaGenerator::Createthrows instead of proceeding to the OOB write.Testing
New regression test follows the existing
SamplingTestsoverlay pattern (tiny-random-gpt2-fp32+vocab_size/eos_token_idoverlay). Local build isn't available in my environment; relying on CI to build and run the C++ unit tests.