ffi: No interior NULs (part 1)#8245
Conversation
Interior NULs is a security hazard for C-style strings. A NUL byte truncates a string which can lead the caller and callee to see two different strings. It can cause path traversal attacks where a path in Python looks complete but it is interpreted differently through FFI. RustPython needs to handle this for its C-API as well as raw libc or Windows calls. Both Rust's standard library as well as Rustix handle interior NULs for us. Finally, this PR is non-exhaustive. I will have to rely heavily on CodeRabbit to help lint it to ensure that interior NUL checks are only introduced for FFI and not outside of it. **AI disclosure:** I relied on AI to ensure I'm solving this problem correctly. Mainly, I used it to check if the FFI functions I'm modifying need to handle interior NULs. **Sources:** * https://owasp.org/www-community/attacks/Embedding_Null_Code * python/cpython#11656 Assisted-by: Codex
📝 WalkthroughWalkthroughThis PR reworks string/byte conversion helpers used for FFI and ctypes to propagate interior-NUL errors instead of silently truncating or inserting terminators. A new ChangesFallible NUL-terminated string conversions
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PyCode as Python ctypes call
participant ConvParam as function::conv_param
participant Base as base::ensure_z_null_terminated / str_to_wchar_bytes
participant HostEnv as host_env::ctypes helpers
participant Wtf8 as Wtf8::encode_wide_ffi
PyCode->>ConvParam: pass PyStr/PyBytes argument
ConvParam->>Base: convert to null-terminated bytes
Base->>HostEnv: null_terminated_bytes / wchar_nul_terminated_bytes
HostEnv->>Wtf8: encode_wide_ffi() for wide strings
Wtf8-->>HostEnv: Ok(bytes) or Err(InteriorNulError)
HostEnv-->>Base: Result<Vec<u8>, Error>
Base-->>ConvParam: Result<(PyObjectRef, usize), Error>
ConvParam-->>PyCode: propagate error via ? or continue call
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/host_env/src/fileutils.rs (1)
80-97: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winAdd the missing
Ok(())and propagate the result
crates/host_env/src/fileutils.rs:76-97currently ends with(), even though the signature returnsResult<(), io::Error>, so this won’t compile. Thecrates/host_env/src/nt.rs:831,836,857call sites also discard the returnedResult, which dropspath.to_wide()?failures; thread it through with?.🤖 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/fileutils.rs` around lines 80 - 97, The helper in fileutils.rs returns Result<(), io::Error> but currently falls off the end with unit, so add an explicit Ok(()) after the permission update logic. Also make the nt.rs callers that use this helper propagate its Result instead of ignoring it, so failures from path.to_wide()? are not dropped; update the call sites around the file metadata handling to use ? and thread the error upward.crates/vm/src/stdlib/_ctypes/base.rs (1)
1596-1596: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winHandle the fallible
str_to_wchar_bytesresult here. This call still destructures aResult;?only works onceInteriorNulErroris mapped intoPyBaseExceptionRef, so either add that conversion or convert the error explicitly at this call site.🤖 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/base.rs` at line 1596, The call in the wchar conversion path is still destructuring a fallible `str_to_wchar_bytes` result directly, so update the logic around `str_to_wchar_bytes` to properly handle its `Result` before destructuring. In the `_ctypes::base` code path that builds the wide string buffer, either add a conversion from `InteriorNulError` into `PyBaseExceptionRef` so `?` can be used cleanly, or explicitly map the error at the call site before extracting `holder` and `ptr`.
🤖 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/ctypes.rs`:
- Around line 542-546: The vec_into_bytes helper currently reinterprets the
original allocation with Vec::from_raw_parts, which can deallocate with the
wrong layout for wide-string buffers. Update vec_into_bytes in ctypes.rs to copy
the bytes out of the source Vec<T> instead of casting the allocation; keep the
existing size_of::<T>() guard, but replace the raw-parts reconstruction with a
safe byte copy approach so the returned Vec<u8> owns a correctly laid-out
allocation.
In `@crates/host_env/src/windows.rs`:
- Around line 413-417: The `to_wide` method in `windows.rs` is using `io::Error`
as a direct mapper, which won’t compile in this context. Update the
`WideCString::from_os_str(self)` error handling to use `io::Error::other`,
matching the pattern used by the other conversion methods, while keeping the
rest of `to_wide` unchanged.
- Around line 428-435: The Wtf8 implementation of ToWideString is incomplete and
uses the wrong encoder for the checked Result-based API. In the ToWideString
impl for Wtf8, add the missing to_wide_cstring method alongside to_wide and
to_wide_with_nul, and update all three methods to use encode_wide_ffi() so they
return Result<Vec<u16>, io::Error> correctly and reject interior NULs. Keep the
fix localized to the Wtf8 trait implementation in windows.rs.
In `@crates/vm/src/stdlib/_ctypes/function.rs`:
- Line 158: In the ctypes conversion helpers, the `?` operator is being used on
errors that do not automatically convert into `PyBaseExceptionRef`, so fix the
error handling in the functions that call
`rustpython_host_env::ctypes::utf16z_bytes` and `null_terminated_bytes` by
explicitly mapping those `InteriorNulError` and `NulError` values into the
Python exception type before propagating them. Apply the same change in the
corresponding logic in `function.rs` and `base.rs`, keeping the conversion
localized near the existing `utf16z_bytes` / `null_terminated_bytes` calls.
In `@crates/wtf8/src/lib.rs`:
- Around line 915-916: The doc comment on encode_wide_ffi names the wrong
encoding: it currently says the function converts to potentially ill-formed
UTF-8, but this helper returns potentially ill-formed UTF-16 wide code units
like encode_wide. Update the comment text for encode_wide_ffi to describe UTF-16
instead of UTF-8, keeping the note about checking for interior NULs.
---
Outside diff comments:
In `@crates/host_env/src/fileutils.rs`:
- Around line 80-97: The helper in fileutils.rs returns Result<(), io::Error>
but currently falls off the end with unit, so add an explicit Ok(()) after the
permission update logic. Also make the nt.rs callers that use this helper
propagate its Result instead of ignoring it, so failures from path.to_wide()?
are not dropped; update the call sites around the file metadata handling to use
? and thread the error upward.
In `@crates/vm/src/stdlib/_ctypes/base.rs`:
- Line 1596: The call in the wchar conversion path is still destructuring a
fallible `str_to_wchar_bytes` result directly, so update the logic around
`str_to_wchar_bytes` to properly handle its `Result` before destructuring. In
the `_ctypes::base` code path that builds the wide string buffer, either add a
conversion from `InteriorNulError` into `PyBaseExceptionRef` so `?` can be used
cleanly, or explicitly map the error at the call site before extracting `holder`
and `ptr`.
🪄 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: de55e068-6ea7-4cd4-8e9e-98e767127b8a
📒 Files selected for processing (6)
crates/host_env/src/ctypes.rscrates/host_env/src/fileutils.rscrates/host_env/src/windows.rscrates/vm/src/stdlib/_ctypes/base.rscrates/vm/src/stdlib/_ctypes/function.rscrates/wtf8/src/lib.rs
| unsafe fn vec_into_bytes<T: Copy>(vec: Vec<T>) -> Vec<u8> { | ||
| assert!(size_of::<T>() != 0); | ||
| let (ptr, len, cap) = vec.into_raw_parts(); | ||
| unsafe { Vec::from_raw_parts(ptr.cast::<u8>(), len * size_of::<T>(), cap * size_of::<T>()) } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Vec::from_raw_parts alignment requirement different type u8 u16
💡 Result:
Using Vec::from_raw_parts to change the type of a Vec from T to U is generally unsafe if T and U have different alignment requirements [1][2]. For Vec::from_raw_parts to be sound, the new type T must have the same alignment as the type that was originally used to allocate the memory [1][2]. This is because Rust's allocator must deallocate memory using the same layout (size and alignment) that was used during allocation [1][2]. When a Vec is dropped, it uses its type's alignment to deallocate the buffer; if you construct a Vec (alignment 1) from a pointer originally allocated for a Vec (alignment 2), the deallocation will be performed with incorrect alignment, which results in undefined behavior and potential corruption of the allocator's internal structures [1][2]. Even if the size in bytes (size_of:: * capacity) is preserved, the alignment requirement remains strict [1][2]. Therefore, you cannot simply cast a pointer from a Vec to a Vec and pass it to Vec::from_raw_parts [1][2]. If you need to view data as a different type, the recommended, safe approach is to use slice::from_raw_parts to create a slice of the desired type rather than constructing a new Vec, provided the pointer is properly aligned for the new type [1][2]. Creating a new Vec with a different type should only be done if you are certain the alignment requirements are identical or if you are managing the allocation manually in a way that respects the original allocation's layout [3].
Citations:
- 1: https://doc.rust-lang.org/stable/std/vec/struct.Vec.html
- 2: https://doc.rust-lang.org/std/vec/struct.Vec.html
- 3: https://users.rust-lang.org/t/can-element-size-change-in-vec-from-raw-parts/47370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- file excerpt ---\n'
sed -n '520,560p' crates/host_env/src/ctypes.rs
printf '\n--- call sites ---\n'
rg -n "vec_into_bytes|wchar_nul_terminated_bytes|utf16z_bytes|into_raw_parts" crates/host_env/src/ctypes.rs
printf '\n--- toolchain hints ---\n'
git ls-files | rg '(^|/)(rust-toolchain(\.toml)?|Cargo\.toml)$' || true
if [ -f rust-toolchain.toml ]; then cat -n rust-toolchain.toml; fi
if [ -f rust-toolchain ]; then cat -n rust-toolchain; fi
if [ -f Cargo.toml ]; then rg -n 'rust-version|edition' Cargo.toml; fi
find . -maxdepth 2 \( -name 'rust-toolchain' -o -name 'rust-toolchain.toml' -o -name 'Cargo.toml' \) -print | sortRepository: RustPython/RustPython
Length of output: 250
🌐 Web query:
Rust Vec::into_raw_parts stabilized version stable or nightly
💡 Result:
The Rust method Vec::into_raw_parts is stable as of Rust 1.93.0 [1][2]. It is no longer an unstable or nightly-only feature [3][4]. The method decomposes a Vec into its raw components: (pointer, length, capacity) [3][2]. As of the current date of July 9, 2026, it is available in the stable release of the Rust standard library [3][4].
Citations:
- 1: https://github.com/rust-lang/rust/blob/7d8ebe3128fc87f3da1ad64240e63ccf07b8f0bd/library/alloc/src/vec/mod.rs
- 2: https://doc.rust-lang.org/src/alloc/vec/mod.rs.html
- 3: https://doc.rust-lang.org/stable/std/vec/struct.Vec.html?search=
- 4: https://doc.rust-lang.org/stable/std/vec/struct.Vec.html?search=retain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file excerpt ---'
sed -n '520,560p' crates/host_env/src/ctypes.rs
echo
echo '--- call sites ---'
rg -n "vec_into_bytes|wchar_nul_terminated_bytes|utf16z_bytes|encode_wide_ffi|into_raw_parts" crates/host_env/src/ctypes.rs
echo
echo '--- toolchain files ---'
find . -maxdepth 2 \( -name 'rust-toolchain' -o -name 'rust-toolchain.toml' -o -name 'Cargo.toml' \) -print | sort
if [ -f rust-toolchain.toml ]; then echo 'rust-toolchain.toml:'; cat -n rust-toolchain.toml; fi
if [ -f rust-toolchain ]; then echo 'rust-toolchain:'; cat -n rust-toolchain; fi
if [ -f Cargo.toml ]; then echo 'Cargo.toml hints:'; rg -n 'rust-version|edition' Cargo.toml; fiRepository: RustPython/RustPython
Length of output: 2118
🌐 Web query:
Rust std Vec::from_raw_parts deallocate same layout alignment documentation
💡 Result:
Yes, Vec::from_raw_parts requires that the memory be deallocated using the exact same layout (size and alignment) that was used during the original allocation [1][2]. This is a strict safety requirement because the Rust allocator (GlobalAlloc/Allocator) must be called with a layout that matches the one used to allocate the memory [1][2][3]. The safety documentation for Vec::from_raw_parts explicitly states [1][2]: - T must have the same alignment as what the pointer was originally allocated with. Providing a type with a less strict alignment is insufficient; the alignment must be exactly the same [1][2]. - The total size (size of T multiplied by the capacity) must be the same as the size used during the original allocation [1][2]. - Capacity must be the same as the capacity used when the memory was originally allocated [1][4][2]. Violating these requirements, such as by casting the pointer to a type with a different alignment or size, causes undefined behavior because it results in the Vec being dropped with an incorrect layout, which can corrupt the allocator's internal data structures [1][2]. For example, if a buffer is allocated as Vec (alignment 2) and subsequently turned into a Vec (alignment 1), the Vec will attempt to deallocate the memory with alignment 1, which violates the requirement to match the original allocation [1][2]. To avoid these issues, if you need to work with a different type or layout, it is generally recommended to use slice::from_raw_parts instead of constructing a new Vec [1][2].
Citations:
- 1: https://doc.rust-lang.org/std/vec/struct.Vec.html
- 2: https://doc.rust-lang.org/stable/std/vec/struct.Vec.html
- 3: https://stackoverflow.com/questions/38840330/what-happens-if-i-call-vecfrom-raw-parts-with-a-smaller-capacity-than-the-poin
- 4: https://doc.rust-lang.org/stable/alloc/vec/struct.Vec.html
Rebuild the wide-string bytes with a copy, not Vec::from_raw_parts
Vec::from_raw_parts must use the same allocation layout, including alignment. Casting the Vec<u16> buffer to Vec<u8> and dropping it deallocates with a different layout, which is undefined behavior. Convert the data by copying the bytes instead.
🤖 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 542 - 546, The vec_into_bytes
helper currently reinterprets the original allocation with Vec::from_raw_parts,
which can deallocate with the wrong layout for wide-string buffers. Update
vec_into_bytes in ctypes.rs to copy the bytes out of the source Vec<T> instead
of casting the allocation; keep the existing size_of::<T>() guard, but replace
the raw-parts reconstruction with a safe byte copy approach so the returned
Vec<u8> owns a correctly laid-out allocation.
| fn to_wide(&self) -> Result<Vec<u16>, io::Error> { | ||
| WideCString::from_os_str(self) | ||
| .map(WideCString::into_vec) | ||
| .map_err(io::Error) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of crates/host_env/src/windows.rs with line numbers.
sed -n '390,450p' crates/host_env/src/windows.rs | cat -n
# Show the trait/impl context around to_wide/to_wide_with_nul if present.
rg -n "fn to_wide|fn to_wide_with_nul|impl .*Wide" crates/host_env/src/windows.rsRepository: RustPython/RustPython
Length of output: 2417
io::Error can't be used as a mapper here. .map_err(io::Error) won't compile; use io::Error::other like the other methods.
🤖 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 413 - 417, The `to_wide` method
in `windows.rs` is using `io::Error` as a direct mapper, which won’t compile in
this context. Update the `WideCString::from_os_str(self)` error handling to use
`io::Error::other`, matching the pattern used by the other conversion methods,
while keeping the rest of `to_wide` unchanged.
| impl ToWideString for Wtf8 { | ||
| fn to_wide(&self) -> Vec<u16> { | ||
| fn to_wide(&self) -> Result<Vec<u16>, io::Error> { | ||
| self.encode_wide().collect() | ||
| } | ||
| fn to_wide_with_nul(&self) -> Vec<u16> { | ||
| fn to_wide_with_nul(&self) -> Result<Vec<u16>, io::Error> { | ||
| self.encode_wide().chain(Some(0)).collect() | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show file size and relevant region around the reported lines.
wc -l crates/host_env/src/windows.rs
sed -n '360,520p' crates/host_env/src/windows.rs
# Map the file structure for the relevant symbols.
ast-grep outline crates/host_env/src/windows.rs --view expandedRepository: RustPython/RustPython
Length of output: 5098
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the exact trait definition and the Wtf8 impl with line numbers.
grep -n "trait ToWideString\|impl ToWideString for Wtf8\|encode_wide_ffi\|to_wide_cstring" -n crates/host_env/src/windows.rs || true
nl -ba crates/host_env/src/windows.rs | sed -n '1,260p'
nl -ba crates/host_env/src/windows.rs | sed -n '380,470p'Repository: RustPython/RustPython
Length of output: 400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate Wtf8 definitions and wide-encoding methods.
rg -n "impl .*AsRef<OsStr> for Wtf8|impl .*Wtf8|fn encode_wide_ffi|fn encode_wide\(" . --glob '!target' --glob '!**/target/**'
# Show the likely source file(s) around Wtf8 method definitions if present.
python3 - <<'PY'
from pathlib import Path
for path in Path('.').rglob('*.rs'):
txt = path.read_text(errors='ignore')
if 'encode_wide_ffi' in txt or 'encode_wide(' in txt and 'Wtf8' in txt:
print(path)
PYRepository: RustPython/RustPython
Length of output: 7295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the Wtf8 encoder definitions and nearby iterator types.
wc -l crates/wtf8/src/lib.rs
sed -n '880,960p' crates/wtf8/src/lib.rs
sed -n '1,120p' crates/host_env/src/windows.rs
# Show the exact Wtf8 encoders and any type names they return.
rg -n "pub fn encode_wide\(|pub fn encode_wide_ffi\(|struct EncodeWide|struct EncodeWideForFfi|type EncodeWide|type EncodeWideForFfi" crates/wtf8/src/lib.rsRepository: RustPython/RustPython
Length of output: 7661
Implement the missing trait method and switch to the checked encoder. crates/host_env/src/windows.rs:428-435 still needs to_wide_cstring, and the current encode_wide().collect() / .chain(Some(0)).collect() calls don't satisfy Result<Vec<u16>, io::Error>; use encode_wide_ffi() so interior NULs still fail.
🤖 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 428 - 435, The Wtf8
implementation of ToWideString is incomplete and uses the wrong encoder for the
checked Result-based API. In the ToWideString impl for Wtf8, add the missing
to_wide_cstring method alongside to_wide and to_wide_with_nul, and update all
three methods to use encode_wide_ffi() so they return Result<Vec<u16>,
io::Error> correctly and reject interior NULs. Keep the fix localized to the
Wtf8 trait implementation in windows.rs.
| // 4. Python str -> wide string pointer (like PyUnicode_AsWideCharString) | ||
| if let Some(s) = value.downcast_ref::<PyStr>() { | ||
| let wide_bytes = rustpython_host_env::ctypes::utf16z_bytes(s.as_wtf8()); | ||
| let wide_bytes = rustpython_host_env::ctypes::utf16z_bytes(s.as_wtf8())?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'From<\s*(InteriorNulError|alloc::ffi::NulError|NulError)\s*>' cratesRepository: RustPython/RustPython
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files 'crates/vm/src/stdlib/_ctypes/*' 'crates/vm/src/*.rs' | sed -n '1,120p'
printf '\n== search for conversion impls ==\n'
rg -n --hidden --glob '!target' --glob '!**/target/**' \
'From<[^>]*(InteriorNulError|NulError|alloc::ffi::NulError)[^>]*>' crates || true
printf '\n== search for the relevant helpers and call sites ==\n'
rg -n --hidden --glob '!target' --glob '!**/target/**' \
'utf16z_bytes|null_terminated_bytes|conv_param|InteriorNulError|NulError' \
crates/vm/src/stdlib/_ctypes crates/vm/src | sed -n '1,220p'Repository: RustPython/RustPython
Length of output: 6491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== PyErr conversion impls ==\n'
rg -n --hidden --glob '!target' --glob '!**/target/**' \
'impl\s+From<.*for\s+PyErr|impl\s+From<.*for\s+PyBaseExceptionRef|ToPyException' \
crates/vm/src crates/common/src crates -g '*.rs' | sed -n '1,240p'
printf '\n== exception type aliases / definitions ==\n'
rg -n --hidden --glob '!target' --glob '!**/target/**' \
'type\s+PyResult|type\s+PyBaseExceptionRef|struct\s+PyErr|enum\s+PyErr|trait\s+ToPyException' \
crates/vm/src crates/common/src crates -g '*.rs' | sed -n '1,220p'
printf '\n== explicit InteriorNulError coverage ==\n'
rg -n --hidden --glob '!target' --glob '!**/target/**' \
'InteriorNulError' crates -g '*.rs' | sed -n '1,220p'Repository: RustPython/RustPython
Length of output: 10543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== conversion plumbing ==\n'
sed -n '1,220p' crates/vm/src/convert/to_pyobject.rs
printf '\n== exception conversions around NulError ==\n'
sed -n '1190,1245p' crates/vm/src/exceptions.rs
printf '\n== any InteriorNulError conversion impls ==\n'
rg -n --hidden --glob '!target' --glob '!**/target/**' \
'impl\s+(ToPyException|From<.*>)\s+for\s+.*InteriorNulError|InteriorNulError.*ToPyException|From<InteriorNulError>' \
crates -g '*.rs' | sed -n '1,200p'Repository: RustPython/RustPython
Length of output: 3264
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== generic From<E> for PyBaseExceptionRef / PyErr ==\n'
rg -n --hidden --glob '!target' --glob '!**/target/**' \
'impl\s*<[^>]*\bE\b[^>]*>\s*From<\s*E\s*>\s*for\s*(PyBaseExceptionRef|PyErr)|impl\s*<[^>]*>\s*From<[^>]*ToPyException[^>]*>\s*for\s*(PyBaseExceptionRef|PyErr)|FromResidual' \
crates -g '*.rs' | sed -n '1,240p'
printf '\n== convert module files ==\n'
git ls-files crates/vm/src/convert/*.rs crates/vm/src/exceptions.rs | sed -n '1,120p'
printf '\n== any impl blocks involving PyBaseExceptionRef in exceptions.rs ==\n'
rg -n --hidden --glob '!target' --glob '!**/target/**' \
'PyBaseExceptionRef|PyErr' crates/vm/src/exceptions.rs crates/vm/src/convert/*.rs | sed -n '1,240p'Repository: RustPython/RustPython
Length of output: 213
Add an explicit conversion for these ?s
utf16z_bytes(...) and null_terminated_bytes(...) need an explicit conversion into PyBaseExceptionRef; there’s no matching From<InteriorNulError> / From<alloc::ffi::NulError> path here, so ? won’t work as written. The same pattern appears in crates/vm/src/stdlib/_ctypes/base.rs.
🤖 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` at line 158, In the ctypes
conversion helpers, the `?` operator is being used on errors that do not
automatically convert into `PyBaseExceptionRef`, so fix the error handling in
the functions that call `rustpython_host_env::ctypes::utf16z_bytes` and
`null_terminated_bytes` by explicitly mapping those `InteriorNulError` and
`NulError` values into the Python exception type before propagating them. Apply
the same change in the corresponding logic in `function.rs` and `base.rs`,
keeping the conversion localized near the existing `utf16z_bytes` /
`null_terminated_bytes` calls.
| /// Converts the WTF-8 string to potentially ill-formed UTF-8 while checking for interior | ||
| /// NULs. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc comment names the wrong encoding.
encode_wide_ffi produces potentially ill-formed UTF-16 wide code units (same as encode_wide), not UTF-8. The doc text was likely copied and should say UTF-16.
📝 Proposed doc fix
- /// Converts the WTF-8 string to potentially ill-formed UTF-8 while checking for interior
- /// NULs.
+ /// Converts the WTF-8 string to potentially ill-formed UTF-16 while checking for interior
+ /// NULs.📝 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.
| /// Converts the WTF-8 string to potentially ill-formed UTF-8 while checking for interior | |
| /// NULs. | |
| /// Converts the WTF-8 string to potentially ill-formed UTF-16 while checking for interior | |
| /// NULs. |
🤖 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 915 - 916, The doc comment on
encode_wide_ffi names the wrong encoding: it currently says the function
converts to potentially ill-formed UTF-8, but this helper returns potentially
ill-formed UTF-16 wide code units like encode_wide. Update the comment text for
encode_wide_ffi to describe UTF-16 instead of UTF-8, keeping the note about
checking for interior NULs.
(This is WIP; the Windows code doesn't even compile yet 😆 Opening as placeholder + CodeRabbit lints )
Interior NULs is a security hazard for C-style strings. A NUL byte truncates a string which can lead the caller and callee to see two different strings. It can cause path traversal attacks where a path in Python looks complete but it is interpreted differently through FFI.
RustPython needs to handle this for its C-API as well as raw libc or Windows calls. Both Rust's standard library as well as Rustix handle interior NULs for us.
Finally, this PR is non-exhaustive. I will have to rely heavily on CodeRabbit to help lint it to ensure that interior NUL checks are only introduced for FFI and not outside of it.
AI disclosure: I relied on AI to ensure I'm solving this problem correctly. Mainly, I used it to check if the FFI functions I'm modifying need to handle interior NULs.
Sources:
Assisted-by: Codex
Summary
Summary by CodeRabbit
New Features
Bug Fixes