rsandbox: RPython-style compile-time sandbox for pyre (#285) - #304
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:
WalkthroughAdds the ChangesPyre Sandbox Infrastructure and Interpreter Integration
Sequence Diagram(s)sequenceDiagram
participant pyre_interact as pyre interact
participant PyPySandboxedProc as PyPySandboxedProc
participant SandboxPolicy as SandboxPolicy
participant sandboxed_pyre as sandboxed pyre
pyre_interact->>PyPySandboxedProc: run_interact(...)
PyPySandboxedProc->>sandboxed_pyre: spawn child with piped stdio
PyPySandboxedProc->>SandboxPolicy: handle_until_return_ticked(...)
sandboxed_pyre->>SandboxPolicy: marshalled request
SandboxPolicy-->>sandboxed_pyre: reply or exception
sandboxed_pyre-->>PyPySandboxedProc: exit code
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a58cf5d). 1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptationsNone. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/8718ee023c95710e8b6d2543eefd2047de3dea96/pyrex/src/lib.rs#L247-L248
When the sandboxed child is invoked with a script filename instead of -c, this filter is installed before the RunMode::Script arm calls std::fs::read_to_string below; on Linux that needs open/openat, which are not allowlisted, so pyre interact <sandbox> /tmp/foo.py is killed before user code runs. If seccomp is disabled for debugging, the same path reads the controller's real filesystem instead of the virtual FS, so script loading needs to go through the seam/controller before executing.
https://github.com/youknowone/pyre/blob/8718ee023c95710e8b6d2543eefd2047de3dea96/pyre-interpreter/src/importing.rs#L806-L808
In sandbox mode this reads PYRE_STDLIB from the child's real process environment, but PyPySandboxedProc::new starts the child with env_clear() and the policy's virtual environment is Vec::new(), while --lib only mounts the directory at /bin/lib. As a result pyre interact --lib <dir> ... never adds the mounted stdlib to sys.path, so pure-Python stdlib imports still fail despite the advertised mount.
https://github.com/youknowone/pyre/blob/8718ee023c95710e8b6d2543eefd2047de3dea96/pyre-sandbox/src/seccomp.rs#L83-L87
Allowing getcwd lets the sandboxed child successfully run the direct std::env::current_dir() calls used to seed sys.path for -c, -m, and the REPL, so untrusted code can inspect sys.path and learn the trusted controller's real working directory instead of the virtual /tmp. This host path is outside the mediated ll_os_getcwd policy, so cwd discovery should be virtualized or the launcher should avoid those direct calls under sandbox.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 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 `@pyre/pyre-interpreter/src/importing.rs`:
- Around line 486-510: `sys.builtin_module_names` is still listing modules that
are skipped in sandbox mode, so the builtin module inventory is out of sync with
`importing.rs`. Update `module/sys/vm.rs` so the entries for `_socket`, `fcntl`,
`select`, and the other omitted builtins use the same `#[cfg(not(feature =
"sandbox"))]` gating as the registration block, or otherwise derive the tuple
from the actual registered modules. Use the existing `pyre_install_module!`
registrations in `importing.rs` as the source of truth when adjusting the
`sys.builtin_module_names` list.
- Around line 802-809: The sandbox-only `PYRE_STDLIB` lookup in `importing.rs`
is bypassing the env seam and reading the host environment directly. Update the
`PYRE_STDLIB` path in the sandbox branch to use `host_seam::ops::getenv` instead
of `host_os::var`, and make sure the controller seeds the same value into the
virtual environment used by `SourceProvider` so the lookup succeeds
consistently.
In `@pyre/pyre-interpreter/src/module/mod.rs`:
- Around line 46-49: The `_random` module is still being exposed in sandbox
builds even though its `seed(None)` path depends on host entropy sources. Gate
`_random` out of the sandbox surface the same way other host-backed modules are
handled by updating the `mod.rs` module declarations and the corresponding
registration path in `importing.rs`. If sandbox access is still needed, route
seeding through a `host_seam`-mediated API instead of calling host `urandom(8)`
or `SystemTime::now()` directly.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 3198-3238: The sandbox stub list and capability advertisement are
out of sync: the sandbox override in interp_posix.rs disables fd-relative
operations like fchdir, fchmod, fchown, fpathconf, fstatvfs, ftruncate,
futimens, and futimes, but _have_functions still reports them as available.
Update the _have_functions set used for sandbox builds to exclude any functions
that are stubbed out so os.py only sees operations that actually work under
sandbox.
- Around line 3229-3230: Hide the sandbox-bypassing DirEntry APIs in the POSIX
interpreter override as well: the current sandbox override only stubs scandir,
but DirEntry is still exported and its methods still reach host_fs::metadata and
symlink_metadata. Update the interp_posix registration path around DirEntry so
it is either replaced with a sandbox-safe stub or gated behind not(feature =
"sandbox"), including any DirEntry method bindings that expose host filesystem
inspection.
In `@pyre/pyre-interpreter/src/module/signal/interp_signal.rs`:
- Around line 1029-1036: The host-backed signal APIs in interp_signal need the
same sandbox protection as pidfd_send_signal, because raise_signal, alarm,
setitimer, pthread_kill, and pthread_sigmask are still enabled under plain
host_env and can bypass the sandbox. Update the cfgs around the relevant match
arms or helper paths in interp_signal to require not(feature = "sandbox") as
well, or add a sandbox-specific stub override for the unsupported signal/timer
surface so sandbox+host_env builds do not reach direct host syscalls.
In `@pyre/pyre-interpreter/src/module/time/interp_time.rs`:
- Around line 167-174: The sandboxed sleep path in interp_time::do_sleep
currently forwards the full user-controlled duration to
crate::host_seam::ops::sleep, which can block the trusted controller
indefinitely. Update the sandbox branch to bound controller-side sleeping by
capping or slicing the requested duration into smaller chunks so the controller
can remain responsive and the watchdog can still enforce timeouts. Keep the fix
localized to do_sleep and preserve the existing error handling via
host_seam::seam_os_err and w_none return behavior.
In `@pyre/pyre-sandbox/src/client.rs`:
- Around line 37-40: The untrusted client is writing diagnostics directly to the
child’s raw stderr path, which bypasses the controller. Update the
`not_implemented_stub` flow in `client.rs` so it does not write to `STDERR_FD`
or otherwise emit raw child output; instead, return the error to the trusted
caller or route diagnostics through the controller-side logging path. Keep the
protocol constants (`STDIN_FD`, `STDOUT_FD`, `STDERR_FD`) untouched and adjust
the affected `not_implemented_stub`/related sandbox error handling to preserve
diagnostics without direct child writes.
- Around line 57-87: Both FdNeedMore::need_more and writeall_not_sandboxed
currently treat any read/write return value <= 0 as SandboxError::Io, which
causes interrupted syscalls to fail spuriously. Update these raw fd I/O paths to
detect libc::EINTR and retry the syscall instead of erroring, while keeping the
existing SandboxError::Io behavior for other failures and preserving the
partial-write loop in writeall_not_sandboxed.
In `@pyre/pyre-sandbox/src/controller.rs`:
- Around line 211-214: The child shutdown flow in Controller::wait currently
reaps the process before stopping the watchdog, leaving a race where the
watchdog can still signal a reused PID. Update the logic in the controller
cleanup path to stop or tear down the watchdog before calling self.child.wait(),
or replace the raw PID tracking with a non-reusable handle such as pidfd so the
watchdog cannot target a different process. Keep the fix localized to the
watchdog management around self.child.wait() and watchdog.stop().
- Around line 147-153: The child process setup in `Command::new(...).spawn()` is
not sanitizing inherited file descriptors, so leaked non-`CLOEXEC` fds from the
controller can remain open in the sandbox. Add fd-cleanup in the `controller.rs`
child launch path before `spawn()` so only stdio stays open, using the existing
process setup around `Command`, `stdin`, and `stdout` to close or mark all other
descriptors for closure before `exec`.
In `@pyre/pyre-sandbox/src/rmarshal.rs`:
- Around line 429-440: The tuple/list parsing in rmarshal::read_value accepts
peer-supplied lengths and passes them straight into Vec::with_capacity, so add a
protocol-level maximum before allocating. Update the TYPE_TUPLE | TYPE_LIST
branch (and the string branch if applicable) to validate length after the
negative check and return SandboxError::Protocol for oversized values, using the
existing readlong/readstr flow as the anchor point.
In `@pyre/pyre-sandbox/src/sandlib.rs`:
- Around line 213-217: Apply the existing MAX_READ cap to the stdin/console read
path in Sandlib’s read handling, not just the virtual file branch. In the logic
that chooses between read_line and read_upto for console.input, clamp the
requested size before passing it into those helpers so fd 0 cannot request an
unbounded read. Use the surrounding read dispatch in sandlib.rs and the MAX_READ
constant to keep console reads consistent with virtual file reads.
- Around line 265-275: The seek handling in the lseek path currently casts the
offset for the SEEK_SET branch in the sandlib seek logic without validating it
first, which can wrap negative values into large unsigned offsets. Update the
match in the seek implementation to reject negative positions before
constructing SeekFrom::Start, returning SandboxError::Os(libc::EINVAL) for
invalid SEEK_SET inputs while leaving SEEK_CUR and SEEK_END behavior unchanged.
Use the existing lseek/seek handling code around the whence and newpos logic to
place the validation in the right branch.
- Around line 418-428: Malformed protocol input is being treated as a normal
end-of-stream in the message loop around at_message_boundary_eof, load_string,
and load_value. Update that parsing path so unexpected EOF or decode failures
return an io::Error with UnexpectedEof or InvalidData instead of breaking and
yielding Ok(()), and keep clean termination only for the explicit valid boundary
case. Use the existing loop that reads fnname and args in sandlib.rs to locate
the change.
In `@pyre/pyre-sandbox/src/seccomp.rs`:
- Around line 77-87: The seccomp allowlist in seccomp:: filter still permits the
real getcwd syscall, which can leak the host cwd before untrusted code runs. Fix
this by either removing SYS_getcwd from the syscall list or ensuring the
sandboxed child is chdir’d to /tmp before execution in the relevant child setup
path so direct getcwd calls cannot expose the parent cwd.
In `@pyre/pyre-sandbox/tests/e2e_interact.rs`:
- Around line 108-119: The write-escape probe in e2e_interact currently uses a
fixed host path, which can leave a stray file on the controller machine if the
sandbox fails open. Update the probe in interact/e2e_interact to use a unique
sentinel filename instead of /tmp/evil.txt, and make sure the test cleans up any
host-side artifact after the interaction completes. Keep the existing
BLOCKED/WROTE assertions and the filesystem check, but ensure the sentinel path
is easy to locate in the test code.
In `@pyre/pyrex/src/lib.rs`:
- Line 32: The timeout handling in the `pyre interact` argument path currently
accepts `timeout: Option<f64>` without validating it before converting to
`Duration`, which can lead to panics for negative, NaN, or oversized values.
Update the `timeout` parsing/handling in `lib.rs` so invalid values are rejected
up front with a usage error, or switch the `timeout` field and conversion flow
to use `Duration` directly. Keep the fix localized around the `timeout` option
used by the interact command so the conversion path is always safe.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f4838113-f0f3-4736-9b3b-ebdc6b2a89db
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/pyre-ci.ymlCargo.tomlpyre/pyre-interpreter/Cargo.tomlpyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/host_seam.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/_locale/interp_locale.rspyre/pyre-interpreter/src/module/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/time/interp_time.rspyre/pyre-interpreter/src/module/time/mod.rspyre/pyre-interpreter/src/sandbox/mod.rspyre/pyre-sandbox/Cargo.tomlpyre/pyre-sandbox/src/client.rspyre/pyre-sandbox/src/controller.rspyre/pyre-sandbox/src/lib.rspyre/pyre-sandbox/src/protocol.rspyre/pyre-sandbox/src/rmarshal.rspyre/pyre-sandbox/src/sandlib.rspyre/pyre-sandbox/src/seccomp.rspyre/pyre-sandbox/src/vfs.rspyre/pyre-sandbox/tests/e2e_interact.rspyre/pyrex/Cargo.tomlpyre/pyrex/src/lib.rs
💤 Files with no reviewable changes (1)
- pyre/pyre-interpreter/src/sandbox/mod.rs
| #[allow(non_snake_case)] | ||
| pub mod _random; | ||
| #[allow(non_snake_case)] | ||
| #[cfg(not(feature = "sandbox"))] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Do not leave _random on the sandbox surface while it seeds from host entropy.
_random is still compiled/registered for sandbox, but its seed(None) path calls host urandom(8) and falls back to SystemTime::now(). That bypasses host_seam and can either leak host state or get killed by seccomp. Gate it out like the other host-backed modules or port seeding through a mediated seam.
Minimal gate-out direction
#[allow(non_snake_case)]
+#[cfg(not(feature = "sandbox"))]
pub mod _random;Also gate its registration in importing.rs:
- pyre_install_module!(_random);
+ #[cfg(not(feature = "sandbox"))]
+ pyre_install_module!(_random);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[allow(non_snake_case)] | |
| pub mod _random; | |
| #[allow(non_snake_case)] | |
| #[cfg(not(feature = "sandbox"))] | |
| #[allow(non_snake_case)] | |
| #[cfg(not(feature = "sandbox"))] | |
| pub mod _random; | |
| #[allow(non_snake_case)] | |
| #[cfg(not(feature = "sandbox"))] |
🤖 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 `@pyre/pyre-interpreter/src/module/mod.rs` around lines 46 - 49, The `_random`
module is still being exposed in sandbox builds even though its `seed(None)`
path depends on host entropy sources. Gate `_random` out of the sandbox surface
the same way other host-backed modules are handled by updating the `mod.rs`
module declarations and the corresponding registration path in `importing.rs`.
If sandbox access is still needed, route seeding through a `host_seam`-mediated
API instead of calling host `urandom(8)` or `SystemTime::now()` directly.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/18f81112a83ca4865af6a3b0dde70b2250ec33af/pyrex/src/lib.rs#L247-L248
When the child is invoked with a script path (for example pyre interact <child> /tmp/app.py), this filter is installed before the RunMode::Script arm, which still calls std::fs::read_to_string at pyre/pyrex/src/lib.rs:281. On Linux that direct openat is not in the allowlist, so the sandboxed child is killed; with PYRE_SANDBOX_NO_SECCOMP, the same path reads the host filesystem instead of the controller VFS. Please load script sources through the seam/controller before executing them.
https://github.com/youknowone/pyre/blob/18f81112a83ca4865af6a3b0dde70b2250ec33af/pyre-sandbox/src/controller.rs#L145
When --lib is supplied, build_virtual_root mounts it at /bin/lib, but the policy is created with an empty virtual_env. In sandbox, detect_stdlib_path() only reads PYRE_STDLIB, and the child is spawned with env_clear(), so the mounted stdlib is never added to sys.path; pyre interact --lib ... <child> -c 'import os' still fails. Seed the virtual environment with PYRE_STDLIB=/bin/lib or add that path directly when lib_root is present.
https://github.com/youknowone/pyre/blob/18f81112a83ca4865af6a3b0dde70b2250ec33af/pyre-sandbox/src/sandlib.rs#L266
For sandboxed posix.lseek(fd, -1, SEEK_SET), this cast turns -1 into u64::MAX, so Cursor/File can accept a huge absolute position and Python sees success instead of OSError(EINVAL), with later reads behaving like EOF. Check pos < 0 for SEEK_SET before constructing SeekFrom::Start.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/module/mod.rs (1)
24-94: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
sys.builtin_module_namesaligned with sandbox-gated modules.These modules are now compiled out under
sandbox, butsys.builtin_module_namesstill advertises several of them. Mirror the samecfg(not(feature = "sandbox"))conditions in the sys registration list so introspection does not report unavailable builtins.🤖 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 `@pyre/pyre-interpreter/src/module/mod.rs` around lines 24 - 94, `sys.builtin_module_names` is still listing modules that are now excluded by the sandbox build, so update the builtin registration in the `mod`/`sys` module setup to use the same `cfg(not(feature = "sandbox"))` guards as the `pub mod` declarations. Align the `builtin_module_names` entries with the sandbox-gated modules such as `_ctypes`, `_multiprocessing`, `_posixsubprocess`, `_socket`, `fcntl`, `resource`, `select`, `syslog`, and any other conditionally compiled modules so introspection only reports modules actually available in this build.
♻️ Duplicate comments (3)
pyre/pyre-interpreter/src/module/signal/interp_signal.rs (1)
1029-1036: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftApply the sandbox stub consistently to the signal surface.
This only stubs
pidfd_send_signal; adjacentraise_signal,alarm,setitimer,sigwait,pthread_kill, andpthread_sigmaskstill compile direct host signal/syscall paths underhost_env + sandbox.🤖 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 `@pyre/pyre-interpreter/src/module/signal/interp_signal.rs` around lines 1029 - 1036, The sandbox stub is only applied in the `pidfd_send_signal` path, while nearby signal entry points still use host signal/syscall logic under `host_env + sandbox`. Update the `interp_signal` implementation to route `raise_signal`, `alarm`, `setitimer`, `sigwait`, `pthread_kill`, and `pthread_sigmask` through the same `crate::host_seam::stub(...)` pattern used for `pidfd_send_signal`, so the entire signal surface is consistently stubbed in sandbox builds.pyre/pyre-interpreter/src/importing.rs (1)
71-71: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRoute sandbox
PYRE_STDLIBthrough the seam.
host_osis now gated out of sandbox builds, while Line 808 still uses it; even if reintroduced, this bypasses the controller’s virtual environment. ReadPYRE_STDLIBthroughhost_seam::ops::getenv.Proposed fix
#[cfg(feature = "sandbox")] { - return host_os::var("PYRE_STDLIB").ok().map(PathBuf::from); + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + return crate::host_seam::ops::getenv(b"PYRE_STDLIB") + .ok() + .flatten() + .map(|value| PathBuf::from(OsString::from_vec(value))); }Also applies to: 806-808
🤖 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 `@pyre/pyre-interpreter/src/importing.rs` at line 71, The sandbox path in importing.rs still reads PYRE_STDLIB via host_os, which bypasses the controller seam and can break sandbox builds. Update the logic in the import/bootstrap flow that uses PYRE_STDLIB to fetch it through host_seam::ops::getenv instead, so the sandboxed environment is respected. Keep the change localized around the existing host_os/PYRE_STDLIB handling in the import initialization code.pyre/pyre-interpreter/src/module/time/interp_time.rs (1)
167-174: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not forward unbounded sleeps to the controller.
This still sends the full user-controlled duration to
do_sleep, which blocks the trusted controller in onestd::thread::sleep. Cap or slice controller-side sleeps so sandbox timeouts remain enforceable.🤖 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 `@pyre/pyre-interpreter/src/module/time/interp_time.rs` around lines 167 - 174, The sandbox sleep path in interp_time::do_sleep is still forwarding the full user-controlled duration to the trusted controller, which can block it in one long std::thread::sleep. Update the sandbox branch to cap or chunk the requested sleep before calling crate::host_seam::ops::sleep, and keep looping in smaller slices so controller-side timeouts remain enforceable. Use the existing do_sleep function and sandbox feature-gated branch as the place to apply the fix.
🤖 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 `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 4812-4818: The tuple() and list() constructors still accept extra
positional arguments because they only check args.is_empty() and then use
args[0], so calls like tuple(1, 2) and list(1, 2) are silently accepted. Update
the tuple and list builtin handling in builtins.rs to validate that exactly one
positional argument is allowed after split_builtin_kwargs, and return a
TypeError when args.len() > 1 before any iteration or conversion logic runs. Use
the existing tuple() and list() constructor branches to keep the behavior
consistent for both builtins.
- Around line 8434-8437: The complex() builtin currently only resolves the first
two positional arguments via split_builtin_kwargs and resolve_pos_or_kw, so a
third positional argument is silently ignored. Update the complex argument
handling in builtins.rs to explicitly reject any extra positional arguments
after the real and imag slots, using the existing complex() validation path so
complex(a, b, c) raises an error instead of being accepted.
In `@pyre/pyre-interpreter/src/lib.rs`:
- Around line 28-31: The `host_seam` module is only available on Unix, but the
standalone `sandbox` feature can still enable code paths that reference
`crate::host_seam`. Update the gating in `lib.rs` and any `host_seam`
imports/calls so sandboxed code only compiles with `all(unix, feature =
"sandbox")`, or make the `sandbox` feature Unix-only. Use the `host_seam` module
and all related sandbox entry points as the symbols to locate and align the
feature flags consistently.
In `@pyre/pyre-interpreter/src/module/_locale/interp_locale.rs`:
- Around line 464-481: The sandbox override in interp_locale currently stubs
only setlocale, localeconv, and nl_langinfo, but strcoll and strxfrm still point
to host-backed locale functions and can leak host state. Update the sandbox
block in interp_locale to also replace strcoll and strxfrm with the same
locale_unavailable stub via dict_storage_store/make_builtin_function so all
locale-sensitive APIs are fully isolated under the sandbox feature.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 517-531: Validate the requested read size in interp_posix::read
before converting it to usize/i64. The current handling of args[1] via
pyre_object::w_int_get_value can wrap negative Python counts into a large usize
and then into a negative i64 under the sandbox path, so update the read-size
handling to keep the signed value, reject negatives up front, and only cast
after validation in both the libc::read and crate::host_seam::ops::read
branches.
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 1059-1074: The sandbox write path in the builtin write
implementation is ignoring failures from host_seam::ops::write, so
sys.stdout.write and sys.stderr.write always appear successful. Update the write
handling in the crate::make_builtin_function("write", ...) branch (and the
matching stderr path) to capture the result from crate::host_seam::ops::write
and return/propagate an error when it fails instead of discarding it with let _.
In `@pyre/pyre-sandbox/src/controller.rs`:
- Around line 147-153: The sandboxed child created in controller.rs is
inheriting raw stderr because Command::new(...).spawn() only sets stdin/stdout,
so update the child setup in the spawn path to explicitly configure stderr as
well. Use the existing Command builder in the controller logic to either route
stderr to a bounded trusted drain or set it to Stdio::null(), ensuring the child
cannot write directly to the host console outside the controller protocol.
In `@pyre/pyre-sandbox/src/sandlib.rs`:
- Around line 373-377: The do_sleep implementation in sandlib::do_sleep
currently lets unbounded values from ll_time_sleep reach
std::time::Duration::from_secs_f64 and also blocks the trusted controller thread
while sleeping. Update do_sleep to validate and clamp the requested seconds
before sleeping by using Duration::try_from_secs_f64 and enforcing a small
maximum delay or the request timeout, and keep the fix local to the do_sleep
path so oversized, infinite, or otherwise unsafe sleep values cannot panic or
stall the controller.
In `@pyre/pyrex/src/lib.rs`:
- Around line 53-56: The help text for the Subcommands section currently
advertises interact unconditionally even though the non-Unix path in the
dispatch logic rejects it; update the help output in lib.rs so interact is only
shown on Unix or is explicitly labeled Unix-only. Use the interact subcommand
entry and the Unix dispatch handling as the reference points, and gate the help
line with #[cfg(unix)] or add a clear Unix-only annotation to keep the CLI help
consistent with runtime behavior.
- Around line 337-345: The --verbose flag is currently ignored because
run_interact takes _verbose and never uses it, so the CLI behavior does not
match parse_interact() and usage(). Update run_interact to consume the verbose
flag and wire it into the controller/logging path, using the existing
run_interact and parse_interact symbols; if verbose should not affect behavior,
remove it from the accepted/documented CLI contract instead so usage() and the
argument parsing stay consistent.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/module/mod.rs`:
- Around line 24-94: `sys.builtin_module_names` is still listing modules that
are now excluded by the sandbox build, so update the builtin registration in the
`mod`/`sys` module setup to use the same `cfg(not(feature = "sandbox"))` guards
as the `pub mod` declarations. Align the `builtin_module_names` entries with the
sandbox-gated modules such as `_ctypes`, `_multiprocessing`, `_posixsubprocess`,
`_socket`, `fcntl`, `resource`, `select`, `syslog`, and any other conditionally
compiled modules so introspection only reports modules actually available in
this build.
---
Duplicate comments:
In `@pyre/pyre-interpreter/src/importing.rs`:
- Line 71: The sandbox path in importing.rs still reads PYRE_STDLIB via host_os,
which bypasses the controller seam and can break sandbox builds. Update the
logic in the import/bootstrap flow that uses PYRE_STDLIB to fetch it through
host_seam::ops::getenv instead, so the sandboxed environment is respected. Keep
the change localized around the existing host_os/PYRE_STDLIB handling in the
import initialization code.
In `@pyre/pyre-interpreter/src/module/signal/interp_signal.rs`:
- Around line 1029-1036: The sandbox stub is only applied in the
`pidfd_send_signal` path, while nearby signal entry points still use host
signal/syscall logic under `host_env + sandbox`. Update the `interp_signal`
implementation to route `raise_signal`, `alarm`, `setitimer`, `sigwait`,
`pthread_kill`, and `pthread_sigmask` through the same
`crate::host_seam::stub(...)` pattern used for `pidfd_send_signal`, so the
entire signal surface is consistently stubbed in sandbox builds.
In `@pyre/pyre-interpreter/src/module/time/interp_time.rs`:
- Around line 167-174: The sandbox sleep path in interp_time::do_sleep is still
forwarding the full user-controlled duration to the trusted controller, which
can block it in one long std::thread::sleep. Update the sandbox branch to cap or
chunk the requested sleep before calling crate::host_seam::ops::sleep, and keep
looping in smaller slices so controller-side timeouts remain enforceable. Use
the existing do_sleep function and sandbox feature-gated branch as the place to
apply the fix.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 97510bea-9663-498a-8b9e-c26527ff8cfd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/pyre-ci.ymlCargo.tomlpyre/pyre-interpreter/Cargo.tomlpyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/host_seam.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/_locale/interp_locale.rspyre/pyre-interpreter/src/module/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/time/interp_time.rspyre/pyre-interpreter/src/module/time/mod.rspyre/pyre-interpreter/src/sandbox/mod.rspyre/pyre-sandbox/Cargo.tomlpyre/pyre-sandbox/src/client.rspyre/pyre-sandbox/src/controller.rspyre/pyre-sandbox/src/lib.rspyre/pyre-sandbox/src/protocol.rspyre/pyre-sandbox/src/rmarshal.rspyre/pyre-sandbox/src/sandlib.rspyre/pyre-sandbox/src/seccomp.rspyre/pyre-sandbox/src/vfs.rspyre/pyre-sandbox/tests/e2e_interact.rspyre/pyrex/Cargo.tomlpyre/pyrex/src/lib.rs
💤 Files with no reviewable changes (1)
- pyre/pyre-interpreter/src/sandbox/mod.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/module/mod.rs (1)
24-94: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
sys.builtin_module_namesaligned with sandbox-gated modules.These modules are now compiled out under
sandbox, butsys.builtin_module_namesstill advertises several of them. Mirror the samecfg(not(feature = "sandbox"))conditions in the sys registration list so introspection does not report unavailable builtins.🤖 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 `@pyre/pyre-interpreter/src/module/mod.rs` around lines 24 - 94, `sys.builtin_module_names` is still listing modules that are now excluded by the sandbox build, so update the builtin registration in the `mod`/`sys` module setup to use the same `cfg(not(feature = "sandbox"))` guards as the `pub mod` declarations. Align the `builtin_module_names` entries with the sandbox-gated modules such as `_ctypes`, `_multiprocessing`, `_posixsubprocess`, `_socket`, `fcntl`, `resource`, `select`, `syslog`, and any other conditionally compiled modules so introspection only reports modules actually available in this build.
♻️ Duplicate comments (3)
pyre/pyre-interpreter/src/module/signal/interp_signal.rs (1)
1029-1036: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftApply the sandbox stub consistently to the signal surface.
This only stubs
pidfd_send_signal; adjacentraise_signal,alarm,setitimer,sigwait,pthread_kill, andpthread_sigmaskstill compile direct host signal/syscall paths underhost_env + sandbox.🤖 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 `@pyre/pyre-interpreter/src/module/signal/interp_signal.rs` around lines 1029 - 1036, The sandbox stub is only applied in the `pidfd_send_signal` path, while nearby signal entry points still use host signal/syscall logic under `host_env + sandbox`. Update the `interp_signal` implementation to route `raise_signal`, `alarm`, `setitimer`, `sigwait`, `pthread_kill`, and `pthread_sigmask` through the same `crate::host_seam::stub(...)` pattern used for `pidfd_send_signal`, so the entire signal surface is consistently stubbed in sandbox builds.pyre/pyre-interpreter/src/importing.rs (1)
71-71: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRoute sandbox
PYRE_STDLIBthrough the seam.
host_osis now gated out of sandbox builds, while Line 808 still uses it; even if reintroduced, this bypasses the controller’s virtual environment. ReadPYRE_STDLIBthroughhost_seam::ops::getenv.Proposed fix
#[cfg(feature = "sandbox")] { - return host_os::var("PYRE_STDLIB").ok().map(PathBuf::from); + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + return crate::host_seam::ops::getenv(b"PYRE_STDLIB") + .ok() + .flatten() + .map(|value| PathBuf::from(OsString::from_vec(value))); }Also applies to: 806-808
🤖 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 `@pyre/pyre-interpreter/src/importing.rs` at line 71, The sandbox path in importing.rs still reads PYRE_STDLIB via host_os, which bypasses the controller seam and can break sandbox builds. Update the logic in the import/bootstrap flow that uses PYRE_STDLIB to fetch it through host_seam::ops::getenv instead, so the sandboxed environment is respected. Keep the change localized around the existing host_os/PYRE_STDLIB handling in the import initialization code.pyre/pyre-interpreter/src/module/time/interp_time.rs (1)
167-174: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not forward unbounded sleeps to the controller.
This still sends the full user-controlled duration to
do_sleep, which blocks the trusted controller in onestd::thread::sleep. Cap or slice controller-side sleeps so sandbox timeouts remain enforceable.🤖 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 `@pyre/pyre-interpreter/src/module/time/interp_time.rs` around lines 167 - 174, The sandbox sleep path in interp_time::do_sleep is still forwarding the full user-controlled duration to the trusted controller, which can block it in one long std::thread::sleep. Update the sandbox branch to cap or chunk the requested sleep before calling crate::host_seam::ops::sleep, and keep looping in smaller slices so controller-side timeouts remain enforceable. Use the existing do_sleep function and sandbox feature-gated branch as the place to apply the fix.
🤖 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 `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 4812-4818: The tuple() and list() constructors still accept extra
positional arguments because they only check args.is_empty() and then use
args[0], so calls like tuple(1, 2) and list(1, 2) are silently accepted. Update
the tuple and list builtin handling in builtins.rs to validate that exactly one
positional argument is allowed after split_builtin_kwargs, and return a
TypeError when args.len() > 1 before any iteration or conversion logic runs. Use
the existing tuple() and list() constructor branches to keep the behavior
consistent for both builtins.
- Around line 8434-8437: The complex() builtin currently only resolves the first
two positional arguments via split_builtin_kwargs and resolve_pos_or_kw, so a
third positional argument is silently ignored. Update the complex argument
handling in builtins.rs to explicitly reject any extra positional arguments
after the real and imag slots, using the existing complex() validation path so
complex(a, b, c) raises an error instead of being accepted.
In `@pyre/pyre-interpreter/src/lib.rs`:
- Around line 28-31: The `host_seam` module is only available on Unix, but the
standalone `sandbox` feature can still enable code paths that reference
`crate::host_seam`. Update the gating in `lib.rs` and any `host_seam`
imports/calls so sandboxed code only compiles with `all(unix, feature =
"sandbox")`, or make the `sandbox` feature Unix-only. Use the `host_seam` module
and all related sandbox entry points as the symbols to locate and align the
feature flags consistently.
In `@pyre/pyre-interpreter/src/module/_locale/interp_locale.rs`:
- Around line 464-481: The sandbox override in interp_locale currently stubs
only setlocale, localeconv, and nl_langinfo, but strcoll and strxfrm still point
to host-backed locale functions and can leak host state. Update the sandbox
block in interp_locale to also replace strcoll and strxfrm with the same
locale_unavailable stub via dict_storage_store/make_builtin_function so all
locale-sensitive APIs are fully isolated under the sandbox feature.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 517-531: Validate the requested read size in interp_posix::read
before converting it to usize/i64. The current handling of args[1] via
pyre_object::w_int_get_value can wrap negative Python counts into a large usize
and then into a negative i64 under the sandbox path, so update the read-size
handling to keep the signed value, reject negatives up front, and only cast
after validation in both the libc::read and crate::host_seam::ops::read
branches.
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 1059-1074: The sandbox write path in the builtin write
implementation is ignoring failures from host_seam::ops::write, so
sys.stdout.write and sys.stderr.write always appear successful. Update the write
handling in the crate::make_builtin_function("write", ...) branch (and the
matching stderr path) to capture the result from crate::host_seam::ops::write
and return/propagate an error when it fails instead of discarding it with let _.
In `@pyre/pyre-sandbox/src/controller.rs`:
- Around line 147-153: The sandboxed child created in controller.rs is
inheriting raw stderr because Command::new(...).spawn() only sets stdin/stdout,
so update the child setup in the spawn path to explicitly configure stderr as
well. Use the existing Command builder in the controller logic to either route
stderr to a bounded trusted drain or set it to Stdio::null(), ensuring the child
cannot write directly to the host console outside the controller protocol.
In `@pyre/pyre-sandbox/src/sandlib.rs`:
- Around line 373-377: The do_sleep implementation in sandlib::do_sleep
currently lets unbounded values from ll_time_sleep reach
std::time::Duration::from_secs_f64 and also blocks the trusted controller thread
while sleeping. Update do_sleep to validate and clamp the requested seconds
before sleeping by using Duration::try_from_secs_f64 and enforcing a small
maximum delay or the request timeout, and keep the fix local to the do_sleep
path so oversized, infinite, or otherwise unsafe sleep values cannot panic or
stall the controller.
In `@pyre/pyrex/src/lib.rs`:
- Around line 53-56: The help text for the Subcommands section currently
advertises interact unconditionally even though the non-Unix path in the
dispatch logic rejects it; update the help output in lib.rs so interact is only
shown on Unix or is explicitly labeled Unix-only. Use the interact subcommand
entry and the Unix dispatch handling as the reference points, and gate the help
line with #[cfg(unix)] or add a clear Unix-only annotation to keep the CLI help
consistent with runtime behavior.
- Around line 337-345: The --verbose flag is currently ignored because
run_interact takes _verbose and never uses it, so the CLI behavior does not
match parse_interact() and usage(). Update run_interact to consume the verbose
flag and wire it into the controller/logging path, using the existing
run_interact and parse_interact symbols; if verbose should not affect behavior,
remove it from the accepted/documented CLI contract instead so usage() and the
argument parsing stay consistent.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/module/mod.rs`:
- Around line 24-94: `sys.builtin_module_names` is still listing modules that
are now excluded by the sandbox build, so update the builtin registration in the
`mod`/`sys` module setup to use the same `cfg(not(feature = "sandbox"))` guards
as the `pub mod` declarations. Align the `builtin_module_names` entries with the
sandbox-gated modules such as `_ctypes`, `_multiprocessing`, `_posixsubprocess`,
`_socket`, `fcntl`, `resource`, `select`, `syslog`, and any other conditionally
compiled modules so introspection only reports modules actually available in
this build.
---
Duplicate comments:
In `@pyre/pyre-interpreter/src/importing.rs`:
- Line 71: The sandbox path in importing.rs still reads PYRE_STDLIB via host_os,
which bypasses the controller seam and can break sandbox builds. Update the
logic in the import/bootstrap flow that uses PYRE_STDLIB to fetch it through
host_seam::ops::getenv instead, so the sandboxed environment is respected. Keep
the change localized around the existing host_os/PYRE_STDLIB handling in the
import initialization code.
In `@pyre/pyre-interpreter/src/module/signal/interp_signal.rs`:
- Around line 1029-1036: The sandbox stub is only applied in the
`pidfd_send_signal` path, while nearby signal entry points still use host
signal/syscall logic under `host_env + sandbox`. Update the `interp_signal`
implementation to route `raise_signal`, `alarm`, `setitimer`, `sigwait`,
`pthread_kill`, and `pthread_sigmask` through the same
`crate::host_seam::stub(...)` pattern used for `pidfd_send_signal`, so the
entire signal surface is consistently stubbed in sandbox builds.
In `@pyre/pyre-interpreter/src/module/time/interp_time.rs`:
- Around line 167-174: The sandbox sleep path in interp_time::do_sleep is still
forwarding the full user-controlled duration to the trusted controller, which
can block it in one long std::thread::sleep. Update the sandbox branch to cap or
chunk the requested sleep before calling crate::host_seam::ops::sleep, and keep
looping in smaller slices so controller-side timeouts remain enforceable. Use
the existing do_sleep function and sandbox feature-gated branch as the place to
apply the fix.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 97510bea-9663-498a-8b9e-c26527ff8cfd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/pyre-ci.ymlCargo.tomlpyre/pyre-interpreter/Cargo.tomlpyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/host_seam.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/_locale/interp_locale.rspyre/pyre-interpreter/src/module/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/time/interp_time.rspyre/pyre-interpreter/src/module/time/mod.rspyre/pyre-interpreter/src/sandbox/mod.rspyre/pyre-sandbox/Cargo.tomlpyre/pyre-sandbox/src/client.rspyre/pyre-sandbox/src/controller.rspyre/pyre-sandbox/src/lib.rspyre/pyre-sandbox/src/protocol.rspyre/pyre-sandbox/src/rmarshal.rspyre/pyre-sandbox/src/sandlib.rspyre/pyre-sandbox/src/seccomp.rspyre/pyre-sandbox/src/vfs.rspyre/pyre-sandbox/tests/e2e_interact.rspyre/pyrex/Cargo.tomlpyre/pyrex/src/lib.rs
💤 Files with no reviewable changes (1)
- pyre/pyre-interpreter/src/sandbox/mod.rs
🛑 Comments failed to post (10)
pyre/pyre-interpreter/src/builtins.rs (2)
4812-4818: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject extra positional arguments for
tuple()andlist().After splitting kwargs, both constructors still ignore
args[1..].tuple(1, 2)/list(1, 2)should raiseTypeError, not silently use the first argument.Proposed fix
let (args, kwargs) = split_builtin_kwargs(args); if has_real_kwargs(kwargs) { return Err(crate::PyError::type_error( "tuple() takes no keyword arguments", )); } +if args.len() > 1 { + return Err(crate::PyError::type_error(format!( + "tuple expected at most 1 argument, got {}", + args.len() + ))); +} if args.is_empty() { return Ok(w_tuple_new(vec![])); }let (args, kwargs) = split_builtin_kwargs(args); if has_real_kwargs(kwargs) { return Err(crate::PyError::type_error( "list() takes no keyword arguments", )); } +if args.len() > 1 { + return Err(crate::PyError::type_error(format!( + "list expected at most 1 argument, got {}", + args.len() + ))); +} if args.is_empty() { return Ok(w_list_new(vec![])); }Also applies to: 4845-4851
🤖 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 `@pyre/pyre-interpreter/src/builtins.rs` around lines 4812 - 4818, The tuple() and list() constructors still accept extra positional arguments because they only check args.is_empty() and then use args[0], so calls like tuple(1, 2) and list(1, 2) are silently accepted. Update the tuple and list builtin handling in builtins.rs to validate that exactly one positional argument is allowed after split_builtin_kwargs, and return a TypeError when args.len() > 1 before any iteration or conversion logic runs. Use the existing tuple() and list() constructor branches to keep the behavior consistent for both builtins.
8434-8437: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject a third positional argument to
complex().The kwargs resolver only consumes
pos[0]andpos[1];complex(a, b, c)should raise instead of ignoringc.Proposed fix
let (pos, kwargs) = split_builtin_kwargs(args); +if pos.len() > 2 { + return Err(crate::PyError::type_error(format!( + "complex expected at most 2 arguments, got {}", + pos.len() + ))); +} kwarg_reject_unknown(kwargs, &["real", "imag"], "complex")?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let (pos, kwargs) = split_builtin_kwargs(args); if pos.len() > 2 { return Err(crate::PyError::type_error(format!( "complex expected at most 2 arguments, got {}", pos.len() ))); } kwarg_reject_unknown(kwargs, &["real", "imag"], "complex")?; let w_real = resolve_pos_or_kw(pos.first().copied(), kwargs, "real", "complex", 1)?; let w_imag = resolve_pos_or_kw(pos.get(1).copied(), kwargs, "imag", "complex", 2)?;🤖 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 `@pyre/pyre-interpreter/src/builtins.rs` around lines 8434 - 8437, The complex() builtin currently only resolves the first two positional arguments via split_builtin_kwargs and resolve_pos_or_kw, so a third positional argument is silently ignored. Update the complex argument handling in builtins.rs to explicitly reject any extra positional arguments after the real and imag slots, using the existing complex() validation path so complex(a, b, c) raises an error instead of being accepted.pyre/pyre-interpreter/src/lib.rs (1)
28-31: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== host_seam references ==" rg -n 'crate::host_seam|host_seam::|use crate::host_seam' pyre/pyre-interpreter/src echo echo "== sandbox feature/cfg declarations ==" rg -n 'feature = "sandbox"|cfg\(.*sandbox|cfg\(unix|target_os|target_family' Cargo.toml pyre/pyre-interpreter/Cargo.toml pyre/pyrex/Cargo.tomlRepository: youknowone/pyre
Length of output: 9908
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== pyre/pyre-interpreter/Cargo.toml ==" sed -n '1,220p' pyre/pyre-interpreter/Cargo.toml echo echo "== pyre/pyre-interpreter/src/lib.rs cfgs around host_seam ==" sed -n '1,70p' pyre/pyre-interpreter/src/lib.rs echo sed -n '800,850p' pyre/pyre-interpreter/src/lib.rs echo echo "== host_seam module declaration ==" sed -n '1,80p' pyre/pyre-interpreter/src/host_seam.rsRepository: youknowone/pyre
Length of output: 9735
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== cross-platform / unix-only signals in pyre-interpreter ==" rg -n 'cfg\((not\()?unix|windows|target_os = "windows"|target_family|unsupported|unix only|cross-platform' pyre/pyre-interpreter/src pyre/pyre-interpreter/Cargo.toml Cargo.toml echo echo "== cfgs around modules that reference host_seam ==" for f in \ pyre/pyre-interpreter/src/importing.rs \ pyre/pyre-interpreter/src/module/time/interp_time.rs \ pyre/pyre-interpreter/src/module/posix/interp_posix.rs \ pyre/pyre-interpreter/src/module/_locale/interp_locale.rs \ pyre/pyre-interpreter/src/module/signal/interp_signal.rs \ pyre/pyre-interpreter/src/module/sys/vm.rs \ pyre/pyre-interpreter/src/builtins.rs do echo "--- $f ---" sed -n '1,80p' "$f" doneRepository: youknowone/pyre
Length of output: 42625
Make
sandboxUnix-only or gate allhost_seamuses withall(unix, feature = "sandbox").host_seamis exported only on Unix, butsandboxis still a standalone feature inpyre/pyre-interpreter/Cargo.toml; non-Unix sandbox builds will hit unresolvedcrate::host_seamimports/calls.🤖 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 `@pyre/pyre-interpreter/src/lib.rs` around lines 28 - 31, The `host_seam` module is only available on Unix, but the standalone `sandbox` feature can still enable code paths that reference `crate::host_seam`. Update the gating in `lib.rs` and any `host_seam` imports/calls so sandboxed code only compiles with `all(unix, feature = "sandbox")`, or make the `sandbox` feature Unix-only. Use the `host_seam` module and all related sandbox entry points as the symbols to locate and align the feature flags consistently.pyre/pyre-interpreter/src/module/_locale/interp_locale.rs (1)
464-481: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Stub
strcollandstrxfrmin sandbox too.
strcollandstrxfrmare still registered above and, withhost_envenabled, callrustpython_host_env::locale::*, so sandboxed code can still observe host collation/locale state. Include them in the sandbox override.Suggested fix
- for name in ["setlocale", "localeconv", "nl_langinfo"] { + for name in ["setlocale", "localeconv", "nl_langinfo", "strcoll", "strxfrm"] {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// setlocale/localeconv/nl_langinfo read the host locale database (and the // active $LANG/$LC_* environment); stub them so the sandbox observes only // the fixed "C" locale defaults already exposed above, never host state. #[cfg(feature = "sandbox")] { fn locale_unavailable( _: &[pyre_object::PyObjectRef], ) -> Result<pyre_object::PyObjectRef, crate::PyError> { Err(crate::host_seam::stub("this locale function")) } for name in ["setlocale", "localeconv", "nl_langinfo", "strcoll", "strxfrm"] { crate::dict_storage_store( ns, name, crate::make_builtin_function(name, locale_unavailable), ); } }🤖 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 `@pyre/pyre-interpreter/src/module/_locale/interp_locale.rs` around lines 464 - 481, The sandbox override in interp_locale currently stubs only setlocale, localeconv, and nl_langinfo, but strcoll and strxfrm still point to host-backed locale functions and can leak host state. Update the sandbox block in interp_locale to also replace strcoll and strxfrm with the same locale_unavailable stub via dict_storage_store/make_builtin_function so all locale-sensitive APIs are fully isolated under the sandbox feature.pyre/pyre-interpreter/src/module/posix/interp_posix.rs (1)
517-531: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate
readsize before casting.A negative Python count is cast through
usizebefore the sandbox call; on 64-bit this wraps and then becomes a negativei64, so sandbox reads return empty bytes instead of raising. Validate the signed value first.Suggested fix
- let n = (unsafe { pyre_object::w_int_get_value(args[1]) }) as usize; + let n_signed = unsafe { pyre_object::w_int_get_value(args[1]) }; + if n_signed < 0 { + return Err(crate::PyError::value_error("negative read size")); + } + let n = n_signed as usize; ... - let buf = crate::host_seam::ops::read(fd, n as i64) + let buf = crate::host_seam::ops::read(fd, n_signed)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let n_signed = unsafe { pyre_object::w_int_get_value(args[1]) }; if n_signed < 0 { return Err(crate::PyError::value_error("negative read size")); } let n = n_signed as usize; #[cfg(not(feature = "sandbox"))] let buf = { let mut buf = vec![0u8; n]; let ret = unsafe { libc::read(fd, buf.as_mut_ptr() as *mut libc::c_void, n as _) }; if ret < 0 { return Err(io_err(std::io::Error::last_os_error(), "")); } buf.truncate(ret as usize); buf }; #[cfg(feature = "sandbox")] let buf = crate::host_seam::ops::read(fd, n_signed) .map_err(|e| crate::host_seam::seam_os_err(e, ""))?;🤖 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 `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 517 - 531, Validate the requested read size in interp_posix::read before converting it to usize/i64. The current handling of args[1] via pyre_object::w_int_get_value can wrap negative Python counts into a large usize and then into a negative i64 under the sandbox path, so update the read-size handling to keep the signed value, reject negatives up front, and only cast after validation in both the libc::read and crate::host_seam::ops::read branches.pyre/pyre-interpreter/src/module/sys/vm.rs (1)
1059-1074: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate sandbox stdio write failures.
host_seam::ops::writecan fail, butsys.stdout.write/sys.stderr.writecurrently report success regardless. Propagate the seam error so controller failures do not silently drop output.Suggested fix
#[cfg(feature = "sandbox")] - let _ = crate::host_seam::ops::write(2, text.as_bytes()); + crate::host_seam::ops::write(2, text.as_bytes()) + .map_err(|e| crate::host_seam::seam_os_err(e, ""))?; ... #[cfg(feature = "sandbox")] - let _ = crate::host_seam::ops::write(1, text.as_bytes()); + crate::host_seam::ops::write(1, text.as_bytes()) + .map_err(|e| crate::host_seam::seam_os_err(e, ""))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.#[cfg(feature = "sandbox")] crate::host_seam::ops::write(2, text.as_bytes()) .map_err(|e| crate::host_seam::seam_os_err(e, ""))?; return Ok(w_int_new(text.len() as i64)); } Ok(w_int_new(0)) }) } else { crate::make_builtin_function("write", |args| { if let Some(text) = pick_str(args) { #[cfg(not(feature = "sandbox"))] { use std::io::Write; let _ = std::io::stdout().write_all(text.as_bytes()); } #[cfg(feature = "sandbox")] crate::host_seam::ops::write(1, text.as_bytes()) .map_err(|e| crate::host_seam::seam_os_err(e, ""))?;🤖 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 `@pyre/pyre-interpreter/src/module/sys/vm.rs` around lines 1059 - 1074, The sandbox write path in the builtin write implementation is ignoring failures from host_seam::ops::write, so sys.stdout.write and sys.stderr.write always appear successful. Update the write handling in the crate::make_builtin_function("write", ...) branch (and the matching stderr path) to capture the result from crate::host_seam::ops::write and return/propagate an error when it fails instead of discarding it with let _.pyre/pyre-sandbox/src/controller.rs (1)
147-153: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not inherit raw stderr into the sandboxed child.
stderris not configured, so the child inherits fd 2 and can write directly to the host console outside the controller protocol. Pipe it through a bounded trusted drain or set it toStdio::null().🤖 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 `@pyre/pyre-sandbox/src/controller.rs` around lines 147 - 153, The sandboxed child created in controller.rs is inheriting raw stderr because Command::new(...).spawn() only sets stdin/stdout, so update the child setup in the spawn path to explicitly configure stderr as well. Use the existing Command builder in the controller logic to either route stderr to a bounded trusted drain or set it to Stdio::null(), ensuring the child cannot write directly to the host console outside the controller protocol.pyre/pyre-sandbox/src/sandlib.rs (1)
373-377: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify all sandbox sleep paths and whether tests cover huge/non-finite sleep requests. rg -n -C3 'll_time_sleep|do_sleep|from_secs_f64|Duration::from_secs_f64|timeout' pyre/pyre-sandbox/src pyre/pyre-sandbox/tests pyre/pyre-interpreter/srcRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant files first. ast-grep outline pyre/pyre-sandbox/src/sandlib.rs --view expanded ast-grep outline pyre/pyre-sandbox/src/controller.rs --view expanded # Read the exact sleep and watchdog implementations with line numbers. sed -n '340,390p' pyre/pyre-sandbox/src/sandlib.rs sed -n '50,220p' pyre/pyre-sandbox/src/controller.rs # Look for tests or docs that mention sleep behavior or non-finite timeout handling. rg -n -C2 'sleep|from_secs_f64|try_from_secs_f64|non-finite|NaN|inf|timeout' pyre/pyre-sandbox/tests pyre/pyre-sandbox/src pyre/pyre-interpreter/srcRepository: youknowone/pyre
Length of output: 50374
🌐 Web query:
Rust std::time::Duration::from_secs_f64 panics NaN infinity overflow docs💡 Result:
The Rust function std::time::Duration::from_secs_f64 is documented to panic under three specific conditions [1][2]: 1. If the provided f64 value is negative. 2. If the provided f64 value is not finite (i.e., NaN or Infinity). 3. If the provided f64 value overflows the capacity of a Duration. The implementation performs these checks and panics if any condition is met [3]. If you need to avoid panics, you should use the non-panicking alternative: Duration::try_from_secs_f64, which returns a Result instead of panicking [3].
Citations:
- 1: https://doc.rust-lang.org/std/time/struct.Duration.html
- 2: https://doc.rust-lang.org/stable/std/time/struct.Duration.html
- 3: https://github.com/rust-lang/rust/blob/0f145634/library/core/src/time.rs
Bound
ll_time_sleepbefore sleeping the trusted controller.seconds > 0.0still lets+infand oversized finite values reachDuration::from_secs_f64, which can panic; the sleep also runs on the trusted controller thread, so a child can stall the whole interaction until it returns. UseDuration::try_from_secs_f64and cap the delay to a small policy limit or the request timeout.🤖 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 `@pyre/pyre-sandbox/src/sandlib.rs` around lines 373 - 377, The do_sleep implementation in sandlib::do_sleep currently lets unbounded values from ll_time_sleep reach std::time::Duration::from_secs_f64 and also blocks the trusted controller thread while sleeping. Update do_sleep to validate and clamp the requested seconds before sleeping by using Duration::try_from_secs_f64 and enforcing a small maximum delay or the request timeout, and keep the fix local to the do_sleep path so oversized, infinite, or otherwise unsafe sleep values cannot panic or stall the controller.pyre/pyrex/src/lib.rs (2)
53-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Help text should call out that
interactis Unix-only.The non-Unix dispatch path exits with “only supported on Unix”, but the help output advertises the subcommand unconditionally. Hiding this line behind
#[cfg(unix)]or annotating it as Unix-only would avoid promising a command that always fails on Windows.🤖 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 `@pyre/pyrex/src/lib.rs` around lines 53 - 56, The help text for the Subcommands section currently advertises interact unconditionally even though the non-Unix path in the dispatch logic rejects it; update the help output in lib.rs so interact is only shown on Unix or is explicitly labeled Unix-only. Use the interact subcommand entry and the Unix dispatch handling as the reference points, and gate the help line with #[cfg(unix)] or add a clear Unix-only annotation to keep the CLI help consistent with runtime behavior.
337-345: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--verboseis currently a no-op.
parse_interact()records this flag andusage()documents it, butrun_interact()takes_verboseand never changes behavior. Either wire it into controller logging or stop accepting/documenting it so the CLI contract matches reality.🤖 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 `@pyre/pyrex/src/lib.rs` around lines 337 - 345, The --verbose flag is currently ignored because run_interact takes _verbose and never uses it, so the CLI behavior does not match parse_interact() and usage(). Update run_interact to consume the verbose flag and wire it into the controller/logging path, using the existing run_interact and parse_interact symbols; if verbose should not affect behavior, remove it from the accepted/documented CLI contract instead so usage() and the argument parsing stay consistent.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00e83fdfb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .unwrap_or_else(|_| executable.as_ref().to_path_buf()); | ||
| let virtual_root = build_virtual_root(&executable, tmpdir.as_deref(), lib_root.as_deref()); | ||
| // pypy_interact.py:41 `virtual_console_isatty = True`. | ||
| let policy = SandboxPolicy::new(virtual_root, VIRTUAL_CWD, Vec::new(), true); |
There was a problem hiding this comment.
When --lib is provided, the controller mounts that directory at /bin/lib, but the policy's virtual environment is still empty here and the child spawn clears the real environment below; sandbox detect_stdlib_path() only adds a stdlib when PYRE_STDLIB is present. As a result pyre interact --lib <stdlib> <pyre> -c 'import os' still cannot find the mounted stdlib. Please set PYRE_STDLIB=/bin/lib in the child and/or virtual env when lib_root is provided.
Useful? React with 👍 / 👎.
| libc::SYS_lseek, | ||
| libc::SYS_fstat, | ||
| libc::SYS_fcntl, | ||
| libc::SYS_getcwd, |
There was a problem hiding this comment.
Because RunMode::Command, RunMode::Module, and RunMode::Repl still call std::env::current_dir() after the sandbox filter is installed and then put that value into sys.path, allowing raw getcwd here lets sandboxed code print sys.path and learn the controller's real working directory instead of only the virtual /tmp. Please initialize those startup paths from the controller/seam virtual cwd before allowlisting this syscall.
Useful? React with 👍 / 👎.
| #[cfg(feature = "sandbox")] | ||
| real_main(binary_name); |
There was a problem hiding this comment.
Sandbox builds now run real_main on the process main thread, but real_main still raises the Python recursion limit to 5000. On the normal main-thread stack (commonly much smaller than the 256 MiB thread used by non-sandbox builds), deeply recursive sandboxed Python code can overflow/abort the process before Pyre raises RecursionError; keep the large-stack interpreter thread for the child or lower the sandbox recursion limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-sandbox/src/seccomp.rs (1)
164-169: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep SIGSYS trapped.
rt_sigactionis still allowlisted, so sandboxed code can replace theSIGSYShandler and turn seccomp traps into a recoverable path. If these signal syscalls are still needed, special-caseSIGSYSso the deny backstop stays fail-closed.🤖 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 `@pyre/pyre-sandbox/src/seccomp.rs` around lines 164 - 169, The seccomp signal allowlist in seccomp.rs still permits rt_sigaction, which lets sandboxed code replace the SIGSYS handler and bypass the trap-based fail-closed behavior. Update the signal syscall handling in the seccomp policy around the signal allowlist to special-case SIGSYS so it remains trapped even if rt_sigaction and related runtime signal syscalls are allowed. Use the existing seccomp rules in the seccomp module to keep the deny backstop intact.
🤖 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 `@pyre/pyre-sandbox/src/seccomp.rs`:
- Around line 184-191: Remove the obsolete SYS_tkill allowance from the seccomp
syscall list and keep SYS_tgkill as the only signal-sending primitive for the
abort/raise path. Update the syscall set in seccomp.rs where the process
self-info and clean-exit permissions are defined so the sandbox surface is
minimized while preserving the existing abort behavior.
---
Outside diff comments:
In `@pyre/pyre-sandbox/src/seccomp.rs`:
- Around line 164-169: The seccomp signal allowlist in seccomp.rs still permits
rt_sigaction, which lets sandboxed code replace the SIGSYS handler and bypass
the trap-based fail-closed behavior. Update the signal syscall handling in the
seccomp policy around the signal allowlist to special-case SIGSYS so it remains
trapped even if rt_sigaction and related runtime signal syscalls are allowed.
Use the existing seccomp rules in the seccomp module to keep the deny backstop
intact.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 592ff893-46d4-49f9-a340-56d816c90d38
📒 Files selected for processing (1)
pyre/pyre-sandbox/src/seccomp.rs
| // Process self-info (read-only) + own resource limits/usage + abort path | ||
| // + clean exit. tkill/tgkill only ever target this single-threaded child. | ||
| libc::SYS_getpid, | ||
| libc::SYS_gettid, | ||
| libc::SYS_getrusage, | ||
| libc::SYS_prlimit64, | ||
| libc::SYS_tkill, | ||
| libc::SYS_tgkill, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether the sandboxed post-filter runtime has a real dependency on tkill/tgkill.
rg -n -C3 '\b(SYS_)?(tkill|tgkill)\b|libc::raise|std::process::abort|abort\s*\(' \
pyre/pyre-sandbox pyre/pyrex pyre/pyre-interpreterRepository: youknowone/pyre
Length of output: 3463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the seccomp allowlist and the abort/raise call paths that justify it.
sed -n '1,260p' pyre/pyre-sandbox/src/seccomp.rs
printf '\n--- faulthandler handler ---\n'
sed -n '250,340p' pyre/pyre-interpreter/src/module/faulthandler/handler.rs
printf '\n--- signal/syscall references ---\n'
rg -n -C2 '\b(tkill|tgkill|raise|abort|SIGABRT|SIGSYS)\b' pyre/pyre-sandbox pyre/pyre-interpreter pyre/pyrexRepository: youknowone/pyre
Length of output: 50374
🌐 Web query:
Linux tkill tgkill man page semantics target same thread group vs any process; glibc abort raise implementation tgkill
💡 Result:
The primary difference between the Linux tkill and tgkill system calls lies in how they identify the target thread and prevent unintended signal delivery [1][2]. tkill(tid, sig) is an obsolete system call that targets a thread solely by its thread ID (TID) [1]. Because TIDs can be recycled after a thread terminates, tkill risks signaling the wrong thread if the original target thread has exited and a new thread has been assigned its ID [1][3]. tgkill(tgid, tid, sig) addresses this vulnerability by requiring both the thread group ID (TGID) and the TID [1][4]. By verifying that the thread belongs to the specified thread group, tgkill ensures the signal is delivered only to the intended thread, even if the TID has been recycled by a thread in a different process [1][5]. Regarding glibc implementation, abort() and raise() utilize tgkill to send signals reliably and safely [6][7]. In modern glibc versions, internal helpers such as pthread_raise_internal are used to issue tgkill calls [6][8][9]. These helpers typically use the getpid and gettid system calls to ensure that the signal is correctly routed to the caller, even in complex scenarios like post-vfork execution where standard process/thread identifiers might be temporarily inconsistent [6][9]. The use of tgkill in these contexts is preferred over tkill for its increased safety and compatibility with modern security features like seccomp sandboxing, which often restrict tkill to prevent arbitrary signal injection [10][11]. [1][10][6][2]
Citations:
- 1: https://man7.org/linux/man-pages/man2/tkill.2.html
- 2: https://linux.die.net/man/2/tgkill
- 3: https://linux.die.net/man/2/tkill
- 4: https://man7.org/linux/man-pages/man3/probe::signal.sys_tgkill.3stap.html
- 5: https://www.besthub.dev/articles/how-linux-implements-signals-across-threads-and-thread-groups-5f19dd755ff4
- 6: https://sourceware.org/pipermail/glibc-cvs/2024q2/085461.html
- 7: https://sourceware.org/pipermail/glibc-cvs/2024q4/086479.html
- 8: https://sourceware.org/pipermail/libc-alpha/2023-July/150515.html
- 9: https://github.com/bminor/glibc/blob/master/nptl/pthread_kill.c
- 10: runtime: use tgkill instead of tkill to keep Android happy golang/go#24924
- 11: runtime: use tgkill instead of tkill golang/go#27548
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where the sandbox filter is installed and whether post-filter code paths
# explicitly rely on abort/raise signals.
rg -n -C3 'install_runtime_filter\(|libc::abort\(|libc::raise\(|SIGABRT|SIGSEGV|SIGFPE|SIGSYS|tkill|tgkill' \
pyre/pyre-sandbox pyre/pyrex pyre/pyre-interpreterRepository: youknowone/pyre
Length of output: 12173
Drop tkill; keep tgkill for the abort/raise path. abort()/raise() route through tgkill, so tkill is the only obsolete signal primitive here and widens the sandbox surface without a runtime need.
🤖 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 `@pyre/pyre-sandbox/src/seccomp.rs` around lines 184 - 191, Remove the obsolete
SYS_tkill allowance from the seccomp syscall list and keep SYS_tgkill as the
only signal-sending primitive for the abort/raise path. Update the syscall set
in seccomp.rs where the process self-info and clean-exit permissions are defined
so the sandbox surface is minimized while preserving the existing abort
behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 000bad8b6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn do_sleep(&mut self, args: &[MarshalValue]) -> SandboxResult<Reply> { | ||
| let seconds = arg_float(args, 0)?; | ||
| if seconds > 0.0 { | ||
| std::thread::sleep(std::time::Duration::from_secs_f64(seconds)); |
There was a problem hiding this comment.
When untrusted code calls time.sleep(large_value), the request is handled by the trusted controller here, so the watchdog can kill the child after --timeout but cannot interrupt this controller thread while it is sleeping. In that scenario pyre interact --timeout 1 ... -c 'import time; time.sleep(10**9)' still hangs in the parent until the full sleep finishes; cap/poll controller-side sleeps against the timeout or make the watchdog interrupt the handler as well as the child.
Useful? React with 👍 / 👎.
| if more.is_empty() { | ||
| return Ok(true); | ||
| } | ||
| self.buf.extend_from_slice(&more); |
There was a problem hiding this comment.
handle_until_return_ticked reuses one Loader for the entire child lifetime; after a request is fully consumed pos == buf.len(), but this appends the next request to the old buffer instead of clearing it. A sandboxed program that performs many mediated calls (for example repeated print()/time.time() calls) makes the trusted controller retain all prior protocol traffic and can grow memory until OOM; reset or drain the buffer at message boundaries before appending new bytes.
Useful? React with 👍 / 👎.
| if !is_interact && std::env::var_os("PYRE_SANDBOX_NO_SECCOMP").is_none() { | ||
| if let Err(e) = pyre_sandbox::seccomp::install_runtime_filter() { |
There was a problem hiding this comment.
With a sandbox child invoked on a script path (for example pyre interact --tmp DIR <pyre> /tmp/app.py), seccomp is installed here before the RunMode::Script arm still does std::fs::read_to_string(&path). On Linux that raw openat is killed before the virtual /tmp script can run, while on non-Linux/no-seccomp builds it reads the controller's real filesystem instead of the VFS; script source should be obtained through the controller/seam rather than the host FS path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8b0f169a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .get_mut(&fd) | ||
| .ok_or(SandboxError::Os(libc::EBADF))?; | ||
| let whence = match how { | ||
| libc::SEEK_SET => SeekFrom::Start(pos as u64), |
There was a problem hiding this comment.
When sandboxed code calls seek/posix.lseek with SEEK_SET and a negative offset, this cast turns values like -1 into a huge u64. The in-memory Cursor used for virtual files accepts that position, so open('/tmp/x').seek(-1, 0) succeeds and returns a huge offset instead of raising OSError(EINVAL) like the real libc path. Please reject pos < 0 before constructing SeekFrom::Start.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
pyre/pyrex/src/lib.rs (1)
32-32: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate
--timeoutbefore constructingDuration.
f64accepts values like negative numbers,NaN, and infinity; passing those toDuration::from_secs_f64can panicpyre interact. Reject invalid values with a usage error, or store a validatedDurationinRunMode::Interact.Suggested localized fix
- let timeout = timeout.map(std::time::Duration::from_secs_f64); + let timeout = match timeout { + Some(secs) => match std::time::Duration::try_from_secs_f64(secs) { + Ok(duration) => Some(duration), + Err(_) => { + eprintln!("{binary_name}: invalid --timeout value: {secs}"); + std::process::exit(2); + } + }, + None => None, + };#!/bin/bash # Verify the declared Rust toolchain supports Duration::try_from_secs_f64, # or replace the suggested fix with explicit finite/range validation. rg -n 'rust-version|channel|toolchain|try_from_secs_f64|from_secs_f64' Cargo.toml pyre --glob '*.toml' --glob '*.rs'Also applies to: 135-135, 350-350
🤖 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 `@pyre/pyrex/src/lib.rs` at line 32, Validate the `timeout` value before converting it in `RunMode::Interact` and any other `from_secs_f64` call sites in `pyrex/src/lib.rs`; `f64` can be negative, NaN, or infinite, so reject invalid inputs with a usage error (or precompute a validated `Duration`) before constructing `Duration::from_secs_f64`, and apply the same check wherever `timeout` is passed through the `RunMode`/`interact` flow.pyre/pyre-interpreter/src/importing.rs (1)
806-809: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRoute sandbox
PYRE_STDLIBthroughhost_seam::ops::getenv.The sandbox branch still uses
host_os::var, but thehost_osimport is non-sandbox-gated at Line 71; this either fails sandbox compilation or reintroduces direct host-env access. Read the value through the env seam and convert the returned bytes to aPathBuf.Suggested fix
#[cfg(feature = "sandbox")] { - return host_os::var("PYRE_STDLIB").ok().map(PathBuf::from); + use std::os::unix::ffi::OsStringExt; + + return crate::host_seam::ops::getenv(b"PYRE_STDLIB") + .ok() + .flatten() + .map(|v| PathBuf::from(std::ffi::OsString::from_vec(v))); }🤖 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 `@pyre/pyre-interpreter/src/importing.rs` around lines 806 - 809, The sandbox-specific `PYRE_STDLIB` lookup in `importing::get_stdlib_path` still reads directly from `host_os::var`, so update that branch to use `host_seam::ops::getenv` instead and convert the returned bytes into a `PathBuf`. Keep the change scoped to the `#[cfg(feature = "sandbox")]` path and preserve the existing `get_stdlib_path` behavior when the environment variable is absent.pyre/pyre-interpreter/src/module/signal/interp_signal.rs (1)
1029-1036: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftApply the same sandbox gate to the rest of the host-backed signal surface.
pidfd_send_signalis now stubbed, but adjacent APIs likeraise_signal,alarm,pause,setitimer,sigwait,pthread_kill, andpthread_sigmaskstill compile under plain#[cfg(feature = "host_env")], so sandbox+host_env builds can still reach direct host signal/timer syscalls. Gate those bodies withnot(feature = "sandbox")or add sandbox stubs for the whole unsupported signal surface.🤖 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 `@pyre/pyre-interpreter/src/module/signal/interp_signal.rs` around lines 1029 - 1036, The sandbox guard in interp_signal.rs is only applied to pidfd_send_signal, while other host-backed signal/timer entry points such as raise_signal, alarm, pause, setitimer, sigwait, pthread_kill, and pthread_sigmask still compile under host_env and can reach direct syscalls. Update the cfgs on those functions in the signal module so they are excluded when feature = "sandbox" is enabled, either by adding not(feature = "sandbox") to the existing host_env gates or by routing each unsupported path to a sandbox stub like the pidfd_send_signal handling.pyre/pyre-interpreter/src/module/time/interp_time.rs (1)
167-172: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not let sandbox
sleep()block the trusted controller for the full user duration.This forwards the full untrusted duration to the controller; the controller-side sleep implementation blocks in one
std::thread::sleep, so a hugetime.sleep()can keeppyre interactstuck even after the watchdog kills the child. Cap or slice the sleep request so controller timeout handling remains responsive.🤖 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 `@pyre/pyre-interpreter/src/module/time/interp_time.rs` around lines 167 - 172, The sandbox sleep path in the time interpreter currently forwards the full user-provided duration through `crate::host_seam::ops::sleep`, which can block the trusted controller too long. Update the sandbox branch in `interp_time.rs` to cap, chunk, or otherwise slice large sleep requests before calling `sleep()` so controller-side timeout handling stays responsive. Keep the existing `w_none()` behavior and error mapping, but ensure `sleep()` is only asked to wait for bounded intervals rather than the entire untrusted duration at once.
🤖 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 `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 7495-7498: The fd cleanup path in the sandbox close branch is
ignoring the result of crate::host_seam::ops::close(fd), which hides
controller-side failures and leaves the wrapper marked closed. Update the close
handling in the builtins code so the closure state is only recorded after a
successful close, and propagate or surface the error from host_seam::ops::close
instead of discarding it.
In `@pyre/pyre-interpreter/src/lib.rs`:
- Around line 827-828: The sandbox output relay in the `write(1, s.as_bytes())`
path ignores partial writes, so large `print()` output can be truncated. Update
the sandbox write handling in `lib.rs` to keep writing until all bytes from
`s.as_bytes()` are sent, using the returned byte count from
`crate::host_seam::ops::write`; only stop on `Ok(0)` or an error. Keep the fix
localized to the output relay logic around the Unix `sandbox` branch.
In `@pyre/pyre-interpreter/src/module/_locale/interp_locale.rs`:
- Around line 464-481: The sandbox locale gate in interp_locale.rs only stubs
setlocale, localeconv, and nl_langinfo, while strcoll and strxfrm still reach
rustpython_host_env::locale::* under sandbox+host_env. Update the
strcoll/strxfrm branches to be excluded when feature = "sandbox" is enabled so
they fall back to the pure locale behavior, or add sandbox stubs for them in the
same locale_unavailable pattern. Keep the change localized around the existing
sandbox block and the strcoll/strxfrm function paths.
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 1059-1061: The sandbox stdio write path in `sys::vm::write` is
ignoring the `Result<i64>` from `crate::host_seam::ops::write` and always
returning the full `text.len()`, so update both sandbox branches to handle the
return value from `host_seam::ops::write` and propagate any failure instead of
discarding it. Use the actual byte count returned by `write` for the
`w_int_new(...)` result, and ensure the same fix is applied in the other
matching sandbox branch around the `write` call so partial writes and errors are
preserved.
In `@pyre/pyre-sandbox/src/controller.rs`:
- Around line 147-153: The child process setup in Command::new(...).spawn()
currently clears the environment but still allows inherited non-stdio file
descriptors to leak into the sandboxed exec. Update the controller::spawn path
to add a pre_exec close sweep using close_range, closefrom, or an equivalent
fd-closing loop so that only stdin/stdout/stderr remain open before exec. Keep
the fix localized around the child creation logic and ensure it runs before
spawning the untrusted process.
In `@pyre/pyre-sandbox/src/sandlib.rs`:
- Around line 373-377: The do_sleep method currently trusts the untrusted
seconds argument, so update it to validate and bound the value before calling
std::time::Duration::from_secs_f64. In sandlib.rs, within do_sleep, reject
non-finite, negative, or excessively large durations, and cap any accepted sleep
to the configured controller timeout so a malicious request cannot panic the
controller or block it beyond the watchdog.
In `@pyre/pyre-sandbox/src/seccomp.rs`:
- Line 189: The seccomp allowlist currently permits libc::SYS_prlimit64 without
restricting which process is targeted, so update the filtering in seccomp.rs to
keep prlimit64 enabled but only allow calls where the pid argument is 0. Use the
existing syscall filtering setup around the syscall allowlist and prlimit64
entry to add argument-aware checks, so getrlimit/setrlimit continue working
while preventing inspection or modification of other same-UID processes’ limits.
In `@pyre/pyre-sandbox/src/vfs.rs`:
- Line 4: The path validation in the VFS name handling is incomplete because
checking name.contains(std::path::MAIN_SEPARATOR) can miss separators on some
platforms and still allow nested paths to reach self.path.join(name). Update the
logic in the relevant vfs.rs path-checking code to validate with
Path::components() instead, and reject any input with more than one component,
including prefixes, . and .., before joining it into the sandbox path.
---
Duplicate comments:
In `@pyre/pyre-interpreter/src/importing.rs`:
- Around line 806-809: The sandbox-specific `PYRE_STDLIB` lookup in
`importing::get_stdlib_path` still reads directly from `host_os::var`, so update
that branch to use `host_seam::ops::getenv` instead and convert the returned
bytes into a `PathBuf`. Keep the change scoped to the `#[cfg(feature =
"sandbox")]` path and preserve the existing `get_stdlib_path` behavior when the
environment variable is absent.
In `@pyre/pyre-interpreter/src/module/signal/interp_signal.rs`:
- Around line 1029-1036: The sandbox guard in interp_signal.rs is only applied
to pidfd_send_signal, while other host-backed signal/timer entry points such as
raise_signal, alarm, pause, setitimer, sigwait, pthread_kill, and
pthread_sigmask still compile under host_env and can reach direct syscalls.
Update the cfgs on those functions in the signal module so they are excluded
when feature = "sandbox" is enabled, either by adding not(feature = "sandbox")
to the existing host_env gates or by routing each unsupported path to a sandbox
stub like the pidfd_send_signal handling.
In `@pyre/pyre-interpreter/src/module/time/interp_time.rs`:
- Around line 167-172: The sandbox sleep path in the time interpreter currently
forwards the full user-provided duration through `crate::host_seam::ops::sleep`,
which can block the trusted controller too long. Update the sandbox branch in
`interp_time.rs` to cap, chunk, or otherwise slice large sleep requests before
calling `sleep()` so controller-side timeout handling stays responsive. Keep the
existing `w_none()` behavior and error mapping, but ensure `sleep()` is only
asked to wait for bounded intervals rather than the entire untrusted duration at
once.
In `@pyre/pyrex/src/lib.rs`:
- Line 32: Validate the `timeout` value before converting it in
`RunMode::Interact` and any other `from_secs_f64` call sites in
`pyrex/src/lib.rs`; `f64` can be negative, NaN, or infinite, so reject invalid
inputs with a usage error (or precompute a validated `Duration`) before
constructing `Duration::from_secs_f64`, and apply the same check wherever
`timeout` is passed through the `RunMode`/`interact` flow.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 596f94e0-c75f-4c91-b01d-28b925b0ab5d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/pyre-ci.ymlCargo.tomlpyre/pyre-interpreter/Cargo.tomlpyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/host_seam.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/_locale/interp_locale.rspyre/pyre-interpreter/src/module/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/time/interp_time.rspyre/pyre-interpreter/src/module/time/mod.rspyre/pyre-interpreter/src/sandbox/mod.rspyre/pyre-sandbox/Cargo.tomlpyre/pyre-sandbox/src/client.rspyre/pyre-sandbox/src/controller.rspyre/pyre-sandbox/src/lib.rspyre/pyre-sandbox/src/protocol.rspyre/pyre-sandbox/src/rmarshal.rspyre/pyre-sandbox/src/sandlib.rspyre/pyre-sandbox/src/seccomp.rspyre/pyre-sandbox/src/vfs.rspyre/pyre-sandbox/tests/e2e_interact.rspyre/pyrex/Cargo.tomlpyre/pyrex/src/lib.rs
💤 Files with no reviewable changes (1)
- pyre/pyre-interpreter/src/sandbox/mod.rs
| fn do_sleep(&mut self, args: &[MarshalValue]) -> SandboxResult<Reply> { | ||
| let seconds = arg_float(args, 0)?; | ||
| if seconds > 0.0 { | ||
| std::thread::sleep(std::time::Duration::from_secs_f64(seconds)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'll_time_sleep|do_sleep|from_secs_f64|timeout' \
pyre/pyre-sandbox/src pyre/pyre-interpreter/src/module/timeRepository: youknowone/pyre
Length of output: 9364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sleep handler, timeout wiring, and request/response loop.
sed -n '1,260p' pyre/pyre-sandbox/src/sandlib.rs
printf '\n--- controller.rs ---\n'
sed -n '1,280p' pyre/pyre-sandbox/src/controller.rsRepository: youknowone/pyre
Length of output: 19819
Bound sandbox-requested sleep durations. seconds comes from the untrusted request; reject non-finite or oversized values before Duration::from_secs_f64, or a bad value can panic the controller and a very large valid sleep can block the trusted side well past the watchdog timeout. Cap this to the configured controller timeout.
🤖 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 `@pyre/pyre-sandbox/src/sandlib.rs` around lines 373 - 377, The do_sleep method
currently trusts the untrusted seconds argument, so update it to validate and
bound the value before calling std::time::Duration::from_secs_f64. In
sandlib.rs, within do_sleep, reject non-finite, negative, or excessively large
durations, and cap any accepted sleep to the configured controller timeout so a
malicious request cannot panic the controller or block it beyond the watchdog.
| libc::SYS_getpid, | ||
| libc::SYS_gettid, | ||
| libc::SYS_getrusage, | ||
| libc::SYS_prlimit64, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 '\b(prlimit64|getrlimit|setrlimit|SYS_prlimit64|rlimit)\b' pyreRepository: youknowone/pyre
Length of output: 10250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- seccomp.rs around prlimit64 ---'
sed -n '150,240p' pyre/pyre-sandbox/src/seccomp.rs
echo
echo '--- search for prlimit64 syscall usage ---'
rg -n '\bprlimit64\b|SYS_prlimit64|prlimit' pyre -g '!**/target/**'Repository: youknowone/pyre
Length of output: 4283
🌐 Web query:
Linux prlimit64 pid 0 same uid resource limits glibc getrlimit setrlimit wrapper syscall semantics
💡 Result:
In Linux, the prlimit64 system call is the kernel-level interface for managing process resource limits [1][2]. When the pid argument is set to 0, the system call operation applies specifically to the calling process [1][3]. Glibc implementation details and semantics: 1. Glibc Wrapper: The glibc library provides a wrapper function called prlimit, which serves as the user-facing interface for the underlying prlimit64 system call [1][2]. 2. Evolution of getrlimit/setrlimit: Since glibc 2.13, the standard glibc wrapper functions getrlimit and setrlimit no longer invoke their historical, separate system calls [4][2]. Instead, they are implemented as wrappers that internally call prlimit [2][5]. 3. 32-bit/64-bit Compatibility: A primary motivation for this architectural shift was to resolve limitations on 32-bit systems [2][6]. Older getrlimit/setrlimit system calls used a 32-bit unsigned long for resource limits, which was insufficient for large values like those required for file size limits (RLIMIT_FSIZE) when using a 64-bit off_t [1][6]. The prlimit64 system call uses a 64-bit data structure (rlimit64), allowing glibc to handle large limits correctly even on 32-bit architectures [3][5]. 4. Permission Semantics: While pid 0 refers to the calling process, operating on a non-zero pid requires specific privileges [1][2]. To change or retrieve the limits of another process, the caller must either possess the CAP_SYS_RESOURCE capability in the target process's user namespace or satisfy strict UID/GID matching requirements (the real, effective, and saved set user IDs of the target must match the caller's real user ID, and the same applies to group IDs) [1][2]. In summary, prlimit64 with pid 0 is the canonical, modern way to manage the calling process's limits, and glibc transparently utilizes this interface to provide 64-bit clean resource management for all standard limit-related functions [4][2][5].
Citations:
- 1: https://manpages.debian.org/bookworm/manpages-dev/prlimit64.2.en.html
- 2: https://man7.org/linux/man-pages/man2/prlimit.2.html
- 3: https://lkml.iu.edu/1006.2/02958.html
- 4: https://man.archlinux.org/man/prlimit.2
- 5: https://codebrowser.dev/glibc/glibc/sysdeps/unix/sysv/linux/prlimit.c.html
- 6: https://manpages.ubuntu.com/manpages/xenial/man2/setrlimit.2.html
Constrain prlimit64 to pid == 0. getrlimit/setrlimit use prlimit64, so removing it would break those wrappers; keep the syscall, but add argument-aware filtering so this sandbox can only touch its own limits. The current number-only allowlist still permits adjusting or inspecting other same-UID processes.
🤖 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 `@pyre/pyre-sandbox/src/seccomp.rs` at line 189, The seccomp allowlist
currently permits libc::SYS_prlimit64 without restricting which process is
targeted, so update the filtering in seccomp.rs to keep prlimit64 enabled but
only allow calls where the pid argument is 0. Use the existing syscall filtering
setup around the syscall allowlist and prlimit64 entry to add argument-aware
checks, so getrlimit/setrlimit continue working while preventing inspection or
modification of other same-UID processes’ limits.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pyre/pyre-sandbox/src/vfs.rs (1)
298-305: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
is_absolute()still leaves a Windows-prefixed escape here.Line 305 blocks absolute paths, but Windows-prefixed names like
C:orC:fooare not absolute and contain no/or\, so they still reachself.path.join(name). That breaks the “single child underself.path” invariant on Windows. Please validate withPath::components()and accept only a singleComponent::Normal(_), while keeping the explicit/and\rejection for cross-platform hardening.Suggested fix
- if name.is_empty() - || name == ".." - // Reject BOTH separators on every platform: a child name is a single - // component, and `std::path::MAIN_SEPARATOR` alone would miss `/` on - // Windows (letting `a/b` resolve as a nested path = a traversal). - || name.contains(['/', '\\']) - || std::path::Path::new(name).is_absolute() - { + let mut components = std::path::Path::new(name).components(); + let is_single_child = + matches!(components.next(), Some(std::path::Component::Normal(_))) + && components.next().is_none(); + if name.contains(['/', '\\']) || !is_single_child { return Err(VfsError { errno: ENOENT, object: name.to_owned(), }); }🤖 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 `@pyre/pyre-sandbox/src/vfs.rs` around lines 298 - 305, The join method in VfsNode still allows Windows drive-prefixed inputs like C: or C:foo because Path::is_absolute() does not reject them. Update join to validate name using Path::components() and only accept a single Component::Normal(_) while keeping the explicit rejection of / and \ separators and the existing empty/.. checks. Reference the join function in FsNode/VfsNode so the fix preserves the single-child-under-self.path invariant across platforms.
🤖 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.
Duplicate comments:
In `@pyre/pyre-sandbox/src/vfs.rs`:
- Around line 298-305: The join method in VfsNode still allows Windows
drive-prefixed inputs like C: or C:foo because Path::is_absolute() does not
reject them. Update join to validate name using Path::components() and only
accept a single Component::Normal(_) while keeping the explicit rejection of /
and \ separators and the existing empty/.. checks. Reference the join function
in FsNode/VfsNode so the fix preserves the single-child-under-self.path
invariant across platforms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca2eb9f4-9ebe-4e8d-826c-9c0c197ecaa3
📒 Files selected for processing (4)
.github/workflows/pyre-ci.ymlpyre/pyre-sandbox/src/vfs.rspyre/pyrex/Cargo.tomlpyre/pyrex/src/lib.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 182ce554f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // durability + real process environment mutation | ||
| "fsync", "fdatasync", "setenv", "unsetenv", "putenv", | ||
| // host filesystem inspection that bypasses the controller VFS | ||
| "readlink", "scandir", "statvfs", "fstatvfs", |
There was a problem hiding this comment.
In sandbox builds this stub pass removes scandir, but it leaves the exported posix.DirEntry type live; because that type has a writable instance dict, sandboxed code can do import posix; d = posix.DirEntry(); d.path = '/etc/passwd'; d.stat()/d.is_file() and those methods call host_fs::metadata/symlink_metadata on the real host path. With the default sandbox feature (seccomp off), this bypasses the VFS and leaks host filesystem metadata; please also stub or omit DirEntry and its host-backed methods under sandbox.
Useful? React with 👍 / 👎.
| "setuid", "setgid", "setreuid", "setregid", "setresuid", "setresgid", | ||
| "setgroups", "setsid", "setpgid", "setpgrp", "nice", "setpriority", | ||
| // durability + real process environment mutation | ||
| "fsync", "fdatasync", "setenv", "unsetenv", "putenv", |
There was a problem hiding this comment.
This sandbox override list covers durability operations like fsync/fdatasync, but sync is registered earlier in the same host_posix block and is not overwritten here. On Unix sandbox builds without the optional seccomp feature, import os; os.sync() still reaches the real host sync(2) syscall from untrusted code; with seccomp enabled it will kill the child instead of raising the intended sandbox stub. Please include sync in this sandbox stub list or compile its registration out.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ae205af33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # RPython-style sandbox: route every OS call through host_seam's marshalling | ||
| # trampoline instead of the real syscall. Implies host_env (same call surface, | ||
| # swapped bodies); the real-vs-trampoline choice lives only inside host_seam. | ||
| sandbox = ["host_env", "dep:pyre-sandbox"] |
There was a problem hiding this comment.
When the new sandbox feature enables host_env, read_source_line() still compiles its rustpython_host_env::fs::read_to_string(filename) branch and is not routed through the sandbox SourceProvider. A sandboxed program can set an arbitrary traceback filename, e.g. via compile(..., "/etc/passwd", "exec"), then raise an exception and have the child read and print host file lines on stderr; with seccomp enabled the same path would die on the raw open instead of producing a normal sandbox error. Please gate this host_env path out under sandbox or read traceback lines through the seam.
Useful? React with 👍 / 👎.
| "readlink", "scandir", "DirEntry", "statvfs", "fstatvfs", | ||
| // host process / environment information leaks | ||
| "getpid", "getppid", "uname", "getlogin", "getloadavg", | ||
| "getpriority", "times", "umask", |
There was a problem hiding this comment.
This sandbox override list still leaves the host_env pathconf/fpathconf/sysconf registrations live above, so sandboxed code can call, for example, posix.pathconf('/etc/passwd', name) and execute the host pathconf against the real filesystem instead of the controller VFS. Since these operations are not in the mediated ll_os surface, add them to the raising stubs (or route them through the controller) before exposing posix in sandbox builds.
Useful? React with 👍 / 👎.
| #[cfg(not(target_arch = "wasm32"))] | ||
| pyre_install_module!(pwd); | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| pyre_install_module!(grp); | ||
| #[cfg(unix)] | ||
| pyre_install_module!(resource); | ||
| #[cfg(unix)] | ||
| pyre_install_module!(fcntl); | ||
| #[cfg(unix)] | ||
| pyre_install_module!(syslog); | ||
| pyre_install_module!(select); | ||
| pyre_install_module!(termios); | ||
| pyre_install_module!(_socket); | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| pyre_install_module!(mmap); | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| pyre_install_module!(faulthandler); |
There was a problem hiding this comment.
faulthandler remains registered in sandbox builds even though its implementation calls host signal APIs and raw libc::write directly (enable/register install signal handlers, crash helpers raise/abort). With the default sandbox feature this gives untrusted code an unmediated host syscall surface, and with seccomp it will trap instead of behaving like the other omitted host-access modules; gate this registration with not(feature = "sandbox") or replace it with stubs.
Useful? React with 👍 / 👎.
| libc::syscall( | ||
| libc::SYS_close_range, | ||
| 3 as libc::c_long, | ||
| libc::c_uint::MAX as libc::c_long, | ||
| 0 as libc::c_long, | ||
| ); |
There was a problem hiding this comment.
On Linux kernels before close_range(2) exists, this syscall returns ENOSYS and the hook ignores it, so any non-CLOEXEC fd that the trusted controller had open can still cross into the untrusted child despite the comment promising only stdio survives. Please either loop over close(3..max) on ENOSYS/failure or return an error so the spawn fails closed on those kernels.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db151410fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "getpid", "getppid", "uname", "getlogin", "getloadavg", | ||
| "getpriority", "times", "umask", |
There was a problem hiding this comment.
In sandbox builds, the real registrations above still overwrite the initial no-op stubs for host-backed probes such as cpu_count/_cpu_count (host_posix::get_number_of_os_threads) and getgroups (host_posix::getgroups), but this final sandbox override list does not replace them. A sandboxed script can run import posix; posix.getgroups() or posix.cpu_count() and query host process/topology information directly instead of going through the controller, contrary to the host-info leak policy this block is enforcing; please add those live aliases here or compile their registrations out under sandbox.
Useful? React with 👍 / 👎.
| ) -> Result<pyre_object::PyObjectRef, crate::PyError> { | ||
| Err(crate::host_seam::stub("this locale function")) | ||
| } | ||
| for name in ["setlocale", "localeconv", "nl_langinfo"] { |
There was a problem hiding this comment.
This sandbox override only replaces setlocale, localeconv, and nl_langinfo, but sandbox implies host_env, so the strcoll and strxfrm registrations above still take their #[cfg(all(unix, feature = "host_env"))] arms and call rustpython_host_env::locale directly. In a sandbox build, _locale.strxfrm('x') or _locale.strcoll('a', 'b') can therefore reach the host locale machinery instead of observing the fixed C-locale/stub policy; include both names in this override or gate their real registrations out under sandbox.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Line 340 in 27552bc
Avoid canonicalizing script paths inside the sandbox
When a sandbox child is invoked in script mode with the seccomp feature enabled, the source read is now mediated, but this Path::canonicalize() still performs a real filesystem lookup after install_runtime_filter() has run; the allowlist in pyre-sandbox/src/seccomp.rs does not include file-open/stat syscalls, so pyre interact <seccomp-built-pyre> /tmp/app.py can be killed before user code runs. In non-seccomp sandbox builds it also derives sys.path[0] from the child's real host path rather than the controller VFS, so virtual script directories can be searched incorrectly; seed this from the virtual script parent instead of host canonicalization.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/cc9a0220e9987720b63065bfe74efb202ccb343f/pyrex/src/lib.rs#L326
In sandbox script mode this switches the source read to the seam, but the code immediately below still computes the script directory with Path::canonicalize() on the child’s host filesystem. For pyre interact --tmp DIR <pyre> sub/app.py, that raw host lookup runs after sandbox setup: with seccomp it can trap the child, and without seccomp it can seed sys.path from the controller’s real filesystem rather than the virtual /tmp. Please compute the script directory from the virtual path (or through the seam) instead of canonicalizing on the host.
https://github.com/youknowone/pyre/blob/cc9a0220e9987720b63065bfe74efb202ccb343f/pyre-interpreter/src/module/posix/interp_posix.rs#L3271-L3273
This sandbox deny-list still omits the Linux getresuid/getresgid registrations above, and sandbox implies host_env, so those functions remain backed by rustpython_host_env::posix in a sandbox build. A sandboxed script can call posix.getresuid()/posix.getresgid() and observe the real process IDs, while the mediated getuid/getgid path deliberately returns the virtual UID/GID 1000. Add these names to the sandbox stubs or route them through the controller.
https://github.com/youknowone/pyre/blob/cc9a0220e9987720b63065bfe74efb202ccb343f/pyre-interpreter/src/module/time/mod.rs#L57
This new sandbox guard only removes the clock setters; the clock_gettime, clock_gettime_ns, and clock_getres registrations immediately above remain live because sandbox builds also enable host_env. As a result time.clock_gettime(time.CLOCK_REALTIME) executes rustpython_host_env::time directly in the untrusted child instead of using the controller’s ll_time path (and seccomp allows these clock syscalls, while default sandbox has no seccomp), so the time surface is not fully mediated. Gate these getters out under sandbox or route them through host_seam.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Lines 388 to 392 in 01cf2f8
Avoid canonicalizing script paths in the sandbox
In sandbox script mode this runs after the seccomp filter is installed, so canonicalize() performs raw host filesystem lookups for the virtual script directory. For a sandboxed script such as /tmp/app.py, Linux/seccomp builds can be killed by the unmediated openat/readlink path, and builds with seccomp disabled seed sys.path[0] from the controller's real filesystem rather than the VFS. Use the virtual/lexical parent path or route this through the sandbox seam.
pyre/pyre/pyre-interpreter/src/module/posix/interp_posix.rs
Lines 3184 to 3186 in 01cf2f8
Gate residual UID probes out of sandbox builds
Because sandbox implies host_env, this getresuid registration is compiled in, and the final sandbox override list does not replace getresuid/getresgid (I checked the stub list below). In sandbox builds with seccomp disabled these calls expose the real process IDs instead of the virtual UID/GID used by getuid, while with seccomp enabled they trap and kill the child instead of raising the intended sandbox stub. Please add these names to the sandbox stub list or gate the real registrations with not(feature = "sandbox").
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let stream = TcpStream::connect((host, port)) | ||
| .map_err(|e| SandboxError::Os(e.raw_os_error().unwrap_or(libc::ECONNREFUSED)))?; |
There was a problem hiding this comment.
When --allow-net is enabled, the untrusted child can request os.open('tcp://host:port', ...) and the trusted controller blocks here in TcpStream::connect; the watchdog only kills the child and cannot interrupt this controller thread. A connect to an address that stalls (for example a blackholed IP) can therefore keep pyre interact --timeout 1 --allow-net ... hung until the OS connect timeout instead of the configured sandbox timeout. Use a nonblocking connect or connect_timeout tied to the remaining sandbox budget.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/607b527944bcb186f3bbbc4cd3752e87f79f8c62/pyrex/src/lib.rs#L391
When the sandbox child is invoked on a script path, the source read is now seam-backed, but this remaining canonicalize() still runs after the seccomp filter is installed. On Linux it issues raw filesystem syscalls for the virtual path (for example /tmp/app.py) and the child is SIGSYS-killed before the script can run; with seccomp disabled it consults the controller host filesystem instead of the VFS. Please derive the script directory as a virtual path or ask the controller rather than canonicalizing through std::fs.
https://github.com/youknowone/pyre/blob/607b527944bcb186f3bbbc4cd3752e87f79f8c62/pyrex/src/lib.rs#L407
When a sandbox child reaches the REPL path (pyre interact <pyre> with no -c/script, or -i after a command), repl_readline::read_basic_line still prints prompts with print!/stdout().flush() and reads from stdin() directly. In the child, fd 1 is the marshal request pipe, so the prompt bytes are parsed by the controller as protocol data and the session fails; fd 0 also bypasses the controller's mediated console input. Route the REPL through host_seam/controller I/O or disable REPL in sandbox mode.
https://github.com/youknowone/pyre/blob/607b527944bcb186f3bbbc4cd3752e87f79f8c62/pyre-sandbox/src/sandlib.rs#L302-L304
With --allow-net, if the sandbox opens a tcp:// fd and the peer accepts but does not send data, the trusted controller blocks here in TcpStream::read. The watchdog only kills the child and sets the cancellation flag; it cannot interrupt this blocking read, so pyre interact --timeout 1 --allow-net ... can hang until the remote peer sends or closes. Use nonblocking I/O/read timeouts tied to the remaining sandbox budget, or poll so controller-side socket reads observe cancellation.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 779c9e7782
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // VFS mediates it, the same channel module imports use; off sandbox | ||
| // it is a plain host read. | ||
| #[cfg(feature = "sandbox")] | ||
| let read = importing::read_source_to_string(Path::new(&path)); |
There was a problem hiding this comment.
When a sandbox child is run on a script path, this new seam read lets the script source load successfully, but the same RunMode::Script arm still immediately computes script_dir with Path::canonicalize() below. Because seccomp has already been installed, that raw host-filesystem canonicalization will SIGSYS on Linux (or, without seccomp, resolve against the controller's real filesystem and seed sys.path with a host path). Route the script directory through the virtual path/seam instead of letting the post-read startup path use std::fs.
Useful? React with 👍 / 👎.
| "getpid", "getppid", "uname", "getlogin", "getloadavg", | ||
| "getpriority", "times", "umask", "getgroups", "cpu_count", | ||
| "_cpu_count", |
There was a problem hiding this comment.
This sandbox override now covers some host identity leaks, but it still leaves the Linux host_posix registrations for getresuid and getresgid above live in sandbox builds. On non-seccomp/debug sandbox runs, untrusted code can call posix.getresuid()/posix.getresgid() and read the real process IDs instead of the controller's fixed virtual IDs; with seccomp enabled the same calls kill the child rather than raising the intended sandbox stub. Add these names to this override list alongside the other host process information probes.
Useful? React with 👍 / 👎.
| self.timeout_control.idle.store(false, Ordering::Relaxed); | ||
| r | ||
| } else { | ||
| read_upto(console.input, want) |
There was a problem hiding this comment.
When the sandboxed program requests os.read(0, n) while the controller's stdin is a non-tty pipe whose writer stays open without producing enough data, this branch blocks inside read_upto on the trusted controller's stdin. The watchdog can kill the child after --timeout, but it cannot interrupt this controller-side read, so the whole pyre interact --timeout ... process can hang until stdin writes/closes. Poll stdin nonblocking or check the cancellation/timeout state while servicing fd 0 reads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ce75a50a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let got = stream | ||
| .read(&mut buf) | ||
| .map_err(|_| SandboxError::Os(libc::EIO))?; |
There was a problem hiding this comment.
When --allow-net is used and the peer accepts a tcp:// connection but then stops sending data, the trusted controller blocks here in TcpStream::read; the watchdog can kill only the child, so pyre interact --timeout ... can hang in the parent indefinitely. The same issue exists for the socket write path below once the send buffer fills; use nonblocking I/O or read/write timeouts tied to the remaining sandbox budget.
Useful? React with 👍 / 👎.
| let cwd = sys_path_cwd(); | ||
| importing::init_sys_path(&cwd); | ||
| repl::run_repl(quiet, no_site); |
There was a problem hiding this comment.
In a sandbox child invoked without program args (or after -i), this enters repl::run_repl, but the REPL implementation uses raw std::io stdin/stdout and history filesystem access instead of the host seam. Under pyre interact <sandbox>, stdout is the marshal request pipe and stdin is the reply pipe, so the banner/prompt bytes corrupt the protocol and input bypasses the controller's fd-0 mediation; route REPL I/O through the sandbox seam or reject REPL mode in sandbox builds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d12c8f2987
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // guest-controlled path on the real filesystem. | ||
| "pathconf", "fpathconf", "sysconf", | ||
| // terminal / tty inspection + control | ||
| "tcgetpgrp", "tcsetpgrp", "get_terminal_size", |
There was a problem hiding this comment.
In sandbox builds the host_env block still registers ttyname above and calls host_posix::ttyname on the real fd, but this terminal/tty override list omits it. Because the child inherits a real fd 2 from the controller, posix.ttyname(2) can query the host terminal name when seccomp is disabled/debugged, and with seccomp enabled it can trap instead of raising the intended sandbox stub; add ttyname to this override list.
Useful? React with 👍 / 👎.
| "setuid", "setgid", "setreuid", "setregid", "setresuid", "setresgid", | ||
| "setgroups", "setsid", "setpgid", "setpgrp", "nice", "setpriority", |
There was a problem hiding this comment.
The sandbox override covers direct privilege/group mutators like setgroups, but the real initgroups registration above is still live in sandbox builds and calls host_posix::initgroups. A sandboxed script can therefore invoke the host group-database/setgroups path (or get SIGSYS under seccomp) instead of receiving the normal sandbox-unavailable error; include initgroups with the other privilege stubs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a260ecf1b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # RPython-style sandbox: route every OS call through host_seam's marshalling | ||
| # trampoline instead of the real syscall. Implies host_env (same call surface, | ||
| # swapped bodies); the real-vs-trampoline choice lives only inside host_seam. | ||
| sandbox = ["host_env", "dep:pyre-sandbox"] |
There was a problem hiding this comment.
Because sandbox implies host_env while _thread remains registered for sandbox builds, _thread.get_ident() takes the rustpython_host_env::thread::current_thread_id() branch and _thread.get_native_id() still calls the allowlisted host gettid syscall. A sandboxed script can therefore read real pthread/kernel thread IDs even though comparable process IDs are stubbed; gate both functions to the existing single-thread sentinel under feature = "sandbox".
Useful? React with 👍 / 👎.
| // file-descriptor duplication / pipes / ttys / cross-fd copy | ||
| "dup", "dup2", "dup3", "pipe", "pipe2", "openpty", "login_tty", | ||
| "sendfile", |
There was a problem hiding this comment.
In sandbox builds, the host_env block above still registers the real posix.set_inheritable implementation (host_posix::set_inheritable), but this final override list leaves that name live. Since the seccomp policy allows fcntl, posix.set_inheritable(1, True) mutates the real marshalling stdout fd instead of raising like the other fd-control operations; add set_inheritable to this sandbox stub list.
Useful? React with 👍 / 👎.
| // privilege / scheduling | ||
| "setuid", "setgid", "setreuid", "setregid", "setresuid", "setresgid", | ||
| "setgroups", "setsid", "setpgid", "setpgrp", "nice", "setpriority", |
There was a problem hiding this comment.
The real sched_get_priority_max/sched_get_priority_min registrations above are still active under feature = "sandbox", but this scheduling override list omits them. In a seccomp-enabled sandbox those calls reach unallowlisted host scheduler syscalls and terminate the child, while a non-seccomp/debug sandbox queries the host instead of raising the intended sandbox-unavailable error; include both names here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
pyre/pyre/pyre-interpreter/src/module/time/interp_time.rs
Lines 430 to 431 in 4c5ca10
Gate clock_getres consistently in sandbox builds
In a Unix sandbox build, the sandbox feature implies host_env, so time/mod.rs still enters its #[cfg(all(unix, feature = "host_env"))] init block and registers t::clock_getres. This cfg removes the function for the same build, so cargo build -p pyrex --features sandbox fails with an unresolved clock_getres; either keep this function compiled with a sandbox-safe body or also gate the registration out under sandbox.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let secs = | ||
| crate::host_seam::ops::clock().map_err(|e| crate::host_seam::seam_os_err(e, ""))?; | ||
| Ok((secs * 1_000_000_000.0) as i128) |
There was a problem hiding this comment.
In sandbox builds, time.process_time() and process_time_ns() take this branch and decode ll_time_clock, but the controller’s do_clock is the RPython wall-clock elapsed-since-first-call emulation rather than CPU time. A sandboxed program that sleeps or blocks will therefore see process_time() advance by wall time, unlike the non-sandbox branch and the Python API contract; route this to a CPU-time source or avoid using ll_time_clock for process_time.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/1274919c560b58ab431ee8900f687db536b7d3ec/pyrex/src/lib.rs#L108-L110
When the sandbox child receives interact as its first forwarded argument, e.g. pyre interact <sandbox-pyre> interact /bin/true, this branch selects RunMode::Interact inside the untrusted process. real_main then treats it as is_interact, skips the sandbox seccomp install, and the match below calls the trusted controller path, which can spawn host executables outside the VFS; the sandbox build should compile out or reject this subcommand for child invocations.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
os.open ORs O_CLOEXEC (unix) / O_NOINHERIT (Windows) into flags so the descriptor is not inherited across exec, matching interp_posix.py. Assisted-by: Claude
Add the `sandbox` compile-out surface (issue #285): a `pyre-sandbox` crate (rmarshal codec, protocol, VFS, controller, client trampoline, seccomp allowlist, e2e test) and the `pyre interact` controller subcommand, driven through a `host_seam` OS-call seam in pyre-interpreter that routes the `ll_os.*`/`ll_time.*` surface, program and diagnostic stdio, import source loading, and entropy through the controller under `--features sandbox`. Gate stray raw host-access sites out of the sandbox build and enforce it with a sandbox-scoped clippy `disallowed-methods/types` fence (ci/clippy-sandbox) run as a CI job. Install the seccomp BPF backstop by default on Linux (PYRE_SANDBOX_NO_SECCOMP escape). Port VirtualizedSocketProc `tcp://` mediation as opt-in (--allow-net) and add `--log`/`--heapsize` CLI parity. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/b6466916c559b148b9c078767b6ba7a19d183bf0/pyre-interpreter/src/module/time/mod.rs#L57-L58
In a sandbox build this new cfg only removes clock_settime{,_ns}, but the clock_gettime, clock_gettime_ns, and clock_getres registrations immediately above still compile because sandbox enables host_env. Those functions call rustpython_host_env::time::{clock_gettime,clock_getres} directly, and the seccomp policy allowlists the raw clock syscalls, so sandboxed code such as time.clock_gettime(time.CLOCK_MONOTONIC) can query host clocks outside the controller/ll_time mediation. Please stub these names under sandbox or route them through host_seam.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Closes #285.
Implements an RPython/PyPy-style sandbox for pyre. pyre's interpreter is never
translated to a standalone binary (there is no genc backend), so the faithful
analog of RPython's
translate.py --sandboxisrustc+ cargo as the"translation", and
#[cfg(feature = "sandbox")]as the compile-time rewrite:the
sandboxfeature compiles out the real-syscall implementations andcompiles in marshalling trampolines, reproducing RPython's security property
— syscall code unreachable from untrusted Python — at compile time.
Untrusted Python runs as
pyre --features sandbox, driven by a trustedpyre interactcontroller over the rmarshal wire protocol and a virtualfilesystem. All OS access is mediated; the host is never touched directly.
Architecture
pyre-sandboxcrate — the single live home shared by the untrusted clientand the trusted controller: byte-exact
rmarshalwire codec,protocol,trait-based
vfs,clienttrampoline half, and thesandlib/controllerports of
sandlib.py+pypy_interact.py.pyre interact(inpyrex) drivesit. Deps:
libc+stdonly.host_seam(interpreter) — the single indirection to the OS. Two#[cfg]-selected impls:RealHost(today's libc/std bodies, off sandbox) andTrampolineHost(marshals to the controller, on sandbox). Builtins arenon-capturing
fnpointers, so the seam is reached by module path throughhost_seam::ops::*.Defense layers
posix/timeOS surface gothrough
host_seam::ops::*(marshalled to the controller).raise;
_socket/_ctypes/_posixsubprocess/_signal/fcntl/… modules arecompiled out entirely. Import resolution is routed through a seam-backed
SourceProvider(nostd::fs), soimportcannot read arbitrary host files.host_seam::sysfacade — under sandbox,sysre-exportsonly libc TYPES, CONSTANTS and curated PURE functions; the mediated modules
name libc as
crate::host_seam::sys, so any direct syscall call left outsidethe seam is a compile error. A green
cargo build --features sandboxisthe proof.
filter after startup and before untrusted code: a curated allowlist of
host-neutral runtime syscalls (memory, signals, time, I/O on the open
marshalling fds), everything else
SECCOMP_RET_KILL_PROCESS. This is theanalog of RPython's
os_level_sandboxingand covers what the source-levelseam cannot: the linked
host_envcrate, std, or a syscall reached by amemory-safety exploit.
A 2026-06-27 escape audit (workflow, adversarially verified) found 40 reachable
escapes in an earlier import-free state (import spawned
python3, read hostfiles via
HostFsProvider,sendfile/readlink/scandir/… unstubbed). All areclosed here and locked by the e2e regression guard.
Verification
pyre/check.py --backend dynasm: 160/160 ×2 (default feature set, sandboxOFF — all changes are sandbox-cfg or
not(sandbox), so the default build isbyte-identical).
cargo build --release -p pyrex --bin pyre --features sandbox: green (thefails-closed facade proof).
pyre-sandbox/tests/e2e_interact.rs, port oftest_pypy_interact.py):2/2 — virtual-FS read,
/etc/passwd+ write attempts blocked, and aregression guard asserting the audited escape surface stays closed.
sandbox-build(ubuntu): builds the sandbox binary and runs thee2e — and, on Linux, validates the seccomp allowlist (a SIGSYS-killed child
surfaces as a failed run).
Caveats
cargo check --target x86_64-unknown-linux-gnu) but not runtime-validated locally (dev ismacOS, where seccomp is
cfg-d out); the Linux CI e2e is the runtime check. Amissing runtime syscall fails safe (over-restrictive: the child is killed, not
escaped). It is default-on under
sandboxon Linux;PYRE_SANDBOX_NO_SECCOMPbypasses it for direct-run debugging (the controller
env_clears the child, soit cannot be disabled through the real path).
sandbox = ["host_env"]is deliberate: thehost_envcrate stays linked butis unreachable from untrusted Python (and killed by seccomp if reached). The
majit-translatesandbox shells +rtyper.rs:734hook stay as inertRPython-parity mirrors (there is no genc backend to attach to).
🤖 Generated with Claude Code
Summary by CodeRabbit
pyre interactto run an external program inside a controlled sandbox with optional virtual filesystem roots, optional library root, and a timeout.sys.executable) with a virtualized environment and console behavior.