more hostenv isolation#7932
Conversation
📝 WalkthroughWalkthroughUpdates host_env with many platform-conditional libc re-exports and helpers (fs::which, socket::hostname/mac_address, fileutils::CFile/fclose), adjusts Cargo target deps, then migrates stdlib/vm to import those host_env symbols and modernizes FFI types from libc::c_* to core::ffi::c_* across multiple modules. ChangesLibc abstraction and modernization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/host_env/src/lib.rs`:
- Line 33: The module export `pub mod readline;` is currently unconditional but
its crate dependency is only available for non-wasm32 targets; wrap the export
with a target cfg to match Cargo.toml (e.g. change to `#[cfg(not(target_arch =
"wasm32"))] pub mod readline;`) so the module is only compiled on supported
targets; locate the `pub mod readline` line in lib.rs and apply the cfg
attribute (or an equivalent cfg_if) to avoid the cfg/dependency mismatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 17e7dc80-94cd-446a-8624-c6b9bff09c11
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
crates/host_env/Cargo.tomlcrates/host_env/src/faulthandler.rscrates/host_env/src/fcntl.rscrates/host_env/src/fileutils.rscrates/host_env/src/fs.rscrates/host_env/src/lib.rscrates/host_env/src/locale.rscrates/host_env/src/mmap.rscrates/host_env/src/multiprocessing.rscrates/host_env/src/readline.rscrates/host_env/src/select.rscrates/host_env/src/shm.rscrates/host_env/src/socket.rscrates/host_env/src/syslog.rscrates/host_env/src/termios.rscrates/stdlib/Cargo.tomlcrates/stdlib/src/faulthandler.rscrates/stdlib/src/fcntl.rscrates/stdlib/src/locale.rscrates/stdlib/src/mmap.rscrates/stdlib/src/multiprocessing.rscrates/stdlib/src/openssl.rscrates/stdlib/src/openssl/cert.rscrates/stdlib/src/posixshmem.rscrates/stdlib/src/select.rscrates/stdlib/src/socket.rscrates/stdlib/src/syslog.rscrates/stdlib/src/termios.rscrates/stdlib/src/uuid.rscrates/vm/Cargo.tomlcrates/vm/src/getpath.rscrates/vm/src/lib.rs
💤 Files with no reviewable changes (2)
- crates/vm/Cargo.toml
- crates/stdlib/Cargo.toml
| pub mod fs; | ||
| #[cfg(any(unix, windows))] | ||
| pub mod locale; | ||
| pub mod readline; |
There was a problem hiding this comment.
Gate readline export by target to avoid cfg/dependency mismatch.
Line 33 exports readline on all targets, but its dependency surface is target-gated in Cargo.toml (non-wasm32). This can break unsupported targets at compile time.
🔧 Proposed fix
- pub mod readline;
+ #[cfg(not(target_arch = "wasm32"))]
+ pub mod readline;📝 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.
| pub mod readline; | |
| #[cfg(not(target_arch = "wasm32"))] | |
| pub mod readline; |
🤖 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 `@crates/host_env/src/lib.rs` at line 33, The module export `pub mod readline;`
is currently unconditional but its crate dependency is only available for
non-wasm32 targets; wrap the export with a target cfg to match Cargo.toml (e.g.
change to `#[cfg(not(target_arch = "wasm32"))] pub mod readline;`) so the module
is only compiled on supported targets; locate the `pub mod readline` line in
lib.rs and apply the cfg attribute (or an equivalent cfg_if) to avoid the
cfg/dependency mismatch.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/stdlib/src/socket.rs (3)
1187-1242:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not accept an
AF_CANpayload you never serialize.This branch allows
(interface, addr)but only writescan_familyandcan_ifindex;tuple[1]is ignored. That meansbind(),connect(), andsendto()can silently target the wrong CAN endpoint instead of rejecting unsupported tuple shapes.🤖 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 `@crates/stdlib/src/socket.rs` around lines 1187 - 1242, The AF_CAN branch currently accepts a two-element tuple (interface, addr) but ignores tuple[1]; update this logic to either fully serialize the second element into sockaddr_can or reject two-element tuples: simplest fix is to disallow tuple.len() == 2 — change the tuple length validation in the AF_CAN branch (the code that builds `tuple: PyTupleRef`) to only accept a single-element tuple `(interface,)` and return a TypeError if a second element is provided; reference the AF_CAN branch, `tuple`, `can_addr`/`sockaddr_can`, and `caller` when making the change so callers get a clear error instead of silently ignoring the second element.
2279-2289:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
AF_CANaddresses are truncated on the read path.
get_addr_tuple()always returns(ifname,)forAF_CAN, sorecvfrom(),getsockname(), andgetpeername()lose the CAN-specific address payload. That breaks address round-tripping for protocol-specific CAN sockets.🤖 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 `@crates/stdlib/src/socket.rs` around lines 2279 - 2289, get_addr_tuple() currently collapses AF_CAN sockaddr_can into only (ifname,) losing the can_id and breaking round-trips for recvfrom()/getsockname()/getpeername(); fix by reading both can_ifindex and can_id from the sockaddr_can (use unsafe { &*(addr.as_ptr() as *const c::sockaddr_can) }), convert ifindex to ifname as before, and return a tuple containing both fields when can_id != 0 (e.g. (ifname, can_id)) or include a zero/null can_id explicitly so vm.ctx.new_tuple(...) produces the full CAN address instead of only vm.ctx.new_str(ifname).into().
1343-1348:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd missing
EISCONNconstant to the platform abstraction layer.Line 1347 references
c::EISCONN, but this constant is not exported fromrustpython_host_env::socket. The import list at lines 60-85 incrates/stdlib/src/socket.rsdoes not include it, and it's missing from both:
- The Windows WinSock imports (where
WSAEISCONNmust be added and mapped)- The Unix libc exports (where
EISCONNshould be re-exported)This is a compile blocker on all platforms. Add
EISCONNtocrates/host_env/src/socket.rsby either directly importing it from libc (Unix) or mappingWSAEISCONNas a platform-specific constant (Windows), then update themod cre-export list to include it.🤖 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 `@crates/stdlib/src/socket.rs` around lines 1343 - 1348, Add the missing EISCONN constant to the platform abstraction so c::EISCONN used in socket.rs compiles: in crates/host_env/src/socket.rs import/re-export EISCONN for Unix (pull from libc::EISCONN) and on Windows define/map a constant from winsock (WSAEISCONN) to the same name, then add EISCONN to the mod c re-export list; this will satisfy the use of c::EISCONN in methods like sock_op / SelectKind::Connect and ensure platform-specific mapping for WSAEISCONN on Windows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/stdlib/src/socket.rs`:
- Around line 1187-1242: The AF_CAN branch currently accepts a two-element tuple
(interface, addr) but ignores tuple[1]; update this logic to either fully
serialize the second element into sockaddr_can or reject two-element tuples:
simplest fix is to disallow tuple.len() == 2 — change the tuple length
validation in the AF_CAN branch (the code that builds `tuple: PyTupleRef`) to
only accept a single-element tuple `(interface,)` and return a TypeError if a
second element is provided; reference the AF_CAN branch, `tuple`,
`can_addr`/`sockaddr_can`, and `caller` when making the change so callers get a
clear error instead of silently ignoring the second element.
- Around line 2279-2289: get_addr_tuple() currently collapses AF_CAN
sockaddr_can into only (ifname,) losing the can_id and breaking round-trips for
recvfrom()/getsockname()/getpeername(); fix by reading both can_ifindex and
can_id from the sockaddr_can (use unsafe { &*(addr.as_ptr() as *const
c::sockaddr_can) }), convert ifindex to ifname as before, and return a tuple
containing both fields when can_id != 0 (e.g. (ifname, can_id)) or include a
zero/null can_id explicitly so vm.ctx.new_tuple(...) produces the full CAN
address instead of only vm.ctx.new_str(ifname).into().
- Around line 1343-1348: Add the missing EISCONN constant to the platform
abstraction so c::EISCONN used in socket.rs compiles: in
crates/host_env/src/socket.rs import/re-export EISCONN for Unix (pull from
libc::EISCONN) and on Windows define/map a constant from winsock (WSAEISCONN) to
the same name, then add EISCONN to the mod c re-export list; this will satisfy
the use of c::EISCONN in methods like sock_op / SelectKind::Connect and ensure
platform-specific mapping for WSAEISCONN on Windows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2130668c-941c-4855-a192-2b4380901f71
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/host_env/Cargo.tomlcrates/host_env/src/grp.rscrates/host_env/src/lib.rscrates/host_env/src/posix.rscrates/host_env/src/resource.rscrates/host_env/src/socket.rscrates/stdlib/Cargo.tomlcrates/stdlib/src/grp.rscrates/stdlib/src/posixsubprocess.rscrates/stdlib/src/resource.rscrates/stdlib/src/scproxy.rscrates/stdlib/src/socket.rs
💤 Files with no reviewable changes (1)
- crates/stdlib/Cargo.toml
✅ Files skipped from review due to trivial changes (2)
- crates/host_env/src/grp.rs
- crates/stdlib/src/scproxy.rs
Add host_env::fs::which wrapper and update getpath.rs call site.
Move crates/vm/src/readline.rs to crates/host_env/src/readline.rs and re-export it from rustpython_vm::readline. Move rustyline dependency from vm to host_env. Drop the host_env feature gates on history I/O inside the moved module.
Add host_env::socket::hostname and host_env::socket::mac_address wrappers. Update _socket.gethostname and _uuid.get_node_id call sites.
Move direct libc constant and type references in stdlib (faulthandler, fcntl, multiprocessing, posixshmem, select, syslog) to use host_env re-exports. Replace libc::c_int and libc::c_uint in stdlib with core::ffi equivalents.
Move direct libc MADV_*, MAP_*, PROT_*, EOVERFLOW references in stdlib::mmap to host_env::mmap re-exports. Replace libc::c_int with core::ffi::c_int.
Move direct libc LC_*, ABDAY_*, ABMON_*, TIOC*, FIO* and related constants in stdlib::locale and stdlib::termios to host_env re-exports. Replace libc::c_char in stdlib::locale and stdlib::openssl::cert with core::ffi::c_char.
Replace libc::c_int, c_long, c_uchar, c_uint, c_ulong, c_void,
c_char in stdlib::openssl with core::ffi equivalents. Replace
libc::ENOENT with host_env::errno::errors::ENOENT. Add
host_env::fileutils::{CFile, fclose} and route the load_dh_params
fclose call and PEM_read_DHparams FILE pointer type through them.
host_mmap::EOVERFLOW and host_faulthandler::{SIGSEGV, SIGFPE}
re-exports were gated to cfg(unix) but stdlib uses them from Windows
and platform-agnostic code paths.
Move direct libc references in stdlib (resource, posixsubprocess,
grp, socket) to host_env re-exports. Replace libc::c_long,
libc::c_longlong, libc::c_char in stdlib::socket with core::ffi
equivalents. Add resource::{RLIMIT_*, RUSAGE_*, c_long, rlim_t,
rlimit, timeval}, posix::{c_char, pid_t}, grp::gid_t, and
socket::{AF_*, SOCK_STREAM, sa_family_t, socklen_t, sockaddr_*}
re-exports.
Re-export socket2 as host_env::socket::raw and dns-lookup as host_env::socket::dns. Update _socket call sites to route through host_env aliases.
Re-export the macOS system-configuration crate as host_env::system_configuration and update _scproxy call sites.
c::EISCONN works on unix via libc but the Windows c:: re-export list does not include it. Route through rustpython_host_env::errno::errors where EISCONN is mapped to WSAEISCONN on Windows.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/stdlib/src/socket.rs (1)
2319-2323:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid panic on non-UTF8 hostnames in
gethostname.Line 2322 calls
err.into_string().unwrap()on anOsStringerror, which panics for non-UTF8 hostnames. Usevm.fsdecode()instead, which is the idiomatic RustPython pattern for handling OS strings with potential non-UTF8 content and is already used elsewhere in this file (line 2279).Proposed fix
#[pyfunction] fn gethostname(vm: &VirtualMachine) -> PyResult<PyStrRef> { - rustpython_host_env::socket::hostname() - .into_string() - .map(|hostname| vm.ctx.new_str(hostname)) - .map_err(|err| vm.new_os_error(err.into_string().unwrap())) + let hostname = rustpython_host_env::socket::hostname(); + Ok(vm.fsdecode(hostname.as_os_str())) }🤖 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 `@crates/stdlib/src/socket.rs` around lines 2319 - 2323, The map_err closure in the gethostname path currently calls err.into_string().unwrap(), which can panic on non-UTF8 hostnames; replace that unwrap with the RustPython fsdecode helper so the OS string is safely converted into a Python string for the error. Locate the closure on the result of rustpython_host_env::socket::hostname() (the map_err that constructs vm.new_os_error) and change it to call vm.fsdecode on the OsString/OsStr error and pass that decoded string into vm.new_os_error (use vm.fsdecode instead of into_string().unwrap()).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/stdlib/src/socket.rs`:
- Around line 2319-2323: The map_err closure in the gethostname path currently
calls err.into_string().unwrap(), which can panic on non-UTF8 hostnames; replace
that unwrap with the RustPython fsdecode helper so the OS string is safely
converted into a Python string for the error. Locate the closure on the result
of rustpython_host_env::socket::hostname() (the map_err that constructs
vm.new_os_error) and change it to call vm.fsdecode on the OsString/OsStr error
and pass that decoded string into vm.new_os_error (use vm.fsdecode instead of
into_string().unwrap()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 707278ab-88aa-4c7b-ab07-01dde84f5278
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
crates/host_env/Cargo.tomlcrates/host_env/src/faulthandler.rscrates/host_env/src/fcntl.rscrates/host_env/src/fileutils.rscrates/host_env/src/fs.rscrates/host_env/src/grp.rscrates/host_env/src/lib.rscrates/host_env/src/locale.rscrates/host_env/src/mmap.rscrates/host_env/src/multiprocessing.rscrates/host_env/src/posix.rscrates/host_env/src/readline.rscrates/host_env/src/resource.rscrates/host_env/src/select.rscrates/host_env/src/shm.rscrates/host_env/src/socket.rscrates/host_env/src/syslog.rscrates/host_env/src/termios.rscrates/stdlib/Cargo.tomlcrates/stdlib/src/faulthandler.rscrates/stdlib/src/fcntl.rscrates/stdlib/src/grp.rscrates/stdlib/src/locale.rscrates/stdlib/src/mmap.rscrates/stdlib/src/multiprocessing.rscrates/stdlib/src/openssl.rscrates/stdlib/src/openssl/cert.rscrates/stdlib/src/posixshmem.rscrates/stdlib/src/posixsubprocess.rscrates/stdlib/src/resource.rscrates/stdlib/src/scproxy.rscrates/stdlib/src/select.rscrates/stdlib/src/socket.rscrates/stdlib/src/syslog.rscrates/stdlib/src/termios.rscrates/stdlib/src/uuid.rscrates/vm/Cargo.tomlcrates/vm/src/getpath.rscrates/vm/src/lib.rs
💤 Files with no reviewable changes (2)
- crates/vm/Cargo.toml
- crates/stdlib/Cargo.toml
✅ Files skipped from review due to trivial changes (2)
- crates/host_env/src/shm.rs
- crates/host_env/src/grp.rs
Summary by CodeRabbit
New Features
Bug Fixes
Refactor