host_env: Fix wcslen, leverage NonNull<T> more by joshuamegnauth54 · Pull Request #8386 · RustPython/RustPython

joshuamegnauth54 · 2026-07-26T18:18:45Z

strlen was both unused and unneeded. Rust's CStr handles the length for us without needing to call into libc manually or implementing our own version of strlen. Our strlen implemented a fallback that incremented and dereferenced a pointer to check for NUL. This is slower than what libc usually does. For example, musl operates on words instead of individual bytes. This implementation also caused UB if the caller passed in a null pointer.

wcslen is similar. The currently implementation can cause UB for null pointers. It's also slow from the same reason mentioned above. Luckily, the widestring crate can handle this for us. The crate panics on null pointers, so using it was a good excuse to use NonNull in more places.

[] Closes #xxxx
This PR follows our AI policy

Summary

Fix some Windows UB.
Ensure non-null pointers at the margin for some APIs.

Summary by CodeRabbit

Bug Fixes
- Improved wide-character (UTF-16) string handling in ctypes operations with safer pointer validation to reduce invalid reads.
- Corrected COM/WMI UTF-16 string length measurement and serialization to handle null/invalid inputs more reliably.
- Fixed wide-character pointer slice behavior to return an empty string when the slice is empty or no valid wide data can be decoded.
- Improved Windows wide-string to application-text conversion for more consistent results across call paths.

coderabbitai · 2026-07-26T18:18:51Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1a67ce81-69e8-45c0-a89e-13701deb7866

📥 Commits

Reviewing files that changed from the base of the PR and between 31608c2 and 021cca1.

📒 Files selected for processing (4)

crates/host_env/Cargo.toml
crates/host_env/src/ctypes.rs
crates/host_env/src/wmi.rs
crates/vm/src/stdlib/_ctypes/pointer.rs

🚧 Files skipped from review as they are similar to previous changes (4)

crates/vm/src/stdlib/_ctypes/pointer.rs
crates/host_env/Cargo.toml
crates/host_env/src/wmi.rs
crates/host_env/src/ctypes.rs

📝 Walkthrough

Walkthrough

Wide-string pointer handling now uses NonNull<WChar>, shared wcslen logic, WideCStr decoding, and explicit null-result handling across host environment and VM ctypes paths.

Changes

Wide-string safety and decoding

Layer / File(s)	Summary
Shared length contract and WMI integration `crates/host_env/Cargo.toml`, `crates/host_env/src/ctypes.rs`, `crates/host_env/src/wmi.rs`	`wcslen` now accepts `NonNull<WChar>` and is reused by WMI, whose property buffers now check for null pointers before calculating lengths.
Non-null decoding paths `crates/host_env/src/ctypes.rs`	Wide-string callbacks, bounded and strided reads, `wstring_at`, pointer slices, and Windows conversion now use the updated non-null and `WideCStr`-based APIs.
c_wchar slice result handling `crates/vm/src/stdlib/_ctypes/pointer.rs`	`c_wchar` slicing returns an empty string when the requested length is zero or the pointer read returns `None`.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

RustPython/RustPython#7043: Both changes touch the c_wchar pointer-slice handling path.

Suggested reviewers: youknowone, shaharnaveh

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main changes: fixing wcslen and increasing use of NonNull pointers.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✨ 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.}

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (1)

crates/host_env/src/ctypes.rs (1)
2255-2255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pass the slice directly.

wchars is already a slice; use Wtf8Buf::from_wide(wchars). Run Clippy to confirm and fix any introduced lint. As per coding guidelines, “Always run clippy to lint code with cargo clippy before completing tasks and fix any warnings or lints introduced by changes.”
🤖 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` at line 2255, Update the Wtf8Buf::from_wide
call to pass the existing wchars slice directly instead of taking a reference to
it, then run cargo clippy and resolve any warnings introduced by this change.
Source: Coding guidelines

🤖 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 2414-2418: Update the function returning Option<Wtf8Buf> to reject
a null ptr_value before computing any offset. Use checked arithmetic for the
start * wchar_size offset and address calculation, returning None on overflow;
only construct NonNull and call read_wide_string_strided after these validations
succeed.

In `@crates/host_env/src/wmi.rs`:
- Around line 474-483: Update the nullable length handling in the inner loop
around prop_name and prop_str so both failure cases clear prop_value with
VariantClear before breaking. Extract the optional length result first, route
either failure through the shared cleanup path, and preserve the existing loop
behavior without duplicating cleanup logic.

---

Nitpick comments:
In `@crates/host_env/src/ctypes.rs`:
- Line 2255: Update the Wtf8Buf::from_wide call to pass the existing wchars
slice directly instead of taking a reference to it, then run cargo clippy and
resolve any warnings introduced by this change.

🪄 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 Plus

Run ID: 94260c2a-7ee6-4bd9-be79-0dfe4b325fe9

📥 Commits

Reviewing files that changed from the base of the PR and between 3aaec06 and a2827bc.

📒 Files selected for processing (4)

crates/host_env/Cargo.toml
crates/host_env/src/ctypes.rs
crates/host_env/src/wmi.rs
crates/vm/src/stdlib/_ctypes/pointer.rs

coderabbitai · 2026-07-27T18:00:47Z

+                    let Some(cb_str1) = NonNull::new(prop_name)
+                        .map(|prop_name| (unsafe { wcslen(prop_name) } * 2) as u32)
+                    else {
+                        break;
+                    };
+                    let Some(cb_str2) = NonNull::new(prop_str.as_ptr().cast_mut())
+                        .map(|prop_str| (unsafe { wcslen(prop_str) } * 2) as u32)
+                    else {
+                        break;
+                    };


🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Clear the VARIANT before the new early break.

A null prop_name exits the inner loop before VariantClear runs, leaking any resources owned by prop_value. Route both nullable-length failures through shared cleanup before breaking. As per coding guidelines, “When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code.”

🤖 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/wmi.rs` around lines 474 - 483, Update the nullable length handling in the inner loop around prop_name and prop_str so both failure cases clear prop_value with VariantClear before breaking. Extract the optional length result first, route either failure through the shared cleanup path, and preserve the existing loop behavior without duplicating cleanup logic.

Source: Coding guidelines

`strlen` was both unused and unneeded. Rust's `CStr` handles the length for us without needing to call into `libc` manually or implementing our own version of `strlen`. Our `strlen` implemented a fallback that incremented and dereferenced a pointer to check for NUL. This is slower than what `libc` usually does. For example, `musl` operates on words instead of individual bytes. This implementation also caused UB if the caller passed in a null pointer. `wcslen` is similar. The currently implementation can cause UB for null pointers. It's also slow from the same reason mentioned above. Luckily, the widestring crate can handle this for us. The crate panics on null pointers, so using it was a good excuse to use NonNull in more places.

youknowone

Thanks!

joshuamegnauth54 marked this pull request as ready for review July 27, 2026 17:55

joshuamegnauth54 force-pushed the unused-strlen branch from da37bda to a2827bc Compare July 27, 2026 17:55

coderabbitai Bot reviewed Jul 27, 2026

View reviewed changes

joshuamegnauth54 marked this pull request as draft July 28, 2026 02:33

joshuamegnauth54 force-pushed the unused-strlen branch from a2827bc to 783d82b Compare July 28, 2026 02:34

joshuamegnauth54 marked this pull request as ready for review July 29, 2026 01:47

joshuamegnauth54 force-pushed the unused-strlen branch 2 times, most recently from 18c0d92 to 31608c2 Compare July 29, 2026 02:04

joshuamegnauth54 force-pushed the unused-strlen branch from 31608c2 to 021cca1 Compare July 29, 2026 02:27

youknowone approved these changes Jul 29, 2026

View reviewed changes

youknowone merged commit 9bf458c into RustPython:main Jul 29, 2026
26 checks passed

joshuamegnauth54 deleted the unused-strlen branch July 29, 2026 18:25

coderabbitai Bot mentioned this pull request Aug 9, 2026

Use C string literals and w! instead of allocs #8474

Merged

2 tasks

Navigation Menu

Uh oh!

host_env: Fix wcslen, leverage NonNull<T> more - #8386

host_env: Fix wcslen, leverage NonNull<T> more#8386
youknowone merged 1 commit into
RustPython:mainfrom
joshuamegnauth54:unused-strlen

joshuamegnauth54 commented Jul 26, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 26, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot Jul 27, 2026

Uh oh!

youknowone left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

joshuamegnauth54 commented Jul 26, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joshuamegnauth54 commented Jul 26, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 26, 2026 •

edited

Loading