ffi: No interior NULs (part 1) - #8245

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/host_env/src/ctypes.rs (1)

1095-1097: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add tests for the new fallible null_terminated_bytes.

This is a security-critical function (interior NUL detection for FFI), but the test module has no coverage for it. Consider adding tests for: valid input without NULs, input containing an interior NUL (should return Err(NulError)), and empty input.

🧪 Suggested tests

#[test]
fn null_terminated_bytes_valid() {
    assert_eq!(
        null_terminated_bytes(b"hello").unwrap(),
        b"hello\0"
    );
}

#[test]
fn null_terminated_bytes_interior_nul_rejected() {
    assert!(null_terminated_bytes(b"hel\0lo").is_err());
}

#[test]
fn null_terminated_bytes_empty() {
    assert_eq!(null_terminated_bytes(b"").unwrap(), b"\0");
}

🤖 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/ctypes.rs` around lines 1095 - 1097, Add test coverage
for the fallible null_terminated_bytes helper in ctypes.rs, since it now
performs FFI-safe interior NUL validation via CString::new. Extend the existing
test module with cases for valid non-NUL input, input containing an interior NUL
that must return Err(NulError), and empty input; use the null_terminated_bytes
function name directly so the tests clearly target the new behavior.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/host_env/src/windows.rs (1)

426-427: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Complete to_wide_cstring; WideCString::from_ cannot compile.

Collect the checked UTF-16 units and construct a WideCString with the constructor supported by the repository’s pinned widestring version.

As per coding guidelines, follow default rustfmt style and run cargo clippy, fixing introduced warnings before completion.

#!/bin/bash
set -euo pipefail

rg -n -C3 'name = "widestring"|widestring\s*=' Cargo.lock Cargo.toml
rg -n -C3 'WideCString::from_(vec|vec_with_nul|os_str|str)' --type rust .

🤖 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/windows.rs` around lines 426 - 427, Complete the
to_wide_cstring method by collecting the validated UTF-16 units and constructing
WideCString with a constructor available in the repository’s pinned widestring
version, replacing the incomplete WideCString::from_ call. Apply default rustfmt
formatting and run cargo clippy, resolving any warnings introduced by this
change.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/host_env/src/windows.rs (1)

426-427: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Complete to_wide_cstring; WideCString::from_ cannot compile.

Collect the checked UTF-16 units and construct a WideCString with the constructor supported by the repository’s pinned widestring version.

As per coding guidelines, follow default rustfmt style and run cargo clippy, fixing introduced warnings before completion.

#!/bin/bash
set -euo pipefail

rg -n -C3 'name = "widestring"|widestring\s*=' Cargo.lock Cargo.toml
rg -n -C3 'WideCString::from_(vec|vec_with_nul|os_str|str)' --type rust .

🤖 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/windows.rs` around lines 426 - 427, Complete the
to_wide_cstring method by collecting the validated UTF-16 units and constructing
WideCString with a constructor available in the repository’s pinned widestring
version, replacing the incomplete WideCString::from_ call. Apply default rustfmt
formatting and run cargo clippy, resolving any warnings introduced by this
change.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/host_env/src/ctypes.rs (1)

533-535: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C3 '\b(type WChar|WCHAR_SIZE|wchar_to_bytes|wchar_null_terminated_bytes)\b' \
  crates/host_env/src/ctypes.rs
rg -n -C3 '\bencode_wide(_ffi)?\s*\(' crates/host_env/src/ctypes.rs crates/wtf8/src/lib.rs

Repository: RustPython/RustPython

Length of output: 5652

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '520,590p' crates/host_env/src/ctypes.rs
printf '\n---\n'
sed -n '900,940p' crates/wtf8/src/lib.rs
printf '\n---\n'
sed -n '1,220p' crates/wtf8/src/lib.rs | sed -n '1,40p'

Repository: RustPython/RustPython

Length of output: 5517

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path
text = Path('crates/wtf8/src/lib.rs').read_text()
for needle in ['struct EncodeWide', 'impl Iterator for EncodeWide', 'struct EncodeWideForFfi', 'impl Iterator for EncodeWideForFfi']:
    idx = text.find(needle)
    print('\n###', needle, idx)
    if idx != -1:
        start = text.rfind('\n', 0, idx-200)
        end = text.find('\n\n', idx)
        print(text[idx:idx+1400])
PY

Repository: RustPython/RustPython

Length of output: 5913

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C4 'EncodeWide|EncodeWideForFfi|encode_wide\(' crates/wtf8/src/lib.rs

Repository: RustPython/RustPython

Length of output: 2350

---

Use encode_wide_ffi() here. Casting code points directly to WChar drops non-BMP characters on 16-bit targets instead of emitting surrogate pairs.

🤖 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/ctypes.rs` around lines 533 - 535, Update
wchar_null_terminated_bytes to use encode_wide_ffi() instead of casting code
points directly to WChar, preserving non-BMP characters as surrogate pairs on
16-bit targets while retaining the existing null-terminated byte iteration
behavior.

crates/host_env/src/nt.rs (2)

220-220: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift

Update downstream callers to handle the new Result return type.

Changing the return type of access (as well as test_file_type_by_name and test_file_exists_by_name) to Result<bool, io::Error> causes compilation errors in downstream consumers like crates/vm/src/stdlib/nt.rs, which still expect a bool.

You need to update the callers to handle the Result. For instance, in crates/vm/src/stdlib/nt.rs, map the error to a Python exception for access, and use .unwrap_or(false) for the internal file testing helpers.

🤖 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/nt.rs` at line 220, Update downstream callers of
`access`, `test_file_type_by_name`, and `test_file_exists_by_name` to handle
their `Result<bool, io::Error>` returns. In the VM stdlib `access` binding, map
I/O errors to the appropriate Python exception; for the internal file-testing
helpers, convert errors to `false` with `.unwrap_or(false)` while preserving
existing boolean behavior.

---

986-986: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Wrap the return value in Ok.

Type mismatch: the function returns Result<bool, io::Error>, so returning (false) will cause a compilation error.

🐛 Proposed fix

-                return (false);
+                return Ok(false);

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

                return Ok(false);

🤖 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/nt.rs` at line 986, Update the return statement in the
surrounding function to return the boolean value as a successful Result,
matching its Result<bool, io::Error> return type; preserve the existing false
outcome.

crates/host_env/src/windows.rs (1)

403-405: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C4 --type rust '\.to_wide_(with_nul|cstring)\s*\(' .

Repository: RustPython/RustPython

Length of output: 47727

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

wc -l crates/host_env/src/windows.rs
sed -n '1,260p' crates/host_env/src/windows.rs
printf '\n---SNIP---\n'
sed -n '260,520p' crates/host_env/src/windows.rs

Repository: RustPython/RustPython

Length of output: 13806

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show all call sites that may still rely on the old infallible behavior.
rg -n -C2 --type rust 'to_wide_with_nul\(\)|to_wide_cstring\(' crates/host_env crates/vm

Repository: RustPython/RustPython

Length of output: 30715

---

Propagate the fallible wide-string conversions to the remaining Windows callers. Several sites still treat to_wide_with_nul() / to_wide_cstring() as infallible (crates/host_env/src/windows.rs:157,184, crates/vm/src/stdlib/winsound.rs:148, crates/vm/src/stdlib/winreg.rs:744,763), so the Windows build still breaks until each one handles the Result.

🤖 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/windows.rs` around lines 403 - 405, Update the remaining
Windows callers of ToWideString::to_wide_with_nul and
ToWideString::to_wide_cstring to handle their Result values explicitly.
Propagate or otherwise handle conversion errors in the callers in windows.rs,
winsound, and winreg, preserving each call site’s existing success behavior and
avoiding infallible assumptions.

crates/host_env/src/winreg.rs (3)

353-355: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Pass an error instance rather than a variant constructor.

Type error: QueryStringError::Utf16 is a tuple variant that expects a FromUtf16Error argument. Passing the variant name without an argument to map_err returns a function pointer rather than an error instance, causing a compilation failure. Consider mapping to an appropriate Windows error code instead.

🐛 Proposed fix

         let wide_sub_key = sub_key
             .to_wide_cstring()
-            .map_err(|_| QueryStringError::Utf16)?;
+            .map_err(|_| QueryStringError::Code(windows_sys::Win32::Foundation::ERROR_INVALID_DATA))?;

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

        let wide_sub_key = sub_key
            .to_wide_cstring()
            .map_err(|_| QueryStringError::Code(windows_sys::Win32::Foundation::ERROR_INVALID_DATA))?;

🤖 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/winreg.rs` around lines 353 - 355, Update the error
mapping in the wide_sub_key conversion within the relevant registry query flow
so map_err returns a concrete QueryStringError::Utf16 instance containing the
conversion error, rather than the tuple variant constructor. Preserve the
existing QueryStringError return path and propagate the original FromUtf16Error
value.

---

464-471: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift

Map ContainsNul error and update downstream consumers.

Two issues exist here:

1. sub_key.to_wide_cstring() returns Result<WideCString, ContainsNul<u16>>, which cannot be implicitly converted to io::Error via ?. You must explicitly map the error.
2. The signature change of set_default_value to Result<u32, io::Error> breaks the downstream caller SetValue in crates/vm/src/stdlib/winreg.rs (which expects a raw u32 error code to perform if res == 0). You will need to update the caller to match the new Result.

🐛 Proposed fix for the local type error

 pub fn set_default_value(
     hkey: Registry::HKEY,
     sub_key: &OsStr,
     typ: u32,
     value: &OsStr,
 ) -> Result<u32, io::Error> {
     let child_key = if !sub_key.is_empty() {
-        let wide_sub_key = sub_key.to_wide_cstring()?;
+        let wide_sub_key = sub_key.to_wide_cstring().map_err(io::Error::other)?;
         let mut out_key = core::ptr::null_mut();

📝 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 fn set_default_value(
    hkey: Registry::HKEY,
    sub_key: &OsStr,
    typ: u32,
    value: &OsStr,
) -> Result<u32, io::Error> {
    let child_key = if !sub_key.is_empty() {
        let wide_sub_key = sub_key.to_wide_cstring().map_err(io::Error::other)?;

🤖 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/winreg.rs` around lines 464 - 471, Update
set_default_value to explicitly map the to_wide_cstring ContainsNul failure into
io::Error before using ?, then update the SetValue caller to handle the
Result<u32, io::Error> contract instead of comparing the result directly with
zero; preserve the existing success and Windows error-code behavior.

---

512-515: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Avoid taking ownership of a borrowed reference and eliminate unnecessary allocations.

Type error: into_vec_with_nul consumes a WideCString by value, but input is a reference (&WideCStr). This will fail to compile because you cannot move out of a shared reference.

Since ExpandEnvironmentStringsW only requires a pointer, you can avoid allocating a new Vec entirely by passing input.as_ptr() directly.

🐛 Proposed fix

 pub fn expand_environment_strings(
     input: &WideCStr,
 ) -> Result<String, ExpandEnvironmentStringsError> {
-    let wide_input = input.into_vec_with_nul();
     let required_size = unsafe {
-        Environment::ExpandEnvironmentStringsW(wide_input.as_ptr(), core::ptr::null_mut(), 0)
+        Environment::ExpandEnvironmentStringsW(input.as_ptr(), core::ptr::null_mut(), 0)
     };
     if required_size == 0 {
         return Err(ExpandEnvironmentStringsError::Os);
     }
 
     let mut out = vec![0u16; required_size as usize];
     let written = unsafe {
-        Environment::ExpandEnvironmentStringsW(wide_input.as_ptr(), out.as_mut_ptr(), required_size)
+        Environment::ExpandEnvironmentStringsW(input.as_ptr(), out.as_mut_ptr(), required_size)
     };

Also applies to: 524-526

🤖 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/winreg.rs` around lines 512 - 515, Update
expand_environment_strings to stop calling into_vec_with_nul on the borrowed
input; pass input.as_ptr() directly to ExpandEnvironmentStringsW and remove the
unnecessary wide_input allocation while preserving the existing expansion
behavior.

crates/vm/src/stdlib/_ctypes/function.rs (1)

779-780: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Validate interior NULs for function symbol names.

Using format!("{name}\0") allows any interior NUL bytes in name to pass through into the resulting byte slice, which can cause the underlying C-style API (e.g., dlsym or GetProcAddress) to truncate the string and look up an unintended symbol. Since this PR aims to secure FFI paths against interior NULs, you should validate name as well.

🛡️ Proposed fix to prevent FFI string truncation

-            let terminated = format!("{name}\0");
+            let terminated = rustpython_host_env::ctypes::null_terminated_bytes(name.as_bytes())
+                .map_err(|e| e.to_pyexception(vm))?;
             let ptr_val = match rustpython_host_env::ctypes::lookup_function_symbol_addr(
                 handle
                     .to_usize()
                     .ok_or_else(|| vm.new_value_error("Invalid handle"))?,
-                terminated.as_bytes(),
+                &terminated,
             ) {

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

            let terminated = rustpython_host_env::ctypes::null_terminated_bytes(name.as_bytes())
                .map_err(|e| e.to_pyexception(vm))?;
            let ptr_val = match rustpython_host_env::ctypes::lookup_function_symbol_addr(
                handle
                    .to_usize()
                    .ok_or_else(|| vm.new_value_error("Invalid handle"))?,
                &terminated,
            ) {

🤖 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/vm/src/stdlib/_ctypes/function.rs` around lines 779 - 780, Validate
the function symbol name before constructing the terminated string in the
surrounding function of the lookup_function_symbol_addr call. Reject names
containing interior NUL bytes and return the existing error path, then preserve
the current format!("{name}\0") lookup flow for valid names.

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

844-846: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Provide the vm context parameter.

Type error: to_pyexception() requires the vm parameter (&VirtualMachine) to instantiate the Python exception. This will cause a compilation error.

🐛 Proposed fix

     fn ExpandEnvironmentStrings(i: String, vm: &VirtualMachine) -> PyResult<String> {
-        let i = WideCString::from_str(&i).map_err(|err| err.to_pyexception())?;
+        let i = WideCString::from_str(&i).map_err(|err| err.to_pyexception(vm))?;
         host_winreg::expand_environment_strings(&i).map_err(|err| err.to_pyexception(vm))
     }

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

    fn ExpandEnvironmentStrings(i: String, vm: &VirtualMachine) -> PyResult<String> {
        let i = WideCString::from_str(&i).map_err(|err| err.to_pyexception(vm))?;
        host_winreg::expand_environment_strings(&i).map_err(|err| err.to_pyexception(vm))
    }

🤖 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/vm/src/stdlib/winreg.rs` around lines 844 - 846, Update the error
mapping in the `WideCString::from_str` conversion within the surrounding winreg
function to pass the available `vm` context to `to_pyexception`, matching the
existing `expand_environment_strings` mapping. Leave the successful conversion
and environment expansion behavior unchanged.

crates/wtf8/src/lib.rs (2)

1532-1548: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Reject all source NULs consistently. Both encoders synthesize their own terminator, so every NUL found in the input is interior and must fail.

• crates/wtf8/src/lib.rs#L1532-L1548: return InteriorNulError immediately for any source NUL and mark the iterator complete.
• crates/host_env/src/ctypes.rs#L545-L557: apply the same rule and prevent iteration from resuming after the error.

📍 Affects 2 files

• crates/wtf8/src/lib.rs#L1532-L1548 (this comment)
• crates/host_env/src/ctypes.rs#L545-L557

🤖 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/wtf8/src/lib.rs` around lines 1532 - 1548, Reject every source NUL
immediately in the encoder iterator, returning InteriorNulError and setting the
iterator’s completion state so iteration cannot resume. Apply this change at
crates/wtf8/src/lib.rs lines 1532-1548 and crates/host_env/src/ctypes.rs lines
545-557, preserving the synthesized terminator behavior while removing the
scan-and-accept path.

---

1557-1560: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Correct the iterator’s size_hint.

The iterator can emit an additional terminator, while an early interior-NUL error can produce fewer items than the wrapped iterator’s lower bound. Forwarding the original hint violates both bounds.

Proposed fix

 fn size_hint(&self) -> (usize, Option<usize>) {
-    self.iter.size_hint()
+    if self.complete {
+        return (0, Some(0));
+    }
+    let (_, upper) = self.iter.size_hint();
+    (1, upper.and_then(|len| len.checked_add(1)))
 }

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

    #[inline]
    fn size_hint(&self) -> (usize, Option<usize>) {
        if self.complete {
            return (0, Some(0));
        }
        let (_, upper) = self.iter.size_hint();
        (1, upper.and_then(|len| len.checked_add(1)))
    }

🤖 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/wtf8/src/lib.rs` around lines 1557 - 1560, Update the size_hint method
to account for the iterator’s possible terminator output and early termination
on interior-NUL errors; do not forward self.iter.size_hint() unchanged. Return
bounds that never overstate the minimum or maximum number of items the iterator
can emit, preserving the appropriate unbounded case.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

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

384-413: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Add Windows regression tests for embedded NUL preservation.

Test "a\0b" with mbcs, oem, and cp65001. Assert that the encoded result retains the NUL byte and the trailing "b". This prevents a future reintroduction of C-string truncation. OsStrExt::encode_wide() does not append a terminator. (doc.rust-lang.org)

Also applies to: 517-535, 880-896

🤖 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/vm/src/stdlib/_codecs.rs` around lines 384 - 413, Add Windows
regression tests covering encoding "a\0b" with the mbcs, oem, and cp65001
codecs, asserting the output preserves both the embedded NUL byte and trailing
"b". Place the tests alongside the existing codec tests for the affected
implementations, including the additional locations noted in the comment,
without changing encoding behavior.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes