Actionable comments posted: 2

In `@crates/stdlib/src/socket.rs`:
- Around line 2204-2222: In settimeout (and the analogous setdefaulttimeout)
reject non-finite or excessively large floats before storing/using them: check
that the converted float is finite (f.is_finite()), non‑negative, and does not
exceed the maximum representable seconds for Duration::from_secs_f64 (e.g.
compare against u64::MAX as f64 or otherwise use a safe upper bound), and return
a ValueError via vm.new_value_error when the check fails; update the same
validations around timeout.store and any code paths that later call
Duration::from_secs_f64 so you never pass NaN/Inf or out‑of‑range values to
Duration::from_secs_f64.

In `@crates/vm/src/stdlib/os.rs`:
- Around line 1376-1385: The non-Unix branch handling of follow_symlinks (in the
match on follow_symlinks.into_option() that computes src_path) currently treats
None the same as Some(false); change it so the default None behaves like
Some(true) to match the Unix implementation: when follow_symlinks is Some(true)
or None, call fs::canonicalize(&src.path).unwrap_or_else(|_|
PathBuf::from(src.path.clone())), otherwise (Some(false)) use
PathBuf::from(src.path.clone()); update the match arms around
follow_symlinks.into_option(), src_path, and canonicalize accordingly.

crates/vm/src/stdlib/nt.rs (1)

186-195: Consider reusing environ() to keep _create_environ in sync.
This avoids drift if the filtering logic changes in one place.

crates/vm/src/stdlib/posix.rs (1)

456-467: Consider extracting shared logic to reduce duplication.

The _create_environ function duplicates the logic from the environ attribute (lines 443-454). While this may be intentional to match CPython's API, consider extracting the shared logic into a helper function.

♻️ Optional refactor to reduce duplication

+fn create_environ_dict(vm: &VirtualMachine) -> PyDictRef {
+    use rustpython_common::os::ffi::OsStringExt;
+
+    let environ = vm.ctx.new_dict();
+    for (key, value) in env::vars_os() {
+        let key: PyObjectRef = vm.ctx.new_bytes(key.into_vec()).into();
+        let value: PyObjectRef = vm.ctx.new_bytes(value.into_vec()).into();
+        environ.set_item(&*key, value, vm).unwrap();
+    }
+    environ
+}
+
 #[pyattr]
 fn environ(vm: &VirtualMachine) -> PyDictRef {
-    use rustpython_common::os::ffi::OsStringExt;
-
-    let environ = vm.ctx.new_dict();
-    for (key, value) in env::vars_os() {
-        let key: PyObjectRef = vm.ctx.new_bytes(key.into_vec()).into();
-        let value: PyObjectRef = vm.ctx.new_bytes(value.into_vec()).into();
-        environ.set_item(&*key, value, vm).unwrap();
-    }
-
-    environ
+    create_environ_dict(vm)
 }

 #[pyfunction]
 fn _create_environ(vm: &VirtualMachine) -> PyDictRef {
-    use rustpython_common::os::ffi::OsStringExt;
-
-    let environ = vm.ctx.new_dict();
-    for (key, value) in env::vars_os() {
-        let key: PyObjectRef = vm.ctx.new_bytes(key.into_vec()).into();
-        let value: PyObjectRef = vm.ctx.new_bytes(value.into_vec()).into();
-        environ.set_item(&*key, value, vm).unwrap();
-    }
-    environ
+    create_environ_dict(vm)
 }

Actionable comments posted: 0

Actionable comments posted: 2

In `@crates/vm/src/convert/try_from.rs`:
- Around line 140-145: The conversion can produce nanos == 1_000_000_000 due to
float precision and Duration::new (Self::new) will panic; after computing secs
and nanos from f (and frac), detect if nanos >= 1_000_000_000 and, if so,
increment secs by 1 and set nanos to 0 (or clamp nanos to 999_999_999 and adjust
secs accordingly) before calling Self::new(secs, nanos) to guarantee nanos <
1_000_000_000 and avoid panic.

In `@crates/vm/src/stdlib/nt.rs`:
- Around line 986-1001: The env handling in spawnve incorrectly accepts
path-like objects via FsPath::try_from_path_like for environment keys/values;
change the logic around FsPath::try_from_path_like(key, ...) and
FsPath::try_from_path_like(value, ...) so that spawnve only accepts native
strings/bytes: convert the incoming Py objects to Rust strings/bytes directly
(fail with a TypeError/ValueError if they are not str/bytes) and build env_str
from those string representations before converting to WideCString (preserve the
existing validation of '=' in key_str and the env_strings push). Apply the same
change to the second spawnve implementation later in the file to ensure
keys/values are not accepted via __fspath__.

crates/vm/src/convert/try_from.rs (2)

132-134: Inconsistent error message ownership between float and int paths.

Line 133 uses .to_owned() for the error message while line 150 uses a string literal directly. This inconsistency suggests either unnecessary allocation on line 133 or a potential compile error on line 150.

♻️ Suggested fix for consistency

Either remove .to_owned() from line 133 if new_value_error accepts &str:

 if f < 0.0 {
-    return Err(vm.new_value_error("negative duration".to_owned()));
+    return Err(vm.new_value_error("negative duration"));
 }

Or add .to_owned() to line 150 for consistency:

 if bigint.sign() == Sign::Minus {
-    return Err(vm.new_value_error("negative duration"));
+    return Err(vm.new_value_error("negative duration".to_owned()));
 }

Also applies to: 149-151

---

153-156: Consider aligning error message with float path for consistency.

The integer overflow error uses "value out of range" while the float path uses "timestamp too large to convert to C PyTime_t". Consider using consistent error messages for the same logical error (value exceeding valid range).

♻️ Suggested fix for message consistency

 let sec = bigint
     .to_u64()
-    .ok_or_else(|| vm.new_value_error("value out of range"))?;
+    .ok_or_else(|| vm.new_overflow_error("timestamp too large to convert to C PyTime_t".to_owned()))?;

crates/vm/src/stdlib/signal.rs (1)

208-222: Consider extracting VALID_SIGNALS to a module-level constant to avoid duplication.

This array is duplicated in raise_signal() at lines 505-513. Extracting it to a single #[cfg(windows)] constant would reduce maintenance burden and ensure consistency.

♻️ Proposed refactor

Add a module-level constant:

#[cfg(windows)]
const WINDOWS_VALID_SIGNALS: &[i32] = &[
    libc::SIGINT,
    libc::SIGILL,
    libc::SIGFPE,
    libc::SIGSEGV,
    libc::SIGTERM,
    SIGBREAK,
    libc::SIGABRT,
];

Then reference WINDOWS_VALID_SIGNALS in both signal() and raise_signal():

 #[cfg(windows)]
 {
-    const VALID_SIGNALS: &[i32] = &[
-        libc::SIGINT,
-        libc::SIGILL,
-        libc::SIGFPE,
-        libc::SIGSEGV,
-        libc::SIGTERM,
-        SIGBREAK,
-        libc::SIGABRT,
-    ];
-    if !VALID_SIGNALS.contains(&signalnum) {
+    if !WINDOWS_VALID_SIGNALS.contains(&signalnum) {
         return Err(vm.new_value_error(format!("signal number {} out of range", signalnum)));
     }
 }

crates/stdlib/src/socket.rs (1)

2204-2226: Validation logic correctly rejects invalid timeout values.

The implementation properly handles NaN, negative values, and infinity. The validation order is correct (NaN first with its specific message, then the combined negative/non-finite check).

Consider extracting the timeout validation into a helper to avoid duplication with setdefaulttimeout:

♻️ Optional: Extract shared validation logic

fn validate_timeout(f: f64, vm: &VirtualMachine) -> PyResult<f64> {
    if f.is_nan() {
        return Err(vm.new_value_error("Invalid value NaN (not a number)".to_owned()));
    }
    if f < 0.0 || !f.is_finite() {
        return Err(vm.new_value_error("Timeout value out of range".to_owned()));
    }
    Ok(f)
}

This could then be used by both settimeout and setdefaulttimeout.

crates/vm/src/ospath.rs (1)

84-95: Consider reusing warn_if_bool_fd from crate::stdlib::os.

This warning logic duplicates the warn_if_bool_fd function in crates/vm/src/stdlib/os.rs (lines 122-136). Both perform the same check and emit the same warning message. Consider calling the shared helper to reduce duplication and ensure consistency.

♻️ Proposed refactor to reuse the shared helper

         // Handle fd (before __fspath__ check, like CPython)
         if let Some(int) = obj.try_index_opt(vm) {
-            // Warn if bool is used as a file descriptor
-            if obj
-                .class()
-                .is(crate::builtins::bool_::PyBool::static_type())
-            {
-                crate::stdlib::warnings::warn(
-                    vm.ctx.exceptions.runtime_warning,
-                    "bool is used as a file descriptor".to_owned(),
-                    1,
-                    vm,
-                )?;
-            }
+            crate::stdlib::os::warn_if_bool_fd(&obj, vm)?;
             let fd = int?.try_to_primitive(vm)?;

Note: This requires warn_if_bool_fd to be made pub(crate) if it isn't already.

crates/vm/src/stdlib/os.rs (1)

1725-1728: Clarify the warning exception handling.

The condition e.fast_isinstance(vm.ctx.exceptions.warning) is checking if the exception is a Warning subclass. However, warn_if_bool_fd raises a RuntimeWarning via warnings.warn(), which typically doesn't raise an exception unless warnings are configured to raise.

If the intent is to propagate warnings-as-errors, this check is correct. Consider adding a brief comment to clarify the purpose.

📝 Proposed comment for clarity

         match path.clone().try_into_value::<crt_fd::Borrowed<'_>>(vm) {
             Ok(fd) => return ftruncate(fd, length).map_err(|e| e.into_pyexception(vm)),
+            // Propagate warnings-as-errors (e.g., RuntimeWarning from warn_if_bool_fd)
             Err(e) if e.fast_isinstance(vm.ctx.exceptions.warning) => return Err(e),
             Err(_) => {}
         }

crates/vm/src/stdlib/posix.rs (2)

296-307: Minor inconsistency: use imported warn_if_bool_fd for consistency.

Line 298 uses the full crate path crate::stdlib::os::warn_if_bool_fd, but the function is already imported at line 38. Line 518 correctly uses just warn_if_bool_fd. Consider using the imported name here for consistency.

Suggested change

     fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult<Self> {
-        crate::stdlib::os::warn_if_bool_fd(&obj, vm)?;
+        warn_if_bool_fd(&obj, vm)?;
         let fd = i32::try_from_object(vm, obj)?;

---

464-475: Consider eliminating code duplication with environ.

_create_environ (lines 464-475) has identical implementation to the environ attribute function (lines 450-462). Both iterate over env::vars_os(), convert keys and values to bytes, and insert them into a new dict.

Suggested refactor

 #[pyfunction]
 fn _create_environ(vm: &VirtualMachine) -> PyDictRef {
-    use rustpython_common::os::ffi::OsStringExt;
-
-    let environ = vm.ctx.new_dict();
-    for (key, value) in env::vars_os() {
-        let key: PyObjectRef = vm.ctx.new_bytes(key.into_vec()).into();
-        let value: PyObjectRef = vm.ctx.new_bytes(value.into_vec()).into();
-        environ.set_item(&*key, value, vm).unwrap();
-    }
-    environ
+    environ(vm)
 }