initial sandbox - #7035
Conversation
Sorry, something went wrong.
📝 Walkthrough
WalkthroughAdds a new Cargo feature flag host_env; gates many host-specific stdlib modules, file-execution, signal and stdio behavior behind it; centralizes OSError construction via an OSErrorBuilder path; introduces SandboxStdio for non-host builds; adds CI sandbox checks and a sandbox smoke test. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / Binary
participant VM as VirtualMachine
participant FS as Filesystem
participant Loader as Importlib/Loader
participant StdLib as stdlib modules
Note over CLI,VM: host_env enabled?
CLI->>VM: run_any_file(path) (host_env)
VM->>FS: stat/read path
FS-->>VM: path type (.py / dir / .pyc)
VM->>Loader: choose loader / set_main_loader
Loader-->>VM: code object
VM->>StdLib: initialize modules (host_env gated)
VM->>VM: execute code
VM-->>CLI: exit status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 2 | ❌ 1 ❌ Failed checks (1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin sandbox |
Sorry, something went wrong.
|
|
||
| #[pymodule] | ||
| mod builtins { | ||
| use crate::py_io; |
There was a problem hiding this comment.
useless diff
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)crates/vm/src/exceptions.rs (1)2148-2158: ⚠️ Potential issue | 🟡 Minor
Setter allows storing sentinel value -1, causing subsequent access to raise AttributeError.
If Python code sets exc.characters_written = -1, the setter stores it without validation. The getter then treats -1 as a sentinel for "not set" and raises AttributeError. This diverges from CPython, where any explicitly set integer value (including -1) is retrievable as-is. The setter should validate that input values differ from the sentinel before storing.
In `@crates/vm/src/lib.rs`: - Around line 70-71: The import of the ospath module in crates/vm/src/stdlib/io.rs must be conditionally compiled to match the module declaration (pub mod ospath) which is #[cfg(feature = "host_env")] in lib.rs; locate the import statement that references ospath (the use/mod of ospath around line ~5059 in stdlib/io.rs) and prefix it with #[cfg(feature = "host_env")] so the import is only compiled when the host_env feature is enabled. In `@crates/vm/src/stdlib/mod.rs`: - Around line 101-104: The two cfg attributes both enable posix::module_def(ctx) on WASI causing a duplicate; update the second attribute so it excludes WASI (e.g. change #[cfg(all(feature = "host_env", not(any(unix, windows)))] to exclude target_os = "wasi", such as #[cfg(all(feature = "host_env", not(any(unix, windows, target_os = "wasi"))))]) so posix::module_def(ctx) is only added once. In `@crates/vm/src/stdlib/sys.rs`: - Around line 122-132: The readline implementation currently ignores the OptionalArg<isize> _size; update TextIOBase::readline (function readline in sys.rs) to explicitly reject non-default size values instead of ignoring them: before reading from stdin, check if _size is Present (i.e., Some) and if so return an appropriate VM error (e.g., a NotImplementedError or ValueError created via VirtualMachine like vm.new_not_implemented or vm.new_value_error) with a clear message such as "readline with size not supported in sandbox"; otherwise proceed with the existing fd check and stdin.read_line flow. - Around line 108-120: The write method currently swallows IO errors; change the write implementation in the Sys* struct's #[pymethod] fn write(&self, s: PyStrRef, vm: &VirtualMachine) -> PyResult<usize> to propagate errors from stdout/stderr by replacing the `let _ = ...` ignores with `.write_all(bytes).map_err(|e| vm.new_os_error(e.to_string()))?` so any write failures become Py OSError; likewise implement a #[pymethod] fn flush(&self, vm: &VirtualMachine) -> PyResult<()> (add the vm parameter) and call the appropriate stream's `flush().map_err(|e| vm.new_os_error(e.to_string()))?` instead of ignoring errors, returning Ok(()) on success.
src/lib.rs (1)extra_tests/snippets/sandbox_smoke.py (1)176-189: Sandbox fallback design is sound but loses OS error detail.
The sandbox path correctly keeps filesystem I/O in the binary (trusted host) while the VM only sees source strings. Two minor observations:
Optional: preserve errno in sandbox error path
- std::fs::read_to_string assumes UTF-8, which is the Python 3 default source encoding — fine for the common case.
- Line 187: err.to_string() discards the original io::ErrorKind/errno. Consider preserving it if the VM's new_os_error supports it, though this is low priority for a sandbox fallback.
- Err(err) => Err(vm.new_os_error(err.to_string())), + Err(err) => { + let msg = format!("Can't open file '{}': {}", path, err); + Err(vm.new_os_error(msg)) + }crates/vm/src/vm/python_run.rs (2)19-21: Minor: sys.stderr is not tested for writability.
stdin/stdout are tested but stderr is mentioned in the docstring (line 4) and available via SandboxStdio. Consider adding assert sys.stderr.writable() for completeness.
Optional: add stderr assertionassert sys.stdout.writable() assert sys.stdin.readable() +assert sys.stderr.writable() assert sys.stdout.fileno() == 1crates/vm/src/exceptions.rs (4)149-155: Read::read may short-read even on regular files.
file.read(&mut buf)? is not guaranteed to fill the 2-byte buffer in a single call. While exceedingly rare for regular files, Read::read_exact would be the robust choice here.
Suggested fix- use std::io::Read; - if file.read(&mut buf)? != 2 { + use std::io::Read; + if file.read_exact(&mut buf).is_err() {
109-112: Commented-out deprecation on run_script.
The #[deprecated] attribute is commented out. If this is intentional for now, consider adding a brief // TODO explaining when the deprecation should be activated. Otherwise it's easy for this to be forgotten.
1307-1345: build() relies on expect() for infallible paths — verify they truly can't fail.
Lines 1338, 1341, and 1343 each call .expect("new_os_error usage error"). While py_new always returns Ok(...) for the current implementation and into_ref_with_type should succeed when using the same exc_type, slot_init is more complex and could theoretically fail (e.g., on a subclass that overrides __init__). If any of these paths fail, the panic will be hard to diagnose.
Consider whether slot_init could fail for exotic subclasses and, if so, whether returning a PyResult from build would be more appropriate. If these are truly infallible, a brief comment on why would help future maintainers.
1320-1335: On non-Windows, a dummy None is injected at position 3 (winerror slot) into the args vec.
When errno is Some, the args vector is always 5 elements: [errno, strerror, filename, winerror_or_none, filename2]. On non-Windows, winerror is vm.ctx.none(). This means slot_init receives 5 args on all platforms — which works because slot_init only processes args[3] inside a #[cfg(windows)] block. However, this couples the builder's arg layout to slot_init's conditional parsing.
This is fine as-is, but a brief comment noting that the 4th position is always None on non-Windows (and ignored by slot_init) would prevent confusion.
1375-1385: libc::strerror is not guaranteed thread-safe.
On Unix, libc::strerror may return a pointer to a shared static buffer (for unknown errno values). While most modern libc implementations use static string literals for known errno values, the general case is not thread-safe. Since the result is immediately copied via to_string_lossy().into_owned(), the risk is low, but it's worth noting.
If thread-safety is a concern, strerror_r (POSIX) or strerror_l would be safer alternatives, though they come with their own portability complications.
1926-1926: Minor: Cloning the atomic ref just to check for None.
exc.filename.to_owned() clones the PyObjectRef behind the atomic ref only to check is_none. Consider using a read/deref approach if available to avoid the unnecessary clone:
-let has_filename = exc.filename.to_owned().filter(|f| !vm.is_none(f)).is_some(); +let has_filename = exc.filename.deref().is_some_and(|f| !vm.is_none(f));(Depends on whether deref() returns a suitable reference — adjust as needed for the PyAtomicRef API.)
Sorry, something went wrong.
| #[pymethod] | ||
| fn readline(&self, _size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> { | ||
| if self.fd != 0 { | ||
| return Err(vm.new_os_error("not readable".to_owned())); | ||
| } | ||
| let mut line = String::new(); | ||
| std::io::stdin() | ||
| .read_line(&mut line) | ||
| .map_err(|e| vm.new_os_error(e.to_string()))?; | ||
| Ok(line) | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
🧩 Analysis chain🌐 Web query:
CPython TextIOBase.readline(size) semantics behavior documentation
💡 Result:
io.TextIOBase.readline(size=-1) (text mode) semantics (CPython)
- What it reads: Reads from the current position up to and including the next line ending, or until EOF if no line ending is found. It returns one str. If already at EOF, it returns "" (empty string). [1]
- size meaning: If size is provided, at most size characters are read (so you may get a partial line that does not end in a newline). [1]
- Newline handling: What counts as a “newline” depends on how the text stream was opened (the newline= argument to open() controls which line terminators are recognized / translated). [1]
Contrast: binary streams (BufferedReader.readline)
- In binary mode, readline(size) reads at most size bytes, and line termination is based on b"\n". [2]
Sources:
[1] Python io docs (TextIOBase.readline) (docs.python.org)
[2] Python io docs (IOBase.readline / binary-vs-text note) (docs.python.org)
Citations:
- 1: https://docs.python.org/uk/3.12/library/io.html?utm_source=openai
- 2: https://docs.python.org/es/3.12/library/io.html?utm_source=openai
readline silently ignores the size parameter, diverging from CPython's TextIOBase.readline(size) behavior.
In CPython, the size parameter controls the maximum number of characters to read. When provided, it may result in a partial line without a newline. The current implementation ignores this parameter regardless of its value, making behavior unpredictable for callers expecting standard TextIOBase semantics.
For a sandbox environment where full support may not be feasible, explicitly reject non-default size values rather than silently ignoring them:
Suggested fix- fn readline(&self, _size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> {
+ fn readline(&self, size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> {
if self.fd != 0 {
return Err(vm.new_os_error("not readable".to_owned()));
}
+ if let OptionalArg::Present(size) = size {
+ if size >= 0 {
+ return Err(vm.new_not_implemented_error(
+ "size argument is not supported in sandbox stdio",
+ ));
+ }
+ }
let mut line = String::new();
std::io::stdin()
.read_line(&mut line)
.map_err(|e| vm.new_os_error(e.to_string()))?;
Ok(line)
}‼️ 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.
| #[pymethod] | |
| fn readline(&self, _size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> { | |
| if self.fd != 0 { | |
| return Err(vm.new_os_error("not readable".to_owned())); | |
| } | |
| let mut line = String::new(); | |
| std::io::stdin() | |
| .read_line(&mut line) | |
| .map_err(|e| vm.new_os_error(e.to_string()))?; | |
| Ok(line) | |
| } | |
| #[pymethod] | |
| fn readline(&self, size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> { | |
| if self.fd != 0 { | |
| return Err(vm.new_os_error("not readable".to_owned())); | |
| } | |
| if let OptionalArg::Present(size) = size { | |
| if size >= 0 { | |
| return Err(vm.new_not_implemented_error( | |
| "size argument is not supported in sandbox stdio", | |
| )); | |
| } | |
| } | |
| let mut line = String::new(); | |
| std::io::stdin() | |
| .read_line(&mut line) | |
| .map_err(|e| vm.new_os_error(e.to_string()))?; | |
| Ok(line) | |
| } |
In `@crates/vm/src/stdlib/sys.rs` around lines 122 - 132, The readline implementation currently ignores the OptionalArg<isize> _size; update TextIOBase::readline (function readline in sys.rs) to explicitly reject non-default size values instead of ignoring them: before reading from stdin, check if _size is Present (i.e., Some) and if so return an appropriate VM error (e.g., a NotImplementedError or ValueError created via VirtualMachine like vm.new_not_implemented or vm.new_value_error) with a clear message such as "readline with size not supported in sandbox"; otherwise proceed with the existing fd check and stdin.read_line flow.
Sorry, something went wrong.
ShaharNaveh
left a comment
There was a problem hiding this comment.
I like the idea a lot
Sorry, something went wrong.
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)crates/vm/src/vm/mod.rs (1)326-394: ⚠️ Potential issue | 🔴 Critical
Fix conflicting cfg conditions for make_stdio branches.
The host_env and stdio features are independent in Cargo.toml (neither depends on the other), but the three cfg branches are not mutually exclusive. When host_env is enabled and stdio is disabled, both the first branch (#[cfg(feature = "host_env")]) and the third branch (#[cfg(not(feature = "stdio"))]) match simultaneously, causing a duplicate make_stdio binding and a compile error.
Change the third condition from #[cfg(not(feature = "stdio"))] to #[cfg(all(not(feature = "host_env"), not(feature = "stdio")))] to ensure all three branches are truly mutually exclusive.
In `@crates/vm/src/stdlib/sys.rs`: - Around line 126-141: The readline implementation in SandboxStdio::readline treats the size parameter as a byte index and calls line.truncate(size as usize), which can panic on UTF‑8 boundaries for non‑ASCII input; change the truncation to operate on character count instead of raw bytes—compute the byte boundary for the first N characters (size) or rebuild the string with line.chars().take(size as usize).collect(), then return that safely truncated String while keeping the existing checks (handle size <= 0 and OptionalArg unwrap as before).
crates/vm/src/exceptions.rs (1)1375-1385: libc::strerror is not guaranteed thread-safe on all platforms.
On some platforms, strerror returns a pointer to a shared static buffer. While glibc uses thread-local storage, POSIX doesn't guarantee thread-safety. The immediate .to_string_lossy().into_owned() conversion minimizes the risk window, but strerror_r (or Rust's std::io::Error::from_raw_os_error(errno).to_string()) would be a safer alternative.
Sorry, something went wrong.
| #[pymethod] | ||
| fn readline(&self, size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> { | ||
| if self.fd != 0 { | ||
| return Err(vm.new_os_error("not readable".to_owned())); | ||
| } | ||
| let size = size.unwrap_or(-1); | ||
| if size == 0 { | ||
| return Ok(String::new()); | ||
| } | ||
| let mut line = String::new(); | ||
| std::io::stdin() | ||
| .read_line(&mut line) | ||
| .map_err(|e| vm.new_os_error(e.to_string()))?; | ||
| if size > 0 { | ||
| line.truncate(size as usize); | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain🌐 Web query:
Rust String::truncate UTF-8 boundary panic behavior
💡 Result:
String::truncate(new_len) truncates by byte index, but it must leave the String as valid UTF-8.
Panic behavior
truncate panics at runtime if:
- new_len is not on a UTF-8 code point / char boundary (i.e., it points into the middle of a multi-byte character). (web.mit.edu)
- (More generally for Rust string boundary-checked operations) the index is past the end / not a valid boundary; Rust enforces these checks to prevent creating invalid UTF-8. (doc.rust-lang.org)
How to avoid the panic
- Check first: s.is_char_boundary(new_len) (method on str). (stackoverflow.com)
- Or compute a safe boundary (e.g., walk char_indices() to the last boundary ≤ new_len) and truncate there. (gist.github.com)
Citations:
- 1: https://web.mit.edu/rust-lang_v1.26.0/arch/amd64_ubuntu1404/share/doc/rust/html/std/string/struct.String.html?utm_source=openai
- 2: https://doc.rust-lang.org/core/primitive.str.html?utm_source=openai
- 3: https://stackoverflow.com/questions/45837114/why-does-a-truncated-string-rust-print-as-an-empty-pair-of-parenthesis?utm_source=openai
- 4: https://gist.github.com/dginev/f6da5e94335d545e0a7b?utm_source=openai
🏁 Script executed:
# Check the actual file and surrounding context
cat -n crates/vm/src/stdlib/sys.rs | sed -n '120,145p'Repository: RustPython/RustPython
Length of output: 1072
Avoid UTF‑8 boundary panics when honoring size in SandboxStdio.readline.
String::truncate expects a byte index on a UTF‑8 char boundary; using the raw size parameter as a byte index will panic on non‑ASCII input. The method interprets size as a character count, but the code treats it as a byte index directly.
🔧 Suggested fix- if size > 0 {
- line.truncate(size as usize);
- }
+ if size > 0 {
+ let max_chars = size as usize;
+ if let Some((idx, _)) = line.char_indices().nth(max_chars) {
+ line.truncate(idx);
+ }
+ }‼️ 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.
| #[pymethod] | |
| fn readline(&self, size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> { | |
| if self.fd != 0 { | |
| return Err(vm.new_os_error("not readable".to_owned())); | |
| } | |
| let size = size.unwrap_or(-1); | |
| if size == 0 { | |
| return Ok(String::new()); | |
| } | |
| let mut line = String::new(); | |
| std::io::stdin() | |
| .read_line(&mut line) | |
| .map_err(|e| vm.new_os_error(e.to_string()))?; | |
| if size > 0 { | |
| line.truncate(size as usize); | |
| } | |
| #[pymethod] | |
| fn readline(&self, size: OptionalArg<isize>, vm: &VirtualMachine) -> PyResult<String> { | |
| if self.fd != 0 { | |
| return Err(vm.new_os_error("not readable".to_owned())); | |
| } | |
| let size = size.unwrap_or(-1); | |
| if size == 0 { | |
| return Ok(String::new()); | |
| } | |
| let mut line = String::new(); | |
| std::io::stdin() | |
| .read_line(&mut line) | |
| .map_err(|e| vm.new_os_error(e.to_string()))?; | |
| if size > 0 { | |
| let max_chars = size as usize; | |
| if let Some((idx, _)) = line.char_indices().nth(max_chars) { | |
| line.truncate(idx); | |
| } | |
| } |
In `@crates/vm/src/stdlib/sys.rs` around lines 126 - 141, The readline implementation in SandboxStdio::readline treats the size parameter as a byte index and calls line.truncate(size as usize), which can panic on UTF‑8 boundaries for non‑ASCII input; change the truncation to operate on character count instead of raw bytes—compute the byte boundary for the first N characters (size) or rebuild the string with line.chars().take(size as usize).collect(), then return that safely truncated String while keeping the existing checks (handle size <= 0 and OptionalArg unwrap as before).
Sorry, something went wrong.
Introduce a `host_env` feature flag that gates all host environment access (filesystem, network, signals, processes). When disabled, the VM operates in sandbox mode: - _io module always available; FileIO gated by host_env - SandboxStdio provides lightweight stdin/stdout/stderr via Rust std::io - BytesIO/StringIO/BufferedIO/TextIOWrapper work without host_env - open() returns UnsupportedOperation in sandbox - stdlib modules (os, socket, signal, etc.) gated by host_env - CI checks both host_env ON and OFF builds
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agentsIn @.github/workflows/ci.yaml: - Around line 156-167: Update the CI job to build with the compiler feature and run sandbox snippets using cargo run instead of invoking the binary directly: add the compiler feature to the build line (the step that currently runs cargo build --no-default-features --features stdlib,importlib,stdio,encodings,freeze-stdlib) so it becomes consistent with the check steps, and replace the two direct binary executions (target/debug/rustpython extra_tests/snippets/sandbox_smoke.py and target/debug/rustpython extra_tests/snippets/stdlib_re.py) in the "sandbox smoke test" step with cargo run --bin rustpython -- <snippet-path> (run in debug mode) to follow the extra_tests/snippets guideline and ensure faster, reproducible builds. In `@crates/vm/src/exceptions.rs`: - Around line 1354-1398: The impl of ToOSErrorBuilder for std::io::Error currently always calls OSErrorBuilder::with_errno(errno, msg, vm) even when posix_errno() returns 0; change to check the errno returned by posix_errno() and only call with_errno when errno > 0, otherwise call OSErrorBuilder::with_subtype(py_exception_type_for_os_error, None, msg, vm) (preserving the msg) so we don't emit OSError(errno=0); update the to_os_error_builder function around the creation of builder (the let mut builder = ... and subsequent #[cfg(windows)] winerror handling) to branch on errno accordingly while keeping the winerror attachment when available.
crates/vm/src/signal.rs (1)82-83: Nit: inconsistent #[cfg] style with set_interrupt_ex below.
Lines 82–83 use two stacked #[cfg(...)] attributes, while line 103 combines them into a single #[cfg(all(...))]. Both are semantically equivalent, but using a consistent style improves readability.
♻️ Suggested consolidation-#[cfg(unix)] -#[cfg(feature = "host_env")] +#[cfg(all(unix, feature = "host_env"))] pub(crate) fn clear_after_fork() {
Sorry, something went wrong.
| - name: check compilation without host_env (sandbox mode) | ||
| run: | | ||
| cargo check -p rustpython-vm --no-default-features --features compiler | ||
| cargo check -p rustpython-stdlib --no-default-features --features compiler | ||
| cargo build --no-default-features --features stdlib,importlib,stdio,encodings,freeze-stdlib | ||
| if: runner.os == 'Linux' | ||
|
|
||
| - name: sandbox smoke test | ||
| run: | | ||
| target/debug/rustpython extra_tests/snippets/sandbox_smoke.py | ||
| target/debug/rustpython extra_tests/snippets/stdlib_re.py | ||
| if: runner.os == 'Linux' |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
🧩 Analysis chain🏁 Script executed:
# Check the actual file and content at the specified lines
head -170 .github/workflows/ci.yaml | tail -25Repository: RustPython/RustPython
Length of output: 1256
🏁 Script executed:
# Search for other uses of extra_tests/snippets in the codebase
rg "extra_tests/snippets" --type yaml --type sh -A 2 -B 2Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check Cargo.toml to understand compiler feature
rg "compiler" Cargo.toml -A 2 -B 2 | head -50Repository: RustPython/RustPython
Length of output: 681
🏁 Script executed:
# Look for how cargo run is typically used in RustPython workflows
rg "cargo run" .github/workflows/ -A 1 -B 1 | head -60Repository: RustPython/RustPython
Length of output: 1057
🏁 Script executed:
# Search for how compiler feature relates to .py execution/compilation
rg "compiler" crates/vm -A 2 -B 2 | head -40Repository: RustPython/RustPython
Length of output: 2602
🏁 Script executed:
# Check if there's any documentation or tests about sandbox mode and compiler feature
rg "sandbox" --type rust --type py -B 3 -A 3 | head -60Repository: RustPython/RustPython
Length of output: 3899
🏁 Script executed:
# Check the feature definitions in Cargo.toml for rustpython-vm
rg "\[features\]" -A 30 rustpython-vm/Cargo.tomlRepository: RustPython/RustPython
Length of output: 129
🏁 Script executed:
# Look for any existing uses of cargo run with sandbox/no-default-features
rg "cargo.*run.*no-default-features" -A 1 -B 1Repository: RustPython/RustPython
Length of output: 231
🏁 Script executed:
# Check the main binary's implementation to see what features it uses for .py execution
cat -n src/lib.rs | head -100Repository: RustPython/RustPython
Length of output: 4289
🏁 Script executed:
# Look for feature guards around .py compilation/execution
rg "feature.*compiler\|feature.*parser" src/lib.rs -B 2 -A 2Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check what features are needed for the main binary
rg "\[features\]" Cargo.toml -A 50 | head -80Repository: RustPython/RustPython
Length of output: 1885
🏁 Script executed:
# Check the actual file reading logic in sandbox mode
cat -n src/lib.rs | sed -n '100,200p'Repository: RustPython/RustPython
Length of output: 4296
🏁 Script executed:
# Look for how compiler feature is gated in the codebase for .py execution
rg "cfg.*compiler" src/ -B 2 -A 2 | head -50Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check if there's a feature guard around compilation in the interpreter
ast-grep --pattern 'fn.*compile($_)' | head -30Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check if other CI workflows use cargo run vs direct binary execution
rg "cargo run" .github/workflows/ci.yaml -B 3 -A 3Repository: RustPython/RustPython
Length of output: 979
🏁 Script executed:
# Look for other direct binary execution in the CI
rg "target/debug" .github/workflows/ci.yaml -B 2 -A 2Repository: RustPython/RustPython
Length of output: 286
🏁 Script executed:
# Check if there's any documentation or rationale for cargo run vs direct binary
find . -name "*.md" -o -name "*.txt" | xargs grep -l "cargo run\|direct binary" 2>/dev/null | head -5Repository: RustPython/RustPython
Length of output: 238
🏁 Script executed:
# Verify the guideline applies to CI/workflows context by checking AGENTS.md
cat -n AGENTS.md | grep -A 5 -B 5 "extra_tests/snippets"Repository: RustPython/RustPython
Length of output: 1029
🏁 Script executed:
# Verify the exact feature syntax by checking if stdlib,importlib,stdio,encodings,freeze-stdlib,compiler is valid
rg "features.*stdlib.*importlib.*stdio" Cargo.toml -A 1 -B 1Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check if there are any other uses of cargo run with multiple features in CI
rg "cargo run.*--features.*," .github/workflows/ -A 1 -B 1Repository: RustPython/RustPython
Length of output: 47
Run sandbox snippets via cargo run instead of direct binary execution.
The smoke test should follow the project guideline for extra_tests/snippets/** tests: use cargo run with debug mode for faster compilation. Additionally, add the compiler feature to the build step for consistency with the check step above it.
🔧 Suggested update - name: check compilation without host_env (sandbox mode)
run: |
cargo check -p rustpython-vm --no-default-features --features compiler
cargo check -p rustpython-stdlib --no-default-features --features compiler
- cargo build --no-default-features --features stdlib,importlib,stdio,encodings,freeze-stdlib
+ cargo build --no-default-features --features stdlib,importlib,stdio,encodings,freeze-stdlib,compiler
if: runner.os == 'Linux'
- name: sandbox smoke test
run: |
- target/debug/rustpython extra_tests/snippets/sandbox_smoke.py
- target/debug/rustpython extra_tests/snippets/stdlib_re.py
+ cargo run --no-default-features --features stdlib,importlib,stdio,encodings,freeze-stdlib,compiler -- extra_tests/snippets/sandbox_smoke.py
+ cargo run --no-default-features --features stdlib,importlib,stdio,encodings,freeze-stdlib,compiler -- extra_tests/snippets/stdlib_re.py
if: runner.os == 'Linux'In @.github/workflows/ci.yaml around lines 156 - 167, Update the CI job to build with the compiler feature and run sandbox snippets using cargo run instead of invoking the binary directly: add the compiler feature to the build line (the step that currently runs cargo build --no-default-features --features stdlib,importlib,stdio,encodings,freeze-stdlib) so it becomes consistent with the check steps, and replace the two direct binary executions (target/debug/rustpython extra_tests/snippets/sandbox_smoke.py and target/debug/rustpython extra_tests/snippets/stdlib_re.py) in the "sandbox smoke test" step with cargo run --bin rustpython -- <snippet-path> (run in debug mode) to follow the extra_tests/snippets guideline and ensure faster, reproducible builds.
Sorry, something went wrong.
| impl ToOSErrorBuilder for std::io::Error { | ||
| fn to_os_error_builder(&self, vm: &VirtualMachine) -> OSErrorBuilder { | ||
| use crate::common::os::ErrorExt; | ||
|
|
||
| let errno = self.posix_errno(); | ||
| #[cfg(windows)] | ||
| let msg = 'msg: { | ||
| // Use C runtime's strerror for POSIX errno values. | ||
| // For Windows-specific error codes, fall back to FormatMessage. | ||
| const MAX_POSIX_ERRNO: i32 = 127; | ||
| if errno > 0 && errno <= MAX_POSIX_ERRNO { | ||
| let ptr = unsafe { libc::strerror(errno) }; | ||
| if !ptr.is_null() { | ||
| let s = unsafe { core::ffi::CStr::from_ptr(ptr) }.to_string_lossy(); | ||
| if !s.starts_with("Unknown error") { | ||
| break 'msg s.into_owned(); | ||
| } | ||
| } | ||
| } | ||
| self.to_string() | ||
| }; | ||
| #[cfg(unix)] | ||
| let msg = { | ||
| let ptr = unsafe { libc::strerror(errno) }; | ||
| if !ptr.is_null() { | ||
| unsafe { core::ffi::CStr::from_ptr(ptr) } | ||
| .to_string_lossy() | ||
| .into_owned() | ||
| } else { | ||
| vec![strerror.to_pyobject(vm)] | ||
| }; | ||
| self.to_string() | ||
| } | ||
| }; | ||
| #[cfg(not(any(windows, unix)))] | ||
| let msg = self.to_string(); | ||
|
|
||
| #[allow(unused_mut)] | ||
| let mut builder = OSErrorBuilder::with_errno(errno, msg, vm); | ||
|
|
||
| let payload = PyOSError::py_new(&exc_type, args.clone().into(), vm) | ||
| .expect("new_os_error usage error"); | ||
| let os_error = payload | ||
| .into_ref_with_type(vm, exc_type) | ||
| .expect("new_os_error usage error"); | ||
| PyOSError::slot_init(os_error.as_object().to_owned(), args.into(), vm) | ||
| .expect("new_os_error usage error"); | ||
| os_error | ||
| #[cfg(windows)] | ||
| if let Some(winerror) = self.raw_os_error() { | ||
| builder = builder.winerror(winerror.to_pyobject(vm)); | ||
| } | ||
|
|
||
| builder | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
# Find ErrorExt::posix_errno implementation
rg -n "posix_errno" -C5 --type=rustRepository: RustPython/RustPython
Length of output: 4538
🏁 Script executed:
#!/bin/bash
# Find OSErrorBuilder implementation and its methods
rg -n "impl OSErrorBuilder" -A20 --type=rustRepository: RustPython/RustPython
Length of output: 1246
🏁 Script executed:
#!/bin/bash
# Find with_errno method in OSErrorBuilder
rg -n "with_errno" -A10 --type=rustRepository: RustPython/RustPython
Length of output: 1486
🏁 Script executed:
#!/bin/bash
# Find errno_to_exc_type to see how it handles errno values
rg -n "fn errno_to_exc_type" -A30 --type=rustRepository: RustPython/RustPython
Length of output: 4348
🏁 Script executed:
#!/bin/bash
# Check how errno=0 is handled elsewhere and if there are any tests
rg -n "errno.*0" -B2 -A2 --type=rust | head -50Repository: RustPython/RustPython
Length of output: 3710
🏁 Script executed:
#!/bin/bash
# Look for cases where std::io::Error might not have raw_os_error
rg -n "raw_os_error" -B3 -A3 --type=rust | head -80Repository: RustPython/RustPython
Length of output: 5378
🏁 Script executed:
#!/bin/bash
# Check if there are any non-OS errors in std::io::Error that would have raw_os_error() = None
# Look for test cases or examples
rg -n "std::io::Error" -B2 -A5 --type=rust | head -100Repository: RustPython/RustPython
Length of output: 6999
Guard errno > 0 before using with_errno; fall back to with_subtype(..., None, msg, ...) for non-OS errors.
The posix_errno() method returns 0 when std::io::Error has no OS error code (see crates/common/src/os.rs:32), resulting in OSError(errno=0) which is misleading since errno=0 means "Success". This is evident from line 1364 where the code already checks errno > 0 to identify valid POSIX errors. Apply the suggested guard to preserve the message while avoiding the invalid errno=0 state.
Suggested fix- let mut builder = OSErrorBuilder::with_errno(errno, msg, vm);
+ let mut builder = if errno > 0 {
+ OSErrorBuilder::with_errno(errno, msg, vm)
+ } else {
+ OSErrorBuilder::with_subtype(vm.ctx.exceptions.os_error.to_owned(), None, msg, vm)
+ };‼️ 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.
| impl ToOSErrorBuilder for std::io::Error { | |
| fn to_os_error_builder(&self, vm: &VirtualMachine) -> OSErrorBuilder { | |
| use crate::common::os::ErrorExt; | |
| let errno = self.posix_errno(); | |
| #[cfg(windows)] | |
| let msg = 'msg: { | |
| // Use C runtime's strerror for POSIX errno values. | |
| // For Windows-specific error codes, fall back to FormatMessage. | |
| const MAX_POSIX_ERRNO: i32 = 127; | |
| if errno > 0 && errno <= MAX_POSIX_ERRNO { | |
| let ptr = unsafe { libc::strerror(errno) }; | |
| if !ptr.is_null() { | |
| let s = unsafe { core::ffi::CStr::from_ptr(ptr) }.to_string_lossy(); | |
| if !s.starts_with("Unknown error") { | |
| break 'msg s.into_owned(); | |
| } | |
| } | |
| } | |
| self.to_string() | |
| }; | |
| #[cfg(unix)] | |
| let msg = { | |
| let ptr = unsafe { libc::strerror(errno) }; | |
| if !ptr.is_null() { | |
| unsafe { core::ffi::CStr::from_ptr(ptr) } | |
| .to_string_lossy() | |
| .into_owned() | |
| } else { | |
| vec![strerror.to_pyobject(vm)] | |
| }; | |
| self.to_string() | |
| } | |
| }; | |
| #[cfg(not(any(windows, unix)))] | |
| let msg = self.to_string(); | |
| #[allow(unused_mut)] | |
| let mut builder = OSErrorBuilder::with_errno(errno, msg, vm); | |
| let payload = PyOSError::py_new(&exc_type, args.clone().into(), vm) | |
| .expect("new_os_error usage error"); | |
| let os_error = payload | |
| .into_ref_with_type(vm, exc_type) | |
| .expect("new_os_error usage error"); | |
| PyOSError::slot_init(os_error.as_object().to_owned(), args.into(), vm) | |
| .expect("new_os_error usage error"); | |
| os_error | |
| #[cfg(windows)] | |
| if let Some(winerror) = self.raw_os_error() { | |
| builder = builder.winerror(winerror.to_pyobject(vm)); | |
| } | |
| builder | |
| } | |
| impl ToOSErrorBuilder for std::io::Error { | |
| fn to_os_error_builder(&self, vm: &VirtualMachine) -> OSErrorBuilder { | |
| use crate::common::os::ErrorExt; | |
| let errno = self.posix_errno(); | |
| #[cfg(windows)] | |
| let msg = 'msg: { | |
| // Use C runtime's strerror for POSIX errno values. | |
| // For Windows-specific error codes, fall back to FormatMessage. | |
| const MAX_POSIX_ERRNO: i32 = 127; | |
| if errno > 0 && errno <= MAX_POSIX_ERRNO { | |
| let ptr = unsafe { libc::strerror(errno) }; | |
| if !ptr.is_null() { | |
| let s = unsafe { core::ffi::CStr::from_ptr(ptr) }.to_string_lossy(); | |
| if !s.starts_with("Unknown error") { | |
| break 'msg s.into_owned(); | |
| } | |
| } | |
| } | |
| self.to_string() | |
| }; | |
| #[cfg(unix)] | |
| let msg = { | |
| let ptr = unsafe { libc::strerror(errno) }; | |
| if !ptr.is_null() { | |
| unsafe { core::ffi::CStr::from_ptr(ptr) } | |
| .to_string_lossy() | |
| .into_owned() | |
| } else { | |
| self.to_string() | |
| } | |
| }; | |
| #[cfg(not(any(windows, unix)))] | |
| let msg = self.to_string(); | |
| #[allow(unused_mut)] | |
| let mut builder = if errno > 0 { | |
| OSErrorBuilder::with_errno(errno, msg, vm) | |
| } else { | |
| OSErrorBuilder::with_subtype(vm.ctx.exceptions.os_error.to_owned(), None, msg, vm) | |
| }; | |
| #[cfg(windows)] | |
| if let Some(winerror) = self.raw_os_error() { | |
| builder = builder.winerror(winerror.to_pyobject(vm)); | |
| } | |
| builder | |
| } | |
| } |
In `@crates/vm/src/exceptions.rs` around lines 1354 - 1398, The impl of ToOSErrorBuilder for std::io::Error currently always calls OSErrorBuilder::with_errno(errno, msg, vm) even when posix_errno() returns 0; change to check the errno returned by posix_errno() and only call with_errno when errno > 0, otherwise call OSErrorBuilder::with_subtype(py_exception_type_for_os_error, None, msg, vm) (preserving the msg) so we don't emit OSError(errno=0); update the to_os_error_builder function around the creation of builder (the let mut builder = ... and subsequent #[cfg(windows)] winerror handling) to branch on errno accordingly while keeping the winerror attachment when available.
Sorry, something went wrong.
#4210
Summary by CodeRabbit
New Features
Tests / Chores