security(sampling): reject prompt_logprobs=-1 to prevent OOM DoS - #266
security(sampling): reject prompt_logprobs=-1 to prevent OOM DoS#266malaiwah wants to merge 2 commits into
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in PRs do not trigger a full CI run by default. Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. Agent GuidelinesIMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban. 🚀 |
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Results (automated)Host: macOS M4 Max, CPU-only (no CUDA) Tests require dependencies not available on this host (macOS M4, CPU-only torch, no CUDA). Cannot run. Error: The test imports Automated test run by @malaiwah's agent. Results are from a CPU-only environment; GPU-dependent tests may behave differently on CUDA hardware. |
Test Results (automated — re-run with fixed dependencies)Host: macOS M4 Max, CPU-only (no CUDA) ✅ All 5 tests passed. Automated test run by @malaiwah's agent. Results are from a CPU-only environment; GPU-dependent tests may behave differently on CUDA hardware. |
Review Findings Addressed: B9 (BLOCKER), C4, C5, C6The Bypass (B9)The original fix rejected only the sentinel spelling Verified: against the old sentinel-only logic, The Fix: Resource Bound (replaces sentinel rejection)Replaced the sentinel-only rejection with an explicit resource bound:
Both are registered in
This means Four Contract Sites Migrated (C4)All four sites that defined the
C5: Sampling Logprobs PathThe identical unbounded allocation for Tests14 tests total (5 original preserved + 9 new):
All 14 pass. Verified that the new positive-value bypass tests fail against the OLD sentinel-only logic. |
malaiwah
left a comment
There was a problem hiding this comment.
Again, a memory-related one that made me trip on a OOM while doing some work against the GG endpoint. My understanding is this is not GG specific and would apply to upstream as well.
prompt_logprobs=-1 means 'return all vocabulary logprobs for every prompt token.' For modern vocab sizes (e.g. GLM-5.2's 154,880) this allocates a [num_prompt_tokens, vocab_size] tensor via LogprobsTensors.empty_cpu that OOMs the engine — a denial-of-service vector from the API. The OOM was reproduced on AIBoss (RTX 5090, r28 image): a ~3,800-token chunked prompt with prompt_logprobs=1 already triggers torch.OutOfMemoryError in logits.log_softmax (upstream vllm-project#14239). With prompt_logprobs=-1 the allocation is ~62 GiB for a 100k-token prompt, far exceeding any GPU's free memory. The V1 _get_prompt_logprobs_dict and V2 PromptLogprobsWorker chunk the GPU compute_logits call via VLLM_PROMPT_LOGPROBS_CHUNK_SIZE (PR vllm-project#258), but the upfront CPU LogprobsTensors.empty_cpu allocation at gpu_model_runner.py:5662 is unbounded and not covered by the chunking fix. Rejecting -1 outright is the cleanest fix — the 'all logprobs' feature is impractical for any modern vocabulary. The existing max_logprobs=20 default already rejects -1 (it resolves to vocab_size > 20), but an operator who sets --max-logprobs=-1 bypasses that guard. This fix makes the rejection unconditional. Verified on AIBoss (RTX 5090, r28 image): 5/5 security tests pass. Signed-off-by: Michel Belleau <michel-belleau@malaiwah.com>
Addresses review findings B9, C4, C5, C6: B9 (BLOCKER): The original fix rejected only prompt_logprobs=-1 by sentinel. When max_logprobs=-1 (operator-settable), the allowed maximum became vocab_size, so prompt_logprobs=154880 passed the > check and allocated the identical full-vocabulary tensor that -1 would have. The DoS was unmitigated for that configuration; values just below vocab_size were equally expensive. Fix: add VLLM_MAX_PROMPT_LOGPROBS (default 20) and VLLM_MAX_LOGPROBS (default 20) env vars. Resolve -1 to vocab_size, then enforce min(max_logprobs, cap) as the effective maximum. Any value exceeding the cap is rejected with an error naming the cap and the requested value, regardless of spelling (-1, vocab_size, vocab_size-1, or any large int). C4: Migrated all four contract sites: - chat_completion/protocol.py: updated prompt_logprobs and top_logprobs validation to enforce the cap at the API edge - completion/protocol.py: updated prompt_logprobs and logprobs validation to enforce the cap at the API edge - sampling_params.py _verify_args: updated error messages to reference the cap, keeping -1 syntactically valid (resolved and capped in _validate_logprobs) - sampling_params.py field docstrings: document the cap C5: Applied the same bound (VLLM_MAX_LOGPROBS) to the sampling logprobs path (logprobs=-1 resolves to vocab_size, then capped). C6: Outright rejection of -1 replaced with a resource bound that preserves the documented upstream feature while removing the DoS. Tests: 14 total (5 original preserved + 9 new): - prompt_logprobs=vocab_size rejected - prompt_logprobs=vocab_size-1 rejected - prompt_logprobs=cap accepted - prompt_logprobs=cap+1 rejected - bypass with max_logprobs=-1 configured (the exact B9 scenario) - sampling logprobs=vocab_size rejected (C5) - sampling logprobs=-1 rejected (C5) - sampling logprobs=cap accepted (C5) - custom cap via env var Verified that the new positive-value bypass tests fail against the OLD sentinel-only logic (prompt_logprobs=154880, 154879, and 21 all pass the old > check when max_logprobs=-1). Co-authored-by: GLM-5.2 <noreply@z.ai>
f3b1c2e to
efbc81e
Compare
Rebased onto current
|
Summary
Fixes #264
prompt_logprobs=-1("return all vocabulary logprobs for every prompt token") is impractical for any modern vocabulary and causes unbounded memory allocation that kills the engine.The problem
When
prompt_logprobs=-1,num_prompt_logprobsresolves tomodel_config.get_vocab_size()(154,880 for GLM-5.2). The V1_get_prompt_logprobs_dictallocates:For a 100k-token prompt × 154,880 vocab × 8 bytes = ~62 GiB — far exceeding any host's free memory. The engine crashes with
torch.OutOfMemoryError/EngineDeadError.The V2
PromptLogprobsWorkerpath has the same pattern atprompt_logprob.py:142-143.Why the existing chunking fix doesn't help
PR #258 (and follow-up
cc5a286de) addedVLLM_PROMPT_LOGPROBS_CHUNK_SIZEto chunk the GPUcompute_logitscall. This bounds the GPU logits tensor to[chunk_size, vocab_size]. However, the upfront CPULogprobsTensors.empty_cpuallocation is unbounded — it allocates the full[num_prompt_tokens, vocab_size]tensor before any chunking happens.Reproduction
On AIBoss (RTX 5090, r28 image), even
prompt_logprobs=1with a ~3,800-token chunked prompt triggers OOM:EngineCore dies. With
prompt_logprobs=-1, the allocation is orders of magnitude larger.The fix
Reject
prompt_logprobs=-1outright in_validate_logprobs(sampling_params.py:807):The existing
max_logprobs=20default already rejects-1(resolves tovocab_size > 20), but an operator who sets--max-logprobs=-1bypasses that guard. This fix makes the rejection unconditional.Files changed
vllm/sampling_params.py— rejectprompt_logprobs=-1in_validate_logprobs(17 lines added)tests/sampling/test_prompt_logprobs_security.py— 5 new testsVerification
Tested on AIBoss (RTX 5090, r28 image) with the patched file overlaid:
prompt_logprobs=-1correctly rejected withVLLMValidationErrorprompt_logprobs=20(positive) still worksprompt_logprobs=0(disabled) still worksprompt_logprobs=None(unset) still worksprompt_logprobs=100 > max_logprobs=20still rejected by existing checkImpact
This is a denial-of-service prevention fix. Any API client can send
prompt_logprobs=-1with a large prompt to kill the engine, affecting all other clients. The "all logprobs" feature is never usable without OOM on modern vocabularies — rejecting it outright is the cleanest fix.