Implement Windows named mmap via CreateFileMappingW#7207
Implement Windows named mmap via CreateFileMappingW#7207youknowone merged 1 commit intoRustPython:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR adds Windows named memory-mapping support: it exposes the Win32 System Memory feature in Cargo.toml and implements a Windows-only NamedMmap type plus associated code paths to create, map, access, flush, and drop named file mappings. Changes
Sequence Diagram(s)sequenceDiagram
participant Python as "Python user"
participant PyMmap as "PyMmap"
participant Validator as "Tagname validator"
participant Win32 as "Win32 APIs"
participant Kernel as "Kernel memory"
Python->>PyMmap: py_new(..., tagname="name")
PyMmap->>Validator: validate/convert tagname
Validator-->>PyMmap: validated tagname
PyMmap->>Win32: CreateFileMappingW(..., name)
Win32->>Kernel: allocate/identify named mapping
Kernel-->>Win32: map_handle
Win32-->>PyMmap: map_handle
PyMmap->>Win32: MapViewOfFile(map_handle, access)
Win32->>Kernel: map view into process
Kernel-->>Win32: view_ptr
Win32-->>PyMmap: view_ptr
PyMmap->>PyMmap: store NamedMmap(map_handle, view_ptr, len)
PyMmap-->>Python: return MmapObj::Named
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin win-mmap |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_mmap.py (TODO: 2) dependencies: dependent tests: (no tests depend on mmap) Legend:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/stdlib/src/mmap.rs (1)
1190-1219:⚠️ Potential issue | 🔴 Critical
resizesilently converts aNamedmmap into an anonymous one.For
Namedmmaps,self.handleisINVALID_HANDLE_VALUE, sois_anonymousistrue. The anonymous resize path creates a newMmapMutviamap_anon()and replaces theNamedvariant withMmapObj::Write. This silently destroys the named shared memory mapping — any other process sharing the tagname will no longer see updates, and the mapping loses its named identity.At minimum,
Namedmmaps should be detected and an error raised (similar to how the Unix path returns an error for missingmremap). Better yet, the named mapping should be re-created withCreateFileMappingW+MapViewOfFileat the new size.Proposed minimal fix: reject resize for Named mmaps
// Get the lock on mmap let mut mmap_guard = self.mmap.lock(); + // Named mmaps cannot be resized via the anonymous path + if let Some(MmapObj::Named(_)) = mmap_guard.as_ref() { + return Err(vm.new_system_error( + "mmap: resizing named memory mappings is not supported", + )); + } + if is_anonymous {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/mmap.rs` around lines 1190 - 1219, The resize implementation silently converts Named mappings into anonymous ones by checking is_anonymous via self.handle and then unconditionally calling map_anon() and assigning MmapObj::Write; instead detect Named mappings in resize (use self.handle, MmapObj variants and mmap.lock()/mmap_guard) and return an error when attempting to resize a Named mmap (mirror the Unix behavior for missing mremap) OR implement re-creation via Windows APIs (CreateFileMappingW + MapViewOfFile) to preserve the named mapping; update the code path that currently calls MmapOptions::map_anon() so it either raises a clear error for Named maps or uses the CreateFileMappingW/MapViewOfFile flow to create a new named mapping and copy data before replacing mmap_guard and updating self.size.
🧹 Nitpick comments (1)
crates/stdlib/src/mmap.rs (1)
849-861:as_bytes_mutreturns a mutable slice forNamedregardless of access mode.Unlike
MmapObj::Readwhich panics, theNamedarm always returns a mutable slice. While current callers (try_writable, buffer protocol) check access mode before reaching this point, adding a debug assertion would strengthen the invariant:#[cfg(windows)] - MmapObj::Named(named) => unsafe { - core::slice::from_raw_parts_mut(named.view_ptr, named.len) - }, + MmapObj::Named(named) => { + debug_assert!( + !matches!(self.access, AccessMode::Read), + "mmap can't modify a readonly memory map." + ); + unsafe { core::slice::from_raw_parts_mut(named.view_ptr, named.len) } + }Note:
selfis not directly accessible inas_bytes_mutsince it operates on theMmapObjmatch. If restructuring isn't feasible, adding a comment documenting the caller-enforced invariant would suffice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/mmap.rs` around lines 849 - 861, The as_bytes_mut method currently returns a mutable slice for MmapObj::Named regardless of access mode; update as_bytes_mut (the match arm for MmapObj::Named) to either add a debug assertion that the named mapping is writable (assert the Named object's access/flags indicate writable access) or, if that field isn't accessible here, add a clear comment above the method and the MmapObj::Named arm documenting the caller-enforced invariant (try_writable and buffer protocol callers must have verified writability) so future readers/maintainers know why a mutable slice is returned despite no runtime check; reference as_bytes_mut, MmapObj::Named, MmapObj::Read, try_writable and the buffer protocol in the comment or assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/stdlib/src/mmap.rs`:
- Around line 1016-1019: The comment is wrong: MmapObj::Named can be file-backed
and thus may need flushing; update the flush handling for the Windows Named
variant in the flush implementation to either detect whether the Named mapping
was created from a real file handle (file-backed) and call FlushViewOfFile only
for those, or simply call FlushViewOfFile unconditionally for MmapObj::Named.
Locate the Windows branch handling MmapObj::Named in the flush function and
replace the no-op with a call to the appropriate Windows flush routine
(FlushViewOfFile) or conditional logic based on how the Named mapping was
constructed.
- Around line 671-680: The current logic closes duplicated_handle and leaves
self.handle as INVALID_HANDLE_VALUE for file-backed named mappings (code around
duplicated_handle, fh and CreateFileMappingW), which changes behavior of size()
and resize(); either preserve the duplicated handle in self.handle or document
the intentional divergence. Fix by not closing duplicated_handle when fh is Some
and tagname is used: keep duplicated_handle open and assign it to self.handle
after successful CreateFileMappingW (ensure you only close duplicated_handle on
error or when the mmap is dropped), update the cleanup paths to avoid
double-closing, and add a short comment near the
duplicated_handle/fh/CreateFileMappingW block explaining why the duplicated OS
file handle is retained in self.handle so size() and resize() continue to
operate on the file-backed mapping.
- Around line 663-728: The code computes total_size = (offset as u64) +
(map_size as u64) without overflow checks in the tagged-path; change this to use
checked_add like the non-tagged path (use offset as u64 .checked_add(map_size as
u64)) and on None return the same kind of error (e.g., return
Err(io::Error::new(...).to_pyexception(vm))) before calling CreateFileMappingW
so CreateFileMappingW and subsequent logic (CreateFileMappingW, MapViewOfFile,
NamedMmap construction) only run with a validated, non-overflowing size.
---
Outside diff comments:
In `@crates/stdlib/src/mmap.rs`:
- Around line 1190-1219: The resize implementation silently converts Named
mappings into anonymous ones by checking is_anonymous via self.handle and then
unconditionally calling map_anon() and assigning MmapObj::Write; instead detect
Named mappings in resize (use self.handle, MmapObj variants and
mmap.lock()/mmap_guard) and return an error when attempting to resize a Named
mmap (mirror the Unix behavior for missing mremap) OR implement re-creation via
Windows APIs (CreateFileMappingW + MapViewOfFile) to preserve the named mapping;
update the code path that currently calls MmapOptions::map_anon() so it either
raises a clear error for Named maps or uses the CreateFileMappingW/MapViewOfFile
flow to create a new named mapping and copy data before replacing mmap_guard and
updating self.size.
---
Nitpick comments:
In `@crates/stdlib/src/mmap.rs`:
- Around line 849-861: The as_bytes_mut method currently returns a mutable slice
for MmapObj::Named regardless of access mode; update as_bytes_mut (the match arm
for MmapObj::Named) to either add a debug assertion that the named mapping is
writable (assert the Named object's access/flags indicate writable access) or,
if that field isn't accessible here, add a clear comment above the method and
the MmapObj::Named arm documenting the caller-enforced invariant (try_writable
and buffer protocol callers must have verified writability) so future
readers/maintainers know why a mutable slice is returned despite no runtime
check; reference as_bytes_mut, MmapObj::Named, MmapObj::Read, try_writable and
the buffer protocol in the comment or assertion.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_mmap.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/stdlib/Cargo.tomlcrates/stdlib/src/mmap.rs
| let fh = if let Some(fh) = fh { | ||
| // Close the duplicated handle - we'll use the original | ||
| // file handle for CreateFileMappingW | ||
| if duplicated_handle != INVALID_HANDLE_VALUE { | ||
| unsafe { CloseHandle(duplicated_handle) }; | ||
| } | ||
| fh | ||
| } else { | ||
| INVALID_HANDLE_VALUE | ||
| }; |
There was a problem hiding this comment.
Closing the duplicated handle when using tagname with a file-backed mapping — verify this is intentional.
When a file handle is provided alongside a tagname, the duplicated handle is closed and the original fh (from get_osfhandle) is passed to CreateFileMappingW. After CreateFileMappingW succeeds, the mapping object is independent of the file handle, so this is technically correct. However, this diverges from the non-tagged path (which keeps the duplicated handle) and means self.handle is INVALID_HANDLE_VALUE for file-backed named mmaps. This affects size() (returns map size instead of file size) and resize() (treats it as anonymous).
Consider storing the duplicated handle in self.handle for file-backed named mmaps, or at minimum add a comment explaining why this is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/stdlib/src/mmap.rs` around lines 671 - 680, The current logic closes
duplicated_handle and leaves self.handle as INVALID_HANDLE_VALUE for file-backed
named mappings (code around duplicated_handle, fh and CreateFileMappingW), which
changes behavior of size() and resize(); either preserve the duplicated handle
in self.handle or document the intentional divergence. Fix by not closing
duplicated_handle when fh is Some and tagname is used: keep duplicated_handle
open and assign it to self.handle after successful CreateFileMappingW (ensure
you only close duplicated_handle on error or when the mmap is dropped), update
the cleanup paths to avoid double-closing, and add a short comment near the
duplicated_handle/fh/CreateFileMappingW block explaining why the duplicated OS
file handle is retained in self.handle so size() and resize() continue to
operate on the file-backed mapping.
Use Win32 CreateFileMappingW + MapViewOfFile when tagname is provided, enabling inter-process shared memory for multiprocessing.heap.Arena. Anonymous mmaps without tagname still use memmap2. Remove expectedFailure from test_tagname in test_mmap.py.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/stdlib/src/mmap.rs (1)
671-680:⚠️ Potential issue | 🟠 MajorFile-backed named mappings lose the OS file handle —
size()won't return the file size.When a tagname is provided alongside a real file handle, the duplicated handle is closed and
self.handleis set toINVALID_HANDLE_VALUE. This causessize()(line 1331) to returnself.__len__()(the mapping length) instead of querying the actual file size viaGetFileSize. CPython'smmap.size()returns the file size for file-backed mappings.Consider storing
duplicated_handleinself.handlefor file-backed named mmaps instead of closing it, or add a comment explaining the intentional divergence from CPython behavior.Also applies to: 720-729
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/mmap.rs` around lines 671 - 680, The code closes duplicated_handle and leaves self.handle as INVALID_HANDLE_VALUE for file-backed named mappings, causing size() to fall back to the mapping length instead of using GetFileSize; change the logic in the block that currently returns fh/INVALID_HANDLE_VALUE (the code handling fh and duplicated_handle) so that for file-backed named mmaps you keep the duplicated_handle (assign duplicated_handle to self.handle) instead of closing it, and only close the original duplicated_handle when it is truly unused; ensure the new ownership is consistent with the destructor/cleanup so no handle is leaked and size() can call GetFileSize on self.handle.
🧹 Nitpick comments (1)
crates/stdlib/src/mmap.rs (1)
851-863:as_bytes_mutdoesn't guard against Read-mode Named mappings.
MmapObj::Readpanics here (line 854), but aNamedmmap created withAccessMode::Read(FILE_MAP_READ) will silently return a mutable slice. Writing to that memory would cause an OS-level access violation. While callers currently check access mode upstream, adding a consistent guard here would be safer.🛡️ Suggested defensive guard
#[cfg(windows)] MmapObj::Named(named) => unsafe { + debug_assert!( + !matches!( + *m.as_ref().unwrap(), + MmapObj::Named(_) if false // access check is on PyMmap, not MmapObj + ) + ); core::slice::from_raw_parts_mut(named.view_ptr, named.len) },Actually, since
as_bytes_mutdoesn't have access toself.access, a simpler approach: document that callers must check access mode, mirroring theMmapObj::Readpanic's implicit contract.Alternatively, pass
accessinto the function or move the check to the match:fn as_bytes_mut(&self) -> BorrowedValueMut<'_, [u8]> { + assert!( + self.access != AccessMode::Read, + "mmap can't modify a readonly memory map." + ); PyMutexGuard::map(self.mmap.lock(), |m| { match m.as_mut().expect("mmap closed or invalid") { MmapObj::Read(_) => panic!("mmap can't modify a readonly memory map."), MmapObj::Write(mmap) => &mut mmap[..],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/mmap.rs` around lines 851 - 863, as_bytes_mut currently returns a mutable slice for MmapObj::Named even if that named mapping was opened read-only, which can lead to an access violation; update as_bytes_mut (or its caller) to check the mapping's access mode and refuse/mirror the MmapObj::Read behavior for read-only Named maps: either accept and use an access parameter in as_bytes_mut and panic when access == AccessMode::Read for MmapObj::Named, or add a guard in the match arm for MmapObj::Named that inspects the named mapping's access flag and panics if it's read-only, ensuring Named maps opened with AccessMode::Read are not exposed as mutable slices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/stdlib/src/mmap.rs`:
- Around line 227-243: The Drop impl for NamedMmap uses .is_null() on map_handle
which is an isize (windows-sys HANDLE) and doesn't have that method; change the
checks that call self.map_handle.is_null() to integer comparisons (e.g.
self.map_handle != 0 or self.map_handle == 0) while leaving pointer checks like
self.view_ptr.is_null() as-is; update the Drop impl (and any other places using
map_handle.is_null(), e.g. the other occurrence noted) to use map_handle != 0
before calling CloseHandle.
---
Duplicate comments:
In `@crates/stdlib/src/mmap.rs`:
- Around line 671-680: The code closes duplicated_handle and leaves self.handle
as INVALID_HANDLE_VALUE for file-backed named mappings, causing size() to fall
back to the mapping length instead of using GetFileSize; change the logic in the
block that currently returns fh/INVALID_HANDLE_VALUE (the code handling fh and
duplicated_handle) so that for file-backed named mmaps you keep the
duplicated_handle (assign duplicated_handle to self.handle) instead of closing
it, and only close the original duplicated_handle when it is truly unused;
ensure the new ownership is consistent with the destructor/cleanup so no handle
is leaked and size() can call GetFileSize on self.handle.
---
Nitpick comments:
In `@crates/stdlib/src/mmap.rs`:
- Around line 851-863: as_bytes_mut currently returns a mutable slice for
MmapObj::Named even if that named mapping was opened read-only, which can lead
to an access violation; update as_bytes_mut (or its caller) to check the
mapping's access mode and refuse/mirror the MmapObj::Read behavior for read-only
Named maps: either accept and use an access parameter in as_bytes_mut and panic
when access == AccessMode::Read for MmapObj::Named, or add a guard in the match
arm for MmapObj::Named that inspects the named mapping's access flag and panics
if it's read-only, ensuring Named maps opened with AccessMode::Read are not
exposed as mutable slices.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_mmap.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/stdlib/Cargo.tomlcrates/stdlib/src/mmap.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/stdlib/Cargo.toml
| #[cfg(windows)] | ||
| impl Drop for NamedMmap { | ||
| fn drop(&mut self) { | ||
| unsafe { | ||
| if !self.view_ptr.is_null() { | ||
| UnmapViewOfFile( | ||
| windows_sys::Win32::System::Memory::MEMORY_MAPPED_VIEW_ADDRESS { | ||
| Value: self.view_ptr as *mut _, | ||
| }, | ||
| ); | ||
| } | ||
| if !self.map_handle.is_null() { | ||
| CloseHandle(self.map_handle); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that HANDLE is indeed `isize` in the windows_sys version used by this repo
echo "=== Checking windows_sys HANDLE definition ==="
rg -n 'type HANDLE' --type rust -g '!target/**' 2>/dev/null | head -20
echo ""
echo "=== Checking all .is_null() calls on HANDLE-typed variables in mmap.rs ==="
rg -n '\.is_null\(\)' crates/stdlib/src/mmap.rs
echo ""
echo "=== Checking the exact context of line 238 and 700 in mmap.rs ==="
sed -n '235,245p' crates/stdlib/src/mmap.rs
echo "---"
sed -n '698,708p' crates/stdlib/src/mmap.rs
echo ""
echo "=== Checking windows_sys version in Cargo.lock ==="
rg -A2 'name = "windows-sys"' Cargo.lock | head -10Repository: RustPython/RustPython
Length of output: 1478
🏁 Script executed:
#!/bin/bash
# Find HANDLE imports and usage in mmap.rs
echo "=== Finding HANDLE imports in mmap.rs ==="
head -100 crates/stdlib/src/mmap.rs | grep -E 'use.*HANDLE|use.*windows'
echo ""
echo "=== Checking struct definitions with HANDLE in mmap.rs ==="
rg -n 'HANDLE' crates/stdlib/src/mmap.rs | head -30
echo ""
echo "=== Checking the actual HANDLE type in windows-sys ==="
# Try to find what HANDLE resolves to by checking the windows-sys documentation or source
# windows_sys defines HANDLE as isize in most versions, but let's verify
python3 << 'EOF'
# Check windows-sys version info
import subprocess
result = subprocess.run(['rg', 'windows-sys', 'Cargo.toml'], capture_output=True, text=True)
print("=== windows-sys in Cargo.toml ===")
print(result.stdout)
EOFRepository: RustPython/RustPython
Length of output: 2360
🏁 Script executed:
#!/bin/bash
# Verify the HANDLE type definition in windows-sys 0.61.2
echo "=== Checking if code compiles (attempting cargo check on mmap.rs) ==="
cd crates/stdlib && cargo check --message-format=short 2>&1 | grep -A3 -B1 'mmap\|is_null' | head -40
echo ""
echo "=== Confirming HANDLE comparison patterns in file ==="
rg -n 'INVALID_HANDLE_VALUE as isize' crates/stdlib/src/mmap.rs | head -10Repository: RustPython/RustPython
Length of output: 151
HANDLE is isize in windows-sys — .is_null() method does not exist on integer types and will fail to compile.
In windows-sys, HANDLE is defined as isize. The .is_null() method is only available on raw pointer types (*const T, *mut T), not on integer types. Lines 238 and 700 (map_handle.is_null() and if !self.map_handle.is_null()) will both produce error[E0599]: no method named is_null found for type isize.
The correct pattern is used elsewhere in the file (e.g., lines 296, 577, 674): compare directly using != or == operators.
Proposed fix
Drop impl at line 238:
- if !self.map_handle.is_null() {
+ if self.map_handle != 0 {And at line 700:
- if map_handle.is_null() {
+ if map_handle != 0 {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(windows)] | |
| impl Drop for NamedMmap { | |
| fn drop(&mut self) { | |
| unsafe { | |
| if !self.view_ptr.is_null() { | |
| UnmapViewOfFile( | |
| windows_sys::Win32::System::Memory::MEMORY_MAPPED_VIEW_ADDRESS { | |
| Value: self.view_ptr as *mut _, | |
| }, | |
| ); | |
| } | |
| if !self.map_handle.is_null() { | |
| CloseHandle(self.map_handle); | |
| } | |
| } | |
| } | |
| } | |
| #[cfg(windows)] | |
| impl Drop for NamedMmap { | |
| fn drop(&mut self) { | |
| unsafe { | |
| if !self.view_ptr.is_null() { | |
| UnmapViewOfFile( | |
| windows_sys::Win32::System::Memory::MEMORY_MAPPED_VIEW_ADDRESS { | |
| Value: self.view_ptr as *mut _, | |
| }, | |
| ); | |
| } | |
| if self.map_handle != 0 { | |
| CloseHandle(self.map_handle); | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/stdlib/src/mmap.rs` around lines 227 - 243, The Drop impl for
NamedMmap uses .is_null() on map_handle which is an isize (windows-sys HANDLE)
and doesn't have that method; change the checks that call
self.map_handle.is_null() to integer comparisons (e.g. self.map_handle != 0 or
self.map_handle == 0) while leaving pointer checks like self.view_ptr.is_null()
as-is; update the Drop impl (and any other places using map_handle.is_null(),
e.g. the other occurrence noted) to use map_handle != 0 before calling
CloseHandle.
1 participant
Use Win32 CreateFileMappingW + MapViewOfFile when tagname is provided, enabling inter-process shared memory for multiprocessing.heap.Arena. Anonymous mmaps without tagname still use memmap2.
Remove expectedFailure from test_tagname in test_mmap.py.
Summary by CodeRabbit