fix(customizer): fix CVEs + vllm venv issue - #1056
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe RL base image updates CUDA, uv, Python, and OpenSSL versions. It removes Ray and Nsight artifacts. It validates prefetched actors and prepares writable per-venv vLLM copies for runtime patching. It also updates the pinned NeMo-RL reference. ChangesRL image build and runtime patching
Sequence Diagram(s)sequenceDiagram
participant PublishStage
participant PrefetchedVllMEnvironment
participant RuntimeUser
PublishStage->>PrefetchedVllMEnvironment: Locate prefetched vLLM package
PrefetchedVllMEnvironment-->>PublishStage: Provide package contents
PublishStage->>RuntimeUser: Create writable per-venv vLLM copy
PublishStage->>RuntimeUser: Assign ownership and Gym fallback cache
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker/rl/README.md`:
- Around line 442-444: Update the cache explanation in README.md to distinguish
the shipped read-only /opt/uv_cache from temporary BuildKit cache mounts that do
not enter the image. Replace the earlier conflicting warmup-cache description
while preserving the existing guidance about Dockerfile.nmp-rl-training
redirecting UV_CACHE_DIR to a writable per-user cache.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5866f5d5-370e-4dcc-8d86-88d2c60a911c
📒 Files selected for processing (3)
docker/rl/Dockerfile.nmp-rl-basedocker/rl/Dockerfile.nmp-rl-trainingdocker/rl/README.md
|
e1e057b to
99584a3
Compare
anubhutivyas
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker/rl/Dockerfile.nmp-rl-base`:
- Around line 475-490: Update the vLLM privatization loop and its final
validation to verify every required worker venv, not merely that privatized is
greater than zero. Use the expected actor venv paths or an explicit manifest,
confirm each contains vllm and has been copied out of /opt/uv_cache before
publishing, and fail with a clear error if any required entry is missing or
remains symlinked.
In `@docker/rl/README.md`:
- Around line 462-464: Update the explanation near the NeMo-RL comparison to
distinguish the two fixes: private site-packages/vllm/ copies isolate each vLLM
venv’s py_executable and prevent shared ray_executor.py patch state, while
non-root ownership or permissions prevent EACCES when creating
<file>.patch_lock. Clarify that root access addresses permissions only and does
not resolve shared-state conflicts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0e6a3040-8564-44a2-b9d1-496770ac3b88
📒 Files selected for processing (2)
docker/rl/Dockerfile.nmp-rl-basedocker/rl/README.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docker/rl/Dockerfile.nmp-rl-base (2)
244-247: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-693)
Reachability: Internal · Exploitability: Theoretical
Make both CVE-pruning steps fail closed.
Remove
|| truefrom both cleanup commands and add explicit checks that no matching Ray JAR or Nsight artifact remains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker/rl/Dockerfile.nmp-rl-base` around lines 244 - 247, The CVE-pruning cleanup steps currently ignore failures and do not verify removal. In docker/rl/Dockerfile.nmp-rl-base lines 244-247, update the Ray jars cleanup to remove “|| true” and explicitly fail if any matching artifact remains; apply the same fail-closed cleanup and post-removal check to the Nsight artifact step at lines 427-437.
480-485: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSecurity Misconfiguration (CWE-732): Incorrect Permission Assignment for Critical Resource
Reachability: External
Do not expose patched
vllm/to all UID-1000 processes.Each
/opt/ray_venvsvLLM copy is writable by UID 1000. Colocated user-authored environment code uses the same UID and can replacevllm/v1/executor/ray_executor.pybefore a worker imports it. Use a separate worker UID or container, or isolate user-authored environment code before worker startup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker/rl/Dockerfile.nmp-rl-base` around lines 480 - 485, Update the vLLM privatization flow around the cp, chmod, and chown commands so patched vllm files are not writable or replaceable by colocated user-authored code running as UID 1000. Isolate the worker-owned vLLM copy using a separate worker UID or container, or ensure user-authored environment code is isolated before worker startup; preserve worker access to the patched files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docker/rl/Dockerfile.nmp-rl-base`:
- Around line 244-247: The CVE-pruning cleanup steps currently ignore failures
and do not verify removal. In docker/rl/Dockerfile.nmp-rl-base lines 244-247,
update the Ray jars cleanup to remove “|| true” and explicitly fail if any
matching artifact remains; apply the same fail-closed cleanup and post-removal
check to the Nsight artifact step at lines 427-437.
- Around line 480-485: Update the vLLM privatization flow around the cp, chmod,
and chown commands so patched vllm files are not writable or replaceable by
colocated user-authored code running as UID 1000. Isolate the worker-owned vLLM
copy using a separate worker UID or container, or ensure user-authored
environment code is isolated before worker startup; preserve worker access to
the patched files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 24858d39-f224-47ce-a77b-0fcdca6a607b
📒 Files selected for processing (2)
docker-bake.hcldocker/rl/Dockerfile.nmp-rl-base
9bdad24 to
3cc718b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Signed-off-by: anubhutiv <anubhutiv@nvidia.com>
3cc718b to
7704fd0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 participants
CVEs:
Bug fix:
GRPO fails with error
It is because,
UV_LINK_MODE=symlink, so all three vLLM venvs symlink to the same physical copy ofray_executor.pyin /opt/uv_cache. And the thing RL writes into that file is a per-venv interpreter path. So even if we fixed permissions, two venvs still can't share one file, the second worker would stomp the first. No amount of chown fixes that.site-packages/vllm/, owned by uid 1000. Everything else (torch, the CUDA libs) stays symlinked and shared.Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Chores
Documentation