feat: fallback to /proc/1/environ for missing environment variables - #235
feat: fallback to /proc/1/environ for missing environment variables#235shellyco-code wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shellyco-code The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @shellyco-code! It looks like this is your first PR to Project-HAMi/HAMi-core 🎉 |
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (6)
📝 WalkthroughWalkthroughAdds ChangesEnvironment Lookup Fallback
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RuntimeCaller
participant vgpu_getenv
participant ProcessEnvironment
participant ProcEnviron
RuntimeCaller->>vgpu_getenv: request configuration variable
vgpu_getenv->>ProcessEnvironment: check getenv
alt variable is unset
vgpu_getenv->>ProcEnviron: read /proc/1/environ
ProcEnviron-->>vgpu_getenv: matching value or NULL
else variable is set
ProcessEnvironment-->>vgpu_getenv: value
end
vgpu_getenv-->>RuntimeCaller: return configuration value
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
3324513 to
d745c51
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/include/utils.h (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the fallback and pointer-lifetime contract.
This public API should state that
getenv()takes precedence,/proc/1/environis used as a fallback, and fallback results are thread-local and overwritten by later lookups. Document the trust boundary before callers treat it like a normal environment pointer.🤖 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 `@src/include/utils.h` at line 8, Document the public vgpu_getenv declaration with its lookup and lifetime contract: use getenv() first, fall back to /proc/1/environ, and state that fallback results are thread-local and overwritten by subsequent lookups. Clarify that callers must treat the returned pointer according to this trust boundary.
🤖 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 `@src/multiprocess/multiprocess_memory_limit.c`:
- Around line 1143-1144: Update the CUDA_TASK_PRIORITY_ENV handling to call
vgpu_getenv once, cache the returned value, and parse it with strtol. Validate
errno, endptr, and the supported priority range before assigning the result to
region->priority; leave the existing priority unchanged for invalid values or
when the variable is absent.
In `@src/utils.c`:
- Around line 29-31: Update the environment-entry parsing around the
thread-local result buffer and single fread so complete values are read without
silent truncation or omission; dynamically handle entries exceeding the fixed
buffers or explicitly reject them, ensuring CUDA_REDIRECT and
MULTIPROCESS_SHARED_REGION_CACHE_ENV are never returned as altered paths. Apply
the same correction to the related logic at the additional occurrence.
- Around line 26-27: Update vgpu_getenv() to read the current process
environment from /proc/self/environ instead of /proc/1/environ. Before copying
any fallback variable into the 256-byte result buffer, detect oversized values
and handle them safely without silent truncation.
- Line 224: Update the logging around the CUDA_VISIBLE_DEVICES lookup in the
relevant utility flow to avoid passing a NULL value to the "%s" formatter:
retain the parsed environment value and log it with a null-safe placeholder when
unset, while preserving the existing behavior when the variable is configured.
---
Nitpick comments:
In `@src/include/utils.h`:
- Line 8: Document the public vgpu_getenv declaration with its lookup and
lifetime contract: use getenv() first, fall back to /proc/1/environ, and state
that fallback results are thread-local and overwritten by subsequent lookups.
Clarify that callers must treat the returned pointer according to this trust
boundary.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 532e8e3e-4c13-4951-b131-27edbeb70a38
📒 Files selected for processing (6)
src/include/utils.hsrc/libvgpu.csrc/log_utils.csrc/multiprocess/multiprocess_memory_limit.csrc/multiprocess/shrreg_tool.csrc/utils.c
This resolves an issue where child processes or SSH-started processes lose the inherited environment variables used to pass GPU isolation limits (such as CGROUP_MEMORY_MAX). By falling back to parsing /proc/1/environ when getenv() fails, libvgpu.so correctly reads the configuration initially passed into the pod by the Mutating Webhook, preserving memory isolation across all processes. Signed-off-by: shellyco-code <shellychahar57@gmail.com>
d745c51 to
a32d70a
Compare
1 participant
Description
This PR fixes the GPU memory isolation bypass vulnerability that occurs when child processes are spawned (e.g., via
env -i) or when a user establishes a new SSH session into a HAMi-managed pod.The Problem:
Currently, HAMi enforces GPU isolation by injecting configuration (like
CGROUP_MEMORY_MAX,CUDA_VISIBLE_DEVICES, etc.) via environment variables into the pod's main process. When an SSH session is initiated or environment variables are purged,libvgpu.sofails to read these configurations throughgetenv(), effectively bypassing all limits.The Solution:
This PR introduces a thread-safe
vgpu_getenvhelper function that acts as a drop-in replacement forgetenv().If standard
getenv()returnsNULL,vgpu_getenvfalls back to parsing/proc/1/environ(the environment of the container's initialization process). Because Kubernetes always retains the webhook-injected environment variables in PID 1,libvgpu.socan securely recover its configuration limits regardless of the user's local shell environment.Changes Made
vgpu_getenv()implementation insrc/utils.c.getenv()acrosslibvgpu.c,utils.c,log_utils.c,multiprocess_memory_limit.c, andshrreg_tool.c.Related Issues
Fixes Project-HAMi/HAMi#2125
Testing
env -i /bin/bashfollowed by a GPU workload respects the original memory limits.Checklist
Summary by CodeRabbit
New Features
Bug Fixes