Use memchr consistently for NUL checks#8372
Open
joshuamegnauth54 wants to merge 1 commit into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 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 |
joshuamegnauth54
force-pushed
the
use-memchr-consistently
branch
from
July 25, 2026 19:48
8549281 to
6788a1e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Comment on lines
+13
to
+16
| use core::{ | ||
| hint::cold_path, | ||
| sync::atomic::{AtomicI32, AtomicU32, Ordering}, | ||
| }; |
Contributor
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
Comment on lines
555
to
558
| if key.contains_null() || value.contains_null() { | ||
| cold_path(); | ||
| return Err(exceptions::nul_char_error(vm)); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual method name(s) defined for NUL detection on PyStr/PyStrRef
ast-grep run --pattern 'fn contains_nul$_($$$) -> $_' --lang rust crates/vm/src/builtins/str.rs crates/vm/src/bytes_inner.rs crates/vm/src/builtins/bytes.rs
rg -n 'fn contains_nul' crates/vm/src/builtins/str.rs crates/vm/src/bytes_inner.rs crates/vm/src/builtins/bytes.rs
rg -n 'contains_null\(\)|contains_nuls\(\)' crates/vm/src -g '!target'Repository: RustPython/RustPython
Length of output: 1692
Call contains_nuls() for NUL checks on PyStrRef.
PyStr/PyStrRef only define contains_nuls(), so key.contains_null() / value.contains_null() will not compile.
🤖 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/nt.rs` around lines 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.
`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.
joshuamegnauth54
force-pushed
the
use-memchr-consistently
branch
from
July 26, 2026 04:13
6788a1e to
70a29fa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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