Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds validation for stop_token_ids in SamplingParams.verify to ensure they fall within the valid range [0, vocab_size - 1], preventing potential out-of-bounds CUDA errors or silent drops. It also adds corresponding unit tests. The reviewer suggested removing the redundant int() conversion on token_id during validation, as stop_token_ids are already guaranteed to be integers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # the whole server), and id == vocab_size is silently dropped by the | ||
| # trailing [:, :vocab_size] slice. Validate like logit_bias keys. | ||
| for token_id in self.stop_token_ids: | ||
| if not 0 <= int(token_id) < vocab_size: |
There was a problem hiding this comment.
Since self.stop_token_ids is already processed in __post_init__ (where all elements are filtered and converted to integers via int(t)), each token_id is guaranteed to be an int. Calling int(token_id) here is redundant and can be safely omitted to clean up the code and avoid unnecessary overhead in the request validation hot path.
| if not 0 <= int(token_id) < vocab_size: | |
| if not 0 <= token_id < vocab_size: |
|
cc @hnyls2002 (you have touched this penalizer) and @merrymercy as sampling/managers owner. User |
User-supplied stop_token_ids reach the min_new_tokens penalizer's scatter_add_ over a [vocab_size + 1] tensor with no range check (SamplingParams.verify validates logit_bias keys but not stop_token_ids). An id >= vocab_size + 1 is an out-of-bounds scatter index: on CPU a RuntimeError, on CUDA a device-side assert that poisons the context and takes down the whole server process (every in-flight request) from a single malformed request. An id == vocab_size does not crash but is silently dropped by the trailing [:, :vocab_size] slice, so that stop token never fires. Reject out-of-range stop_token_ids in verify(), matching the existing logit_bias validation, so a bad request fails cleanly at the boundary.
|
Good point, dropped the redundant |
250c4f3 to
6b46a9d
Compare
|
Part of #31597 (validates one of the unbounded request fields described there). |
|
Thanks @v-code01. Closing this because you have 15 PRs open and none updated in 7 days, over our soft cap of 5 for idle PRs. Reopen the two or three you most want reviewed - a short active set moves faster than a long idle one. Some directories moved recently, so an older branch may need retargeting: |
Motivation
User-supplied
stop_token_idsflow into themin_new_tokenspenalizer, which builds ascatter_add_index over a[vocab_size + 1]tensor (penaltylib/min_new_tokens.py:43-58).SamplingParams.verify()validateslogit_biaskeys againstvocab_sizebut does not range-checkstop_token_ids, so:>= vocab_size + 1is an out-of-bounds scatter index. On CPU it's aRuntimeError; on CUDA it's a device-side assert / illegal memory access that poisons the context and takes down the whole server process (all in-flight requests) from a single malformed request. Bothmin_new_tokens > 0and the id are user-controlled (OpenAIstop_token_ids), so this is a network-reachable DoS.== vocab_sizedoes not crash but is silently dropped by the trailing[:, :vocab_size]slice, so that stop token never fires.Repro (CPU, no model):
SamplingParams(min_new_tokens=1, stop_token_ids=[100000]).verify(32000)is accepted today, and driving the real penalizer with that request raisesindex 100000 is out of bounds for dimension 1 with size 32001.Modifications
Reject out-of-range
stop_token_idsinverify(), mirroring the existinglogit_biaskey validation, so a malformed request fails cleanly at the validation boundary instead of at the scatter.Tests
Four cases added to
test_sampling_params.py::TestSamplingParamsVerify(CPU): id>= vocab_size + 1, id== vocab_size, negative id all raise; in-range ids pass. All fail on the pre-fix code.Checklist
CI States
Latest PR Test (Base): ❌ Run #29597096447
Latest PR Test (Extra): ❌ Run #29597096626