fix(shim): remove rpath which segfault on PIE glibc binary#1016
Open
xhebox wants to merge 2 commits into
Open
Conversation
📦 BoxLite review — couldn't completepowered by BoxLite |
📝 WalkthroughWalkthroughThe C SDK release workflow now prepares symbol-fixed static archives through ChangesC SDK distribution and linking
Seatbelt environment sanitization
CLI
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
DorianZheng
added
the
e2e-local
![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/boxlite/src/jailer/sandbox/seatbelt.rs (1)
389-430: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the test exercise inherited-environment clearing.
SHOULD_NOT_SURVIVEis set on the discarded command, so it would disappear even withoutenv_clear(). Test the rewritten command with a known process-level variable and an allowlisted variable to verify both filtering and preservation.🤖 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/boxlite/src/jailer/sandbox/seatbelt.rs` around lines 389 - 430, Update test_apply_sanitizes_env to set SHOULD_NOT_SURVIVE through the process environment rather than only on the discarded Command, and add a process-level allowlisted variable to verify it is preserved. Keep assertions covering removal of the arbitrary variable and retention of the allowlisted variable after SeatbeltSandbox::apply.
🤖 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 `@docs/reference/c/README.md`:
- Around line 1178-1190: Update the shared-library CMake example following
target_link_libraries in the “Shared library” section to document running the
built executable with the appropriate platform-specific DYLD_LIBRARY_PATH or
LD_LIBRARY_PATH pointing to BOXLITE_LIB_DIR, matching the direct-compilation
example’s runtime setup.
In `@examples/c/README.md`:
- Around line 17-18: Use the prepared-archive workflow for all local C examples:
in examples/c/README.md lines 17-18, replace the raw cargo build prerequisite
with make dist:c or explicitly run fix-go-symbols.sh afterward; in
examples/c/CMakeLists.txt lines 14-15, update the missing-library guidance to
invoke the same prepared-archive workflow.
In `@src/boxlite/src/jailer/sandbox/seatbelt.rs`:
- Around line 119-124: Update the sandbox command environment setup around
shim_process_env() to explicitly restore the validated runtime DYLD_LIBRARY_PATH
produced by configure_library_env() after env_clear(). Keep the existing fixed
environment and shim variables, and do not inherit the host’s DYLD_LIBRARY_PATH.
In `@src/boxlite/src/util/mod.rs`:
- Around line 124-127: Update the library-path setup around std::env::join_paths
to handle its error explicitly instead of calling expect. When joining lib_dirs
fails because an entry contains the platform separator, skip setting
DYLD_LIBRARY_PATH or propagate a clear configuration error, while preserving the
existing environment assignment and debug logging for successful joins.
---
Nitpick comments:
In `@src/boxlite/src/jailer/sandbox/seatbelt.rs`:
- Around line 389-430: Update test_apply_sanitizes_env to set SHOULD_NOT_SURVIVE
through the process environment rather than only on the discarded Command, and
add a process-level allowlisted variable to verify it is preserved. Keep
assertions covering removal of the arbitrary variable and retention of the
allowlisted variable after SeatbeltSandbox::apply.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f06af75-05c5-46ad-8e1e-ff8efbd9fa71
📒 Files selected for processing (12)
.github/workflows/build-c.ymlapps/dashboard/src/lib/quickstart/interfaces.jsondocs/getting-started/quickstart-c.mddocs/reference/c/README.mdexamples/c/CMakeLists.txtexamples/c/README.mdmake/dev.mkmake/dist.mksdks/c/README.mdsrc/boxlite/src/jailer/sandbox/seatbelt.rssrc/boxlite/src/util/mod.rssrc/shim/build.rs
💤 Files with no reviewable changes (1)
- src/shim/build.rs
DorianZheng
added a commit
that referenced
this pull request
Jul 22, 2026
## Why actions/checkout v5.1.0 (2026-07-20) refuses to fetch fork PR code in `pull_request_target` workflows unless the step opts in via `allow-unsafe-pr-checkout: true`. Every fork PR now fails e2e at the Checkout step (first hit: #1016, run 29919073197): > Refusing to check out fork pull request code from a 'pull_request_target' workflow. … set 'allow-unsafe-pr-checkout: true' on the actions/checkout step. ## What Opt the e2e-tests Checkout step in. The mitigations the gate asks for already exist in this workflow: fork runs are gated on the maintainer-applied `e2e-local` label pinned to the approved head SHA (re-label per push), the job holds no secrets, and the token is `contents: read` with `persist-credentials: false`. Note: actionlint ≤1.7.11 flags the input as unknown — stale bundled schema; the input is defined in action.yml at the SHA `v5` resolves to. **After merge:** remove and re-add the `e2e-local` label on #1016 to re-run e2e against the fixed workflow definition (pull_request_target reads the workflow from main, so the fix takes effect only once merged). https://claude.ai/code/session_019NzYgutK3RsJupnSe16M1w Co-authored-by: tester <[email protected]>
G4614
enabled auto-merge
July 22, 2026 12:54
Contributor
|
can |
DorianZheng
added
e2e-local
DorianZheng
reviewed
Jul 22, 2026
| # Fix Go runtime symbol conflicts in the static library | ||
| bash scripts/build/fix-go-symbols.sh target/release/libboxlite.a | ||
| # Build and prepare the C FFI libraries through the local entrypoint. | ||
| make dist:c |
DorianZheng
reviewed
Jul 22, 2026
| let mut new_cmd = Command::new(sandbox_cmd); | ||
| new_cmd.args(sandbox_args); | ||
| new_cmd.env_clear(); | ||
| new_cmd.env("PATH", "/usr/bin:/bin:/usr/sbin:/sbin"); |
auto-merge was automatically disabled
July 22, 2026 15:20
Head branch was pushed to by a user without write access
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 (1)
src/cli/src/cli.rs (1)
785-798: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--rmneeds an explicit--detachpolicy.ManagementFlags::apply_toforcesauto_delete = Some(0), andrun()only callsrt.removeon the foreground path, soboxlite run --rm -d …leaves the box behind (src/cli/src/cli.rs#L785-L798,src/cli/src/commands/run.rs#L78-L123). Reject that combination or add a detached cleanup path.🤖 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/cli/src/cli.rs` around lines 785 - 798, The --rm option currently leaves detached boxes behind because ManagementFlags::apply_to sets auto_delete without enforcing a detach policy, while run() only removes foreground boxes. Update src/cli/src/cli.rs lines 785-798 and src/cli/src/commands/run.rs lines 157-158 to either reject the --rm --detach combination or add detached cleanup, ensuring every accepted --rm invocation removes the box.
🤖 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 `@src/cli/src/cli.rs`:
- Around line 785-798: The --rm option currently leaves detached boxes behind
because ManagementFlags::apply_to sets auto_delete without enforcing a detach
policy, while run() only removes foreground boxes. Update src/cli/src/cli.rs
lines 785-798 and src/cli/src/commands/run.rs lines 157-158 to either reject the
--rm --detach combination or add detached cleanup, ensuring every accepted --rm
invocation removes the box.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: acfe82c1-0acc-4e06-8af0-5191b53e00d1
📒 Files selected for processing (3)
src/cli/src/cli.rssrc/cli/src/commands/run.rssrc/cli/tests/run.rs
This was referenced Jul 22, 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
Changes
How to verify
Risks / rollout
Summary by CodeRabbit
--rmcontainer cleanup to ensure boxes/directories are removed deterministically, even on non-zero exits.--rmso it no longer affects lifecycle auto-deletion behavior.--rmcleanup validation.