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

    • System hostname and MAC address helpers.
    • Cross-platform which helper to resolve executables.
    • Readline now exposed from the host-provided implementation.
  • Bug Fixes

    • Readline history save now creates parent directories; load/save behavior simplified on non‑WASM targets.
  • Refactor

    • Broader, unified platform constant/type sourcing across stdlib and host integration.
    • UUID node-id generation now prefers host MAC when available.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 20, 2026

📝 Walkthrough

Walkthrough

Adds target-specific host_env dependencies, many platform-gated libc re-exports and helpers (fs::which, socket::hostname/mac_address, fileutils::CFile/fclose, readline/system_configuration), and rewires stdlib/vm to consume rustpython_host_env symbols while migrating FFI integer types to core::ffi equivalents.

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, multiprocessing), 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 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

Suggested reviewers

  • ShaharNaveh

Poem

🐰 I hopped through crates with tiny paws,
I gathered constants from system laws.
Host_env now holds the platform keys,
core::ffi whispers through FFI trees.
Cheers — a carrot-shaped CI green light!

🚥 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 language that doesn't convey the specific changes in the changeset. Consider using a more descriptive title that highlights the main refactoring objective, such as 'Refactor: migrate stdlib from libc to rustpython_host_env' or 'Extract platform constants to host_env module for better isolation'.
✅ 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

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

youknowone added 12 commits May 22, 2026 01:44
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.

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/stdlib/src/socket.rs`:
- Around line 2319-2322: The current error branch calls
err.into_string().unwrap(), which can panic on non‑UTF8 hostnames; update the
conversion in the chain starting at rustpython_host_env::socket::hostname() so
it never unwraps: when mapping the Ok case produce vm.ctx.new_str(hostname) as
before, and for the Err path pass a safe String into vm.new_os_error by using
to_string_lossy()/to_owned() (or otherwise fall back to lossy conversion)
instead of unwrap; ensure the map_err closure returns vm.new_os_error(err_lossy)
without panicking.
🪄 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: aee56f42-d640-42e6-b230-76949aee47c0

📥 Commits

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

⛔ 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 (4)
  • crates/host_env/src/grp.rs
  • crates/stdlib/src/scproxy.rs
  • crates/host_env/src/fs.rs
  • crates/vm/src/lib.rs

Comment on lines +2319 to 2322
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()))
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 | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
rg -n --type rust -C3 '\bfn\s+hostname\b' crates/host_env/src/socket.rs
rg -n --type rust -C2 'into_string\(\)|to_string_lossy\(' crates/stdlib/src/socket.rs

Repository: RustPython/RustPython

Length of output: 905


Avoid panic when converting hostname to UTF-8

rustpython_host_env::socket::hostname() returns OsString; the current error path uses err.into_string().unwrap() at crates/stdlib/src/socket.rs:2322, which will panic on non‑UTF8 hostnames.

💡 Suggested fix
     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()))
+            .map_err(|err| vm.new_os_error(err.to_string_lossy().into_owned()))
     }
📝 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
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()))
rustpython_host_env::socket::hostname()
.into_string()
.map(|hostname| vm.ctx.new_str(hostname))
.map_err(|err| vm.new_os_error(err.to_string_lossy().into_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 `@crates/stdlib/src/socket.rs` around lines 2319 - 2322, The current error
branch calls err.into_string().unwrap(), which can panic on non‑UTF8 hostnames;
update the conversion in the chain starting at
rustpython_host_env::socket::hostname() so it never unwraps: when mapping the Ok
case produce vm.ctx.new_str(hostname) as before, and for the Err path pass a
safe String into vm.new_os_error by using to_string_lossy()/to_owned() (or
otherwise fall back to lossy conversion) instead of unwrap; ensure the map_err
closure returns vm.new_os_error(err_lossy) without panicking.

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