Use C string literals and w! instead of allocs - #8474
Conversation
Rust supports C string literals which automatically create a CStr with a trailing NUL. `windows-sys` provides an analogous macro for wide strings. Both of these avoid allocations which is nice for constants.
📝 WalkthroughWalkthroughWindows host environment code now uses compile-time wide strings for fixed Windows API arguments. Dynamic API lookup uses literal DLL and procedure names. WMI query conversion now validates embedded nulls and returns ChangesWindows string handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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: 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/wmi.rs`:
- Around line 558-560: Update the WQL query conversions in the affected query
path and the WQL BSTR construction to call WideCString::into_vec_with_nul()
rather than into_vec(), preserving the terminating u16 required by
SysAllocString through QueryThreadData.query.
🪄 Autofix
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: c4f9810c-e162-4b9e-996a-4def34863457
📒 Files selected for processing (5)
crates/host_env/src/fileutils.rscrates/host_env/src/nt.rscrates/host_env/src/winapi.rscrates/host_env/src/windows.rscrates/host_env/src/wmi.rs
| let query = WideCString::from_str(query_str) | ||
| .map_err(|_| ExecQueryError::Code(ERROR_INVALID_NAME))? | ||
| .into(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cargo tree -i widestring --edges normal
rg -n 'WideCString::from_str|into_vec(_with_nul)?|SysAllocString|struct QueryThreadData' \
crates/host_env/src/wmi.rs Cargo.toml Cargo.lockRepository: RustPython/RustPython
Length of output: 1110
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '240,310p' crates/host_env/src/wmi.rs
printf '\n--- 540-590 ---\n'
sed -n '540,590p' crates/host_env/src/wmi.rs
printf '\n--- Widestring references in Cargo.lock ---\n'
rg -n 'name = "widestring"|version = "1\.' Cargo.lock || true
printf '\n--- Source docs from local wide string crate if present ---\n'
fd -a 'ucstring.rs|widestring' Cargo.lock .cargo registry 2>/dev/null | head -20Repository: RustPython/RustPython
Length of output: 6019
🌐 Web query:
widestring 1.2.1 U16CString into_vec into_vec_with_nul documentation
💡 Result:
In the widestring crate, U16CString is an owned, mutable, nul-terminated 16-bit wide string designed for FFI [1][2][3]. The methods into_vec and into_vec_with_nul are used to consume the U16CString and convert it into a standard Rust Vec [1][4]. into_vec Converts the U16CString into a Vec without a nul terminator [1][4]. The resulting vector will contain no nul values [1]. into_vec_with_nul Converts the U16CString into a Vec including the terminating nul character [1][4]. The resulting vector will contain a nul-terminator at the end and no interior nul values [1][4]. Example usage: use widestring::U16CString; let wcstr = U16CString::from_str("example").unwrap; // Convert to Vec without nul terminator let v = wcstr.into_vec; assert_eq!(v, vec!); // Convert to Vec with nul terminator let wcvec = wcstr.into_vec_with_nul; assert_eq!(wcvec[wcvec.len-1], 0);
Citations:
- 1: https://docs.rs/widestring/latest/widestring/ucstring/struct.U16CString.html
- 2: https://docs.rs/widestring/latest/widestring/ucstring/index.html
- 3: https://dokan-dev.github.io/dokan-rust-doc/html/widestring/type.U16CString.html
- 4: https://docs.rs/widestring/latest/src/widestring/ucstring.rs.html
Retain the WMI query NUL terminator before calling SysAllocString.
QueryThreadData.query: Vec<u16> is passed to SysAllocString(data.query.as_ptr()), so query_vec must include the terminating u16. WideCString::into() drops it via U16CString::<u16>::into_vec(); use into_vec_with_nul() when converting the WQL query string.
Also applies to the WQL Bstr construction at line 386.
🤖 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 558 - 560, Update the WQL query
conversions in the affected query path and the WQL BSTR construction to call
WideCString::into_vec_with_nul() rather than into_vec(), preserving the
terminating u16 required by SysAllocString through QueryThreadData.query.
Rust supports C string literals which automatically create a CStr with a trailing NUL. `windows-sys` provides an analogous macro for wide strings. Both of these avoid allocations which is nice for constants.
Rust supports C string literals which automatically create a CStr with a trailing NUL.
windows-sysprovides an analogous macro for wide strings. Both of these avoid allocations which is nice for constants.Summary
w!moreSummary by CodeRabbit