Skip to content

Align mmap read and search arguments - #9590

Merged
youknowone merged 4 commits into
RustPython:mainfrom
youknowdot:fix/mmap-read-index-errors-20261103
Nov 6, 2026
Merged

youknowone merged 4 commits into
RustPython:mainfrom
youknowdot:fix/mmap-read-index-errors-20261103

Conversation

@youknowdot

@youknowdot youknowdot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Match CPython's optional signed-size conversion for mmap.read: reject non-indexable values while propagating errors from __index__ and integer overflow. Keep conversion outside the mapping lock and preserve the existing safe type-name truncation.
  • For find and rfind, bind arguments before acquiring the search buffer, require the buffer protocol, and convert start followed by end only when start is not None, as CPython 3.15 does. Check mapping validity around conversions that can run Python, and retain the original buffer export during callbacks.
  • Search from an unlocked needle snapshot before taking the destination mapping lock, so a mapping or its view can safely be used as its own search needle. Immutable bytes avoid the previous iterable-to-vector copy. The initial position and size are retained across index callbacks and bounded against the current mapping size.

Validation

  • Independently source-reviewed against CPython 3.15's read Clinic conversion and mmap_gfind_lock_held, including buffer lifetime, self-aliasing, reentrant close/resize, range bounds, and lock release.
  • prek run --files crates/stdlib/src/mmap.rs, normal commit hooks, and git diff --check passed.
  • On the prior read-only head, macOS and Windows hosted CPython-test jobs passed; Linux remained queued. No local RustPython build or runtime tests were run. New-head hosted CI is needed. test_mmap.test_read_invalid_arg and existing find/rfind cases cover related behavior; the existing test_find_end remains skipped and therefore is not execution evidence.

Scope and costs

The search path adds a validation lock, and non-bytes search buffers are snapshotted, including before a reversed-range return. This can change concurrent mutation visibility or allocation-failure timing; no benchmark was run and full free-threaded critical-section parity is not claimed. Existing signed-size overflow wording may differ, as can a diagnostic when a multibyte character crosses CPython's raw 200-byte type-name limit. No Lib or test files were modified.

Distinguish a missing index slot from exceptions raised while converting an index, including overflow and interrupts.

Assisted-by: OpenAI Codex:model version not exposed
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

Caution

Review failed

The pull request is closed.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Assisted-by: OpenAI Codex:model version not exposed
@youknowdot youknowdot changed the title Preserve mmap read index conversion errors Align mmap read and search arguments Nov 3, 2026
Comment thread crates/stdlib/src/mmap.rs
Comment on lines +1124 to +1135
Some(obj) if !vm.is_none(&obj) => {
let index = obj.try_index_opt(vm).ok_or_else(|| {
let mut name = obj.class().slot_name().to_string();
if name.len() > 200 {
let mut end = 200;
while !name.is_char_boundary(end) {
end -= 1;
}
name.truncate(end);
}
vm.new_type_error(format!(
"read argument must be int or None, not {}",
class.name()
"argument should be integer or None, not '{name}'"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't repeat yourself

Apply maintainer review while preserving conversion and audit behavior.

Assisted-by: OpenAI Codex:model version unavailable
Use map_or_else for the pure default bound calculation to satisfy strict Clippy.

Assisted-by: OpenAI Codex:model version unavailable
@youknowone
youknowone marked this pull request as ready for review November 6, 2026 03:35
@youknowone
youknowone merged commit c35ff3a into RustPython:main Nov 6, 2026
28 checks passed
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-11-06T03:35:14.043027Z
We couldn’t complete this review. Comment @codex review to try again.
37e4352 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants