Skip to content

more hostenv isolation#7932

Open
youknowone wants to merge 12 commits into
RustPython:mainfrom
youknowone:hostenv
Open

more hostenv isolation#7932
youknowone wants to merge 12 commits into
RustPython:mainfrom
youknowone:hostenv

Conversation

@youknowone
Copy link
Copy Markdown
Member

@youknowone youknowone commented May 20, 2026

Summary by CodeRabbit

  • New Features

    • Added system hostname and MAC address helpers.
    • Added a cross-platform "which" helper to resolve executables.
  • Bug Fixes

    • Readline history load/save improved: saving will create parent dirs and load behavior simplified on non‑WASM targets.
  • Refactor

    • Expanded and unified platform-specific constant/type exports for broader OS coverage.
    • Centralized readline exposure to the host-provided module.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 20, 2026

📝 Walkthrough

Walkthrough

Updates 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.

Changes

Libc abstraction and modernization

Layer / File(s) Summary
Host surface and helpers
crates/host_env/Cargo.toml, crates/host_env/src/*
host_env adds target-specific dependencies, re-exports many platform libc constants (fcntl, locale, mmap, select, syslog, termios, resource, shm, posix, faulthandler, posix/multiprocessing types), and adds helpers: fs::which(), socket::hostname(), socket::mac_address(), fileutils::CFile/fclose(), plus exposes readline and macOS system_configuration.
Stdlib imports switched to host_env
crates/stdlib/src/{fcntl,locale,mmap,select,syslog,termios,socket,resource,posixsubprocess,grp,uuid,posixshmem}.rs, crates/stdlib/Cargo.toml
Stdlib modules now source platform constants/types and helpers from rustpython_host_env (host_* modules) instead of libc or direct external crates; Cargo target dependencies were adjusted accordingly.
FFI type modernization & API adjustments
crates/stdlib/src/{faulthandler,openssl,mmap,multiprocessing,posixshmem,openssl/cert}.rs
Many FFI signatures, callback types, struct fields, and constants were migrated from libc::c_* to core::ffi::c_*; error/errno handling and file/socket/DNS helpers updated to use host_env equivalents; OpenSSL bindings and BIO/file helpers updated accordingly.
VM/readline/export changes
crates/vm/Cargo.toml, crates/vm/src/{lib,getpath}.rs, crates/host_env/src/readline.rs
Executable-path resolution and readline exposure now use host_env fs::which and readline re-export; readline history save/load logic simplified and parent-dir creation centralized.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

  • RustPython/RustPython#7808 — touches resource/rlimit logic and conversion validation that closely relates to resource.rs changes.

Suggested reviewers

  • ShaharNaveh
  • coolreader18

Poem

🐰 I hopped through crates with tiny paws,

I gathered constants, fixed their laws.
Host_env now keeps the bindings tight,
core::ffi makes FFI right.
A little hop — the build takes flight!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 77.39% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'more hostenv isolation' is vague and generic, using non-descriptive terms that don't clearly convey the actual scope or primary intent of this extensive refactoring changeset. Consider a more specific title that reflects the main objective, such as 'Migrate C type dependencies from libc to rustpython_host_env' or 'Consolidate C FFI types through host_env abstraction layer'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@youknowone youknowone marked this pull request as ready for review May 20, 2026 11:05
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between ae3804f and 1c27152.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (32)
  • crates/host_env/Cargo.toml
  • crates/host_env/src/faulthandler.rs
  • crates/host_env/src/fcntl.rs
  • crates/host_env/src/fileutils.rs
  • crates/host_env/src/fs.rs
  • crates/host_env/src/lib.rs
  • crates/host_env/src/locale.rs
  • crates/host_env/src/mmap.rs
  • crates/host_env/src/multiprocessing.rs
  • crates/host_env/src/readline.rs
  • crates/host_env/src/select.rs
  • crates/host_env/src/shm.rs
  • crates/host_env/src/socket.rs
  • crates/host_env/src/syslog.rs
  • crates/host_env/src/termios.rs
  • crates/stdlib/Cargo.toml
  • crates/stdlib/src/faulthandler.rs
  • crates/stdlib/src/fcntl.rs
  • crates/stdlib/src/locale.rs
  • crates/stdlib/src/mmap.rs
  • crates/stdlib/src/multiprocessing.rs
  • crates/stdlib/src/openssl.rs
  • crates/stdlib/src/openssl/cert.rs
  • crates/stdlib/src/posixshmem.rs
  • crates/stdlib/src/select.rs
  • crates/stdlib/src/socket.rs
  • crates/stdlib/src/syslog.rs
  • crates/stdlib/src/termios.rs
  • crates/stdlib/src/uuid.rs
  • crates/vm/Cargo.toml
  • crates/vm/src/getpath.rs
  • crates/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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Do not accept an AF_CAN payload you never serialize.

This branch allows (interface, addr) but only writes can_family and can_ifindex; tuple[1] is ignored. That means bind(), connect(), and sendto() 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_CAN addresses are truncated on the read path.

get_addr_tuple() always returns (ifname,) for AF_CAN, so recvfrom(), getsockname(), and getpeername() 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 win

Add missing EISCONN constant to the platform abstraction layer.

Line 1347 references c::EISCONN, but this constant is not exported from rustpython_host_env::socket. The import list at lines 60-85 in crates/stdlib/src/socket.rs does not include it, and it's missing from both:

  • The Windows WinSock imports (where WSAEISCONN must be added and mapped)
  • The Unix libc exports (where EISCONN should be re-exported)

This is a compile blocker on all platforms. Add EISCONN to crates/host_env/src/socket.rs by either directly importing it from libc (Unix) or mapping WSAEISCONN as a platform-specific constant (Windows), then update the mod c re-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

📥 Commits

Reviewing files that changed from the base of the PR and between 1c27152 and c2e08c9.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (12)
  • crates/host_env/Cargo.toml
  • crates/host_env/src/grp.rs
  • crates/host_env/src/lib.rs
  • crates/host_env/src/posix.rs
  • crates/host_env/src/resource.rs
  • crates/host_env/src/socket.rs
  • crates/stdlib/Cargo.toml
  • crates/stdlib/src/grp.rs
  • crates/stdlib/src/posixsubprocess.rs
  • crates/stdlib/src/resource.rs
  • crates/stdlib/src/scproxy.rs
  • crates/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

youknowone added 12 commits May 20, 2026 22:40
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.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Avoid panic on non-UTF8 hostnames in gethostname.

Line 2322 calls err.into_string().unwrap() on an OsString error, which panics for non-UTF8 hostnames. Use vm.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

📥 Commits

Reviewing files that changed from the base of the PR and between c2e08c9 and 8d89a7d.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (39)
  • crates/host_env/Cargo.toml
  • crates/host_env/src/faulthandler.rs
  • crates/host_env/src/fcntl.rs
  • crates/host_env/src/fileutils.rs
  • crates/host_env/src/fs.rs
  • crates/host_env/src/grp.rs
  • crates/host_env/src/lib.rs
  • crates/host_env/src/locale.rs
  • crates/host_env/src/mmap.rs
  • crates/host_env/src/multiprocessing.rs
  • crates/host_env/src/posix.rs
  • crates/host_env/src/readline.rs
  • crates/host_env/src/resource.rs
  • crates/host_env/src/select.rs
  • crates/host_env/src/shm.rs
  • crates/host_env/src/socket.rs
  • crates/host_env/src/syslog.rs
  • crates/host_env/src/termios.rs
  • crates/stdlib/Cargo.toml
  • crates/stdlib/src/faulthandler.rs
  • crates/stdlib/src/fcntl.rs
  • crates/stdlib/src/grp.rs
  • crates/stdlib/src/locale.rs
  • crates/stdlib/src/mmap.rs
  • crates/stdlib/src/multiprocessing.rs
  • crates/stdlib/src/openssl.rs
  • crates/stdlib/src/openssl/cert.rs
  • crates/stdlib/src/posixshmem.rs
  • crates/stdlib/src/posixsubprocess.rs
  • crates/stdlib/src/resource.rs
  • crates/stdlib/src/scproxy.rs
  • crates/stdlib/src/select.rs
  • crates/stdlib/src/socket.rs
  • crates/stdlib/src/syslog.rs
  • crates/stdlib/src/termios.rs
  • crates/stdlib/src/uuid.rs
  • crates/vm/Cargo.toml
  • crates/vm/src/getpath.rs
  • crates/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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant