Use memchr consistently for NUL checks#8372
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change centralizes NUL-byte detection through Python string and byte helpers, marks validation failures as cold paths, propagates Unix semaphore interior-NUL errors explicitly, and updates host and standard-library integrations. ChangesNUL validation and error propagation
Estimated code review effort: 3 (Moderate) | ~25 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 |
8549281 to
6788a1e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/stdlib/src/multiprocessing.rs (1)
805-822: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
cold_path()on theSemError::InteriorNulbranches.Every other NUL-error return site touched by this PR (
grp.rs,openssl.rs::set_ciphers/new_py_ssl_socket,mmap.rs,nt.rs::execve,ssl.rs::validate_hostname) callscold_path()immediately before returning the interior-NUL error, per the PR's stated goal of marking these exceptional paths cold. These two branches inpy_newandsem_unlinkreturnexceptions::nul_char_error(vm)onSemError::InteriorNulwithout acold_path()call, leaving this file's semaphore error paths on the "hot" path unlike everywhere else.♻️ Add cold_path() to match the cohort convention
+use core::hint::cold_path; ... SemHandle::create(&args.name, value, args.unlink).map_err(|err| { if err == SemError::InteriorNul { + cold_path(); exceptions::nul_char_error(vm) } else { os_error(vm, err) } })?; ... fn sem_unlink(name: String, vm: &VirtualMachine) -> PyResult<()> { host_multiprocessing::sem_unlink(&name).map_err(|err| { if err == SemError::InteriorNul { + cold_path(); exceptions::nul_char_error(vm) } else { os_error(vm, err) } }) }Also applies to: 838-847
🤖 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/stdlib/src/multiprocessing.rs` around lines 805 - 822, Add cold_path() immediately before returning exceptions::nul_char_error(vm) in both the SemError::InteriorNul branch of py_new and the corresponding branch in sem_unlink. Keep the existing non-NUL error handling unchanged and match the cold-path convention used by the other touched NUL-error sites.
🤖 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/winapi.rs`:
- Around line 312-315: Update the NUL-check condition in the environment-block
validation flow to call .is_some() on both memchr results before combining them
with ||. Keep the existing cold_path() and
BuildEnvironmentBlockError::ContainsNul return behavior unchanged.
In `@crates/stdlib/src/multiprocessing.rs`:
- Around line 13-16: Remove the unused cold_path import from the core imports in
the Windows-only multiprocessing module. Keep the AtomicI32, AtomicU32, and
Ordering imports unchanged.
In `@crates/vm/src/stdlib/nt.rs`:
- Around line 555-558: Update the NUL checks in the affected code path to call
contains_nuls() on both key and value PyStrRef instances instead of
contains_null(), preserving the existing cold_path() and nul_char_error return
behavior.
---
Nitpick comments:
In `@crates/stdlib/src/multiprocessing.rs`:
- Around line 805-822: Add cold_path() immediately before returning
exceptions::nul_char_error(vm) in both the SemError::InteriorNul branch of
py_new and the corresponding branch in sem_unlink. Keep the existing non-NUL
error handling unchanged and match the cold-path convention used by the other
touched NUL-error sites.
🪄 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: 9548f16e-55a1-4f9c-b3d5-72ded4122018
📒 Files selected for processing (19)
crates/host_env/src/multiprocessing.rscrates/host_env/src/time.rscrates/host_env/src/winapi.rscrates/stdlib/src/grp.rscrates/stdlib/src/mmap.rscrates/stdlib/src/multiprocessing.rscrates/stdlib/src/openssl.rscrates/stdlib/src/ssl.rscrates/vm/src/builtins/bytes.rscrates/vm/src/builtins/str.rscrates/vm/src/bytes_inner.rscrates/vm/src/function/fspath.rscrates/vm/src/ospath.rscrates/vm/src/stdlib/_codecs.rscrates/vm/src/stdlib/_io.rscrates/vm/src/stdlib/nt.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/pwd.rscrates/vm/src/utils.rs
💤 Files with no reviewable changes (1)
- crates/vm/src/utils.rs
| use core::{ | ||
| hint::cold_path, | ||
| sync::atomic::{AtomicI32, AtomicU32, Ordering}, | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unused cold_path import in this Windows-only module.
cold_path is imported here but never called anywhere in this mod _multiprocessing (the Windows implementation has no NUL-check branch that uses it). This will trigger an unused_imports warning under cargo clippy.
As per coding guidelines, "Always run clippy to lint code with cargo clippy... 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/stdlib/src/multiprocessing.rs` around lines 13 - 16, Remove the unused
cold_path import from the core imports in the Windows-only multiprocessing
module. Keep the AtomicI32, AtomicU32, and Ordering imports unchanged.
Source: Coding guidelines
70a29fa to
6b4f1d6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/vm/src/function/fspath.rs (1)
43-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared NUL-error path.
The two arms differ only in the source value and constructed variant; derive the NUL flag and
FsPathin the match, then perform the check once.Proposed refactor
- s @ PyStr => { - if check_for_nul && s.contains_nuls() { - cold_path(); - return Err(crate::exceptions::nul_char_error(vm)); - } - Self::Str(s) - } - b @ PyBytes => { - if check_for_nul && b.contains_nuls() { - cold_path(); - return Err(crate::exceptions::nul_char_error(vm)); - } - Self::Bytes(b) - } + s @ PyStr => (s.contains_nuls(), Self::Str(s)), + b @ PyBytes => (b.contains_nuls(), Self::Bytes(b)),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/vm/src/function/fspath.rs` around lines 43 - 53, Refactor the match arms for PyStr and PyBytes to first derive the shared NUL-check flag and corresponding FsPath value, then perform the check_for_nul/contains_nuls validation once before returning the constructed value. Preserve the existing cold_path and nul_char_error behavior, while retaining the distinct Str and Bytes variants.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.
Nitpick comments:
In `@crates/vm/src/function/fspath.rs`:
- Around line 43-53: Refactor the match arms for PyStr and PyBytes to first
derive the shared NUL-check flag and corresponding FsPath value, then perform
the check_for_nul/contains_nuls validation once before returning the constructed
value. Preserve the existing cold_path and nul_char_error behavior, while
retaining the distinct Str and Bytes variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 53b83214-f265-47d2-87f7-1e1197f2ec0a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
crates/host_env/Cargo.tomlcrates/host_env/src/multiprocessing.rscrates/host_env/src/time.rscrates/host_env/src/winapi.rscrates/stdlib/src/grp.rscrates/stdlib/src/mmap.rscrates/stdlib/src/multiprocessing.rscrates/stdlib/src/openssl.rscrates/stdlib/src/ssl.rscrates/vm/src/builtins/bytes.rscrates/vm/src/builtins/str.rscrates/vm/src/bytes_inner.rscrates/vm/src/function/fspath.rscrates/vm/src/ospath.rscrates/vm/src/stdlib/_codecs.rscrates/vm/src/stdlib/_io.rscrates/vm/src/stdlib/nt.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/pwd.rscrates/vm/src/utils.rs
💤 Files with no reviewable changes (1)
- crates/vm/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (14)
- crates/vm/src/ospath.rs
- crates/host_env/Cargo.toml
- crates/host_env/src/winapi.rs
- crates/vm/src/stdlib/_codecs.rs
- crates/vm/src/builtins/bytes.rs
- crates/vm/src/stdlib/nt.rs
- crates/stdlib/src/grp.rs
- crates/stdlib/src/multiprocessing.rs
- crates/vm/src/bytes_inner.rs
- crates/stdlib/src/ssl.rs
- crates/host_env/src/time.rs
- crates/stdlib/src/openssl.rs
- crates/vm/src/stdlib/_io.rs
- crates/host_env/src/multiprocessing.rs
3cb02f0 to
93d0a9c
Compare
`memchr` is used throughout RustPython for searching through bytes expediently. However, for NUL checks, it's scantily used. Instead, our NUL checks either use `contains` or `memchr`. I switched all of the `contains(b'\0')` I could find to using memchr instead. I marked the failure paths as cold to hint to LLVM that interior NULs are truly exceptional. This should help branch prediction a bit which is nice for string/bytes functions since they are likely called a lot.
93d0a9c to
9dacb3e
Compare
1 participant
memchris used throughout RustPython for searching through bytes expediently. However, for NUL checks, it's scantily used. Instead, our NUL checks either usecontainsormemchr.I switched all of the
contains(b'\0')I could find to using memchr instead. I marked the failure paths as cold to hint to LLVM that interior NULs are truly exceptional. This should help branch prediction a bit which is nice for string/bytes functions since they are likely called a lot.Summary
memchrmore as it's specialized for searching through bytes.Summary by CodeRabbit