Reject vocab_size < 2 for beam search to fix OOB in BeamSearch_Cpu::SelectTop - #2272
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens beam search configuration validation to prevent an out-of-bounds access in BeamSearch_Cpu::SelectTop when a malicious (or invalid) genai_config.json sets vocab_size too small for the top_k = 2 * num_beams selection logic.
Changes:
- Add generator-creation validation to reject
num_beams > 1whenvocab_size < 2. - Add a runtime invariant check in
BeamSearch_Cpu::SelectTop(replacing a release-strippedassert) to preventstd::partial_sortfrom operating with an invalid middle iterator. - Add a regression test asserting
OgaGenerator::Createthrows fornum_beams = 2withvocab_size = 1.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/generators.cpp |
Adds upfront validation to reject beam search configs with vocab_size < 2. |
src/search.cpp |
Adds defense-in-depth runtime check guarding partial_sort against top_k > total_elements. |
test/sampling_tests.cpp |
Adds regression coverage for the invalid vocab_size + beam search combination. |
BeamSearch_Cpu::SelectTop partial_sorts an index array of num_beams*vocab_size entries and asks for the top 2*num_beams (top_k). When vocab_size == 1 (and num_beams >= 2), total_elements < top_k, so the partial_sort middle iterator points past the end of select_top_idx_, causing an out-of-bounds read/write (heap-buffer-overflow, CWE-787/CWE-125). The only guard was an assert() that is compiled out under NDEBUG in release builds. Fix: - Reject beam search (num_beams > 1) with vocab_size < 2 in the Generator::Generator validator. - Replace the compiled-out assert in SelectTop with a runtime check that throws, so release builds are protected as defense-in-depth. - Add a CPU regression test overlaying vocab_size = 1 with num_beams = 2. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
David Fan (jiafatom)
force-pushed
the
fix/beamsearch-vocab-size-selecttop
branch
from
July 7, 2026 23:06
caa9057 to
78acea6
Compare
kunal-vaishnavi
approved these changes
Jul 8, 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 a heap out-of-bounds read/write (CWE-787 / CWE-125) reachable from a malicious
genai_config.json.BeamSearch_Cpu::SelectTopbuilds an index array oftotal_elements = num_beams * vocab_sizeentries andpartial_sorts it to surface the toptop_k = 2 * num_beamscandidates:When
vocab_size == 1(andnum_beams >= 2),total_elements = num_beams < top_k = 2*num_beams, sobegin + top_kpoints past the end ofselect_top_idx_.std::partial_sortheapifies[begin, begin+top_k), reading and move-writing slots beyond the allocation. The only guard was anassert, which is compiled out in the shipped release builds (NDEBUG). The path is reached from the normal generation loop (GenerateNextToken->SelectTop).Fix
src/generators.cpp: reject beam search (num_beams > 1) withvocab_size < 2in theGenerator::Generatorvalidator (primary fix, at creation).total_elements >= top_kreduces exactly tovocab_size >= 2.src/search.cpp: replace the compiled-outassert(total_elements >= top_k)with a runtime check that throws, so release builds are protected as defense-in-depth.test/sampling_tests.cpp: addBeamSearchVocabSizeTooSmallThrowsCpu, overlayingvocab_size = 1withnum_beams = 2and assertingOgaGenerator::Createthrows.Note: the greedy path (
GreedySearch_Cpu::SelectTop,num_beams == 1) usesstd::max_elementand is unaffected.Testing
New regression test follows the existing
SamplingTestsoverlay pattern. Local build isn't available in my environment (root-owned build dir / jiafa-dev container needed); relying on CI to build and run the C++ unit tests.