X Tutup
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Lib/test/test_mmap.py
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,6 @@ def test_non_ascii_byte(self):
self.assertEqual(m.read_byte(), b)
m.close()

@unittest.expectedFailure # TODO: RUSTPYTHON
@unittest.skipUnless(os.name == 'nt', 'requires Windows')
def test_tagname(self):
data1 = b"0123456789"
Expand Down
1 change: 1 addition & 0 deletions crates/stdlib/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ features = [
"Win32_System_Environment",
"Win32_System_Console",
"Win32_System_IO",
"Win32_System_Memory",
"Win32_System_Threading"
]

Expand Down
171 changes: 162 additions & 9 deletions crates/stdlib/src/mmap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ mod mmap {
CloseHandle, DUPLICATE_SAME_ACCESS, DuplicateHandle, HANDLE, INVALID_HANDLE_VALUE,
},
Storage::FileSystem::{FILE_BEGIN, GetFileSize, SetEndOfFile, SetFilePointerEx},
System::Memory::{
CreateFileMappingW, FILE_MAP_COPY, FILE_MAP_READ, FILE_MAP_WRITE, FlushViewOfFile,
MapViewOfFile, PAGE_READONLY, PAGE_READWRITE, PAGE_WRITECOPY, UnmapViewOfFile,
},
System::Threading::GetCurrentProcess,
};

Expand Down Expand Up @@ -193,17 +197,68 @@ mod mmap {
vm.ctx.exceptions.os_error.to_owned()
}

/// Named file mapping on Windows using raw Win32 APIs.
/// Supports tagname parameter for inter-process shared memory.
#[cfg(windows)]
struct NamedMmap {
map_handle: HANDLE,
view_ptr: *mut u8,
len: usize,
}

#[cfg(windows)]
// SAFETY: The memory mapping is managed by the OS and is safe to share
// across threads. Access is synchronized by PyMutex in PyMmap.
unsafe impl Send for NamedMmap {}
#[cfg(windows)]
unsafe impl Sync for NamedMmap {}

#[cfg(windows)]
impl core::fmt::Debug for NamedMmap {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
f.debug_struct("NamedMmap")
.field("map_handle", &self.map_handle)
.field("view_ptr", &self.view_ptr)
.field("len", &self.len)
.finish()
}
}

#[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);
}
}
}
}
Comment on lines +227 to +243
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 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 -10

Repository: 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)
EOF

Repository: 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 -10

Repository: 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.

Suggested change
#[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.


#[derive(Debug)]
enum MmapObj {
Write(MmapMut),
Read(Mmap),
#[cfg(windows)]
Named(NamedMmap),
}

impl MmapObj {
fn as_slice(&self) -> &[u8] {
match self {
MmapObj::Read(mmap) => &mmap[..],
MmapObj::Write(mmap) => &mmap[..],
#[cfg(windows)]
MmapObj::Named(named) => unsafe {
core::slice::from_raw_parts(named.view_ptr, named.len)
},
}
}
}
Expand Down Expand Up @@ -276,7 +331,6 @@ mod mmap {
#[pyarg(any)]
length: isize,
#[pyarg(any, default)]
#[allow(dead_code)]
tagname: Option<PyObjectRef>,
#[pyarg(any, default = AccessMode::Default)]
access: AccessMode,
Expand Down Expand Up @@ -494,11 +548,22 @@ mod mmap {
let mut map_size = args.validate_new_args(vm)?;
let MmapNewArgs {
fileno,
tagname,
access,
offset,
..
} = args;

// Parse tagname: None or a string
let tag_str: Option<String> = match tagname {
Some(ref obj) if !vm.is_none(obj) => {
Some(obj.try_to_value::<String>(vm).map_err(|_| {
vm.new_type_error("tagname must be a string or None".to_owned())
})?)
}
_ => None,
};

// Get file handle from fileno
// fileno -1 or 0 means anonymous mapping
let fh: Option<HANDLE> = if fileno != -1 && fileno != 0 {
Expand Down Expand Up @@ -595,6 +660,75 @@ mod mmap {
}
}

// When tagname is provided, use raw Win32 APIs for named shared memory
if let Some(ref tag) = tag_str {
let (fl_protect, desired_access) = match access {
AccessMode::Default | AccessMode::Write => (PAGE_READWRITE, FILE_MAP_WRITE),
AccessMode::Read => (PAGE_READONLY, FILE_MAP_READ),
AccessMode::Copy => (PAGE_WRITECOPY, FILE_MAP_COPY),
};

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
};
Comment on lines +671 to +680
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.


let tag_wide: Vec<u16> = tag.encode_utf16().chain(core::iter::once(0)).collect();

let total_size = (offset as u64)
.checked_add(map_size as u64)
.ok_or_else(|| vm.new_overflow_error("mmap offset plus size would overflow"))?;
let size_hi = (total_size >> 32) as u32;
let size_lo = total_size as u32;

let map_handle = unsafe {
CreateFileMappingW(
fh,
core::ptr::null(),
fl_protect,
size_hi,
size_lo,
tag_wide.as_ptr(),
)
};
if map_handle.is_null() {
return Err(io::Error::last_os_error().to_pyexception(vm));
}

let off_hi = (offset as u64 >> 32) as u32;
let off_lo = offset as u32;

let view =
unsafe { MapViewOfFile(map_handle, desired_access, off_hi, off_lo, map_size) };
if view.Value.is_null() {
unsafe { CloseHandle(map_handle) };
return Err(io::Error::last_os_error().to_pyexception(vm));
}

let named = NamedMmap {
map_handle,
view_ptr: view.Value as *mut u8,
len: map_size,
};

return Ok(Self {
closed: AtomicCell::new(false),
mmap: PyMutex::new(Some(MmapObj::Named(named))),
handle: AtomicCell::new(INVALID_HANDLE_VALUE as isize),
offset,
size: AtomicCell::new(map_size),
pos: AtomicCell::new(0),
exports: AtomicCell::new(0),
access,
});
}

let mut mmap_opt = MmapOptions::new();
let mmap_opt = mmap_opt.offset(offset as u64).len(map_size);

Expand Down Expand Up @@ -719,6 +853,10 @@ mod mmap {
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[..],
#[cfg(windows)]
MmapObj::Named(named) => unsafe {
core::slice::from_raw_parts_mut(named.view_ptr, named.len)
},
}
})
.into()
Expand Down Expand Up @@ -749,15 +887,19 @@ mod mmap {
fn try_writable<R>(
&self,
vm: &VirtualMachine,
f: impl FnOnce(&mut MmapMut) -> R,
f: impl FnOnce(&mut [u8]) -> R,
) -> PyResult<R> {
if matches!(self.access, AccessMode::Read) {
return Err(vm.new_type_error("mmap can't modify a readonly memory map."));
}

match self.check_valid(vm)?.deref_mut().as_mut().unwrap() {
MmapObj::Write(mmap) => Ok(f(mmap)),
_ => unreachable!("already check"),
MmapObj::Write(mmap) => Ok(f(&mut mmap[..])),
#[cfg(windows)]
MmapObj::Named(named) => Ok(f(unsafe {
core::slice::from_raw_parts_mut(named.view_ptr, named.len)
})),
_ => unreachable!("already checked"),
}
}

Expand Down Expand Up @@ -873,6 +1015,14 @@ mod mmap {
mmap.flush_range(offset, size)
.map_err(|e| e.to_pyexception(vm))?;
}
#[cfg(windows)]
MmapObj::Named(named) => {
let ptr = unsafe { named.view_ptr.add(offset) };
let result = unsafe { FlushViewOfFile(ptr as *const _, size) };
if result == 0 {
return Err(io::Error::last_os_error().to_pyexception(vm));
}
}
}

Ok(())
Expand Down Expand Up @@ -1044,11 +1194,17 @@ mod mmap {
}

let handle = self.handle.load();
let is_anonymous = handle == INVALID_HANDLE_VALUE as isize;

// Get the lock on mmap
let mut mmap_guard = self.mmap.lock();

// Check if this is a Named mmap - these cannot be resized
if let Some(MmapObj::Named(_)) = mmap_guard.as_ref() {
return Err(vm.new_system_error("mmap: cannot resize a named memory mapping"));
}

let is_anonymous = handle == INVALID_HANDLE_VALUE as isize;

if is_anonymous {
// For anonymous mmap, we need to:
// 1. Create a new anonymous mmap with the new size
Expand All @@ -1067,10 +1223,7 @@ mod mmap {

// Copy data from old mmap to new mmap
if let Some(old_mmap) = mmap_guard.as_ref() {
let src = match old_mmap {
MmapObj::Write(m) => &m[..copy_size],
MmapObj::Read(m) => &m[..copy_size],
};
let src = &old_mmap.as_slice()[..copy_size];
new_mmap[..copy_size].copy_from_slice(src);
}

Expand Down
Loading
X Tutup