Install the posix module as nt on Windows - #780
Conversation
WalkthroughThe PR makes pathname-backed writes eagerly create or truncate host files and adds Windows-specific interpreter support, including ChangesFilesystem open semantics
Windows compatibility surface
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PythonStdlib
participant BuiltinRegistry
participant PosixNt
participant WindowsAPI
PythonStdlib->>BuiltinRegistry: import nt or winreg
BuiltinRegistry->>PosixNt: register Windows nt helpers
PythonStdlib->>PosixNt: call path or handle helper
PosixNt->>WindowsAPI: execute Windows operation
WindowsAPI-->>PosixNt: result or error
PosixNt-->>PythonStdlib: Python value or OSError
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit bc710ff). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9086cf30f
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if cookie.is_null() { | ||
| return Err(last_error(&path)); | ||
| } | ||
| Ok(pyre_object::w_int_new(cookie as usize as i64)) |
There was a problem hiding this comment.
Return and validate the PyPy-equivalent W_DLLCapsule instead of exposing the DLL_DIRECTORY_COOKIE as an integer. As written, Python code can forge a value such as nt._remove_dll_directory(1), which is cast to a pointer and passed to RemoveDllDirectory; the Windows API only accepts cookies returned by AddDllDirectory, whereas PyPy rejects non-capsules with TypeError, so this can invoke the host API with an invalid opaque pointer.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| } else { | ||
| pyre_object::w_str_new(path) | ||
| }; | ||
| crate::PyError::os_error_syscall(errno, filename) |
There was a problem hiding this comment.
Preserve the raw GetLastError value and populate OSError.winerror when any of these Win32 calls fails. This helper translates the code to a POSIX errno and then calls a builder that only sets errno, so Windows recovery code that inspects exc.winerror gets AttributeError instead of the Win32 code; this notably conflicts with the checked-in ntpath error handling and with PyPy's lastSavedWindowsError/wrap_oserror2 path.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| win_nt::_getfullpathname as crate::gateway::BuiltinCodeFn, | ||
| 1u16, | ||
| ), | ||
| ("_getfinalpathname", win_nt::_getfinalpathname, 1), |
There was a problem hiding this comment.
On Windows, lib-python/3/ntpath.py enables native realpath through one grouped import of _findfirstfile, _getfinalpathname, and readlink; a repo-wide source search finds no _findfirstfile implementation or registration. Consequently that import always raises ImportError and ntpath.realpath is defined as abspath, so Windows symlinks and junctions are never resolved despite registering _getfinalpathname here.
Useful? React with 👍 / 👎.
| let as_bytes = unsafe { pyre_object::bytesobject::is_bytes_like(arg) }; | ||
| let path = crate::gateway::fsencode_w(arg)?; | ||
| Ok((path, as_bytes)) |
There was a problem hiding this comment.
Determine as_bytes from the result of os.fspath, not from the original wrapper object. For a custom PathLike whose __fspath__ returns bytes, this records false, so nt._getfullpathname(pathlike) returns str; PyPy explicitly resolves fspath before choosing the result type, and its checked-in test requires a bytes-returning path-like to produce bytes.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 11740-11754: Update the file-opening logic in the builtins
implementation so the handle returned by OpenOptions::open for the “x” and “a”
modes is retained by the returned file wrapper instead of discarded via map(|_|
()). Populate the wrapper’s fd-backed field, such as __file_fd__, or reuse the
existing fd-backed implementation, while preserving the requested create-new and
append semantics for later write and flush operations.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 386-405: Validate argument 0 as an integer before calling
pyre_object::w_int_get_value in _getfileinformation, get_handle_inheritable,
set_handle_inheritable, and _remove_dll_directory, returning the standard
TypeError for non-integer inputs via crate::baseobjspace::int_w or the module’s
established is_int pattern. In _getfileinformation, also reject
_get_osfhandle(fd) results of -1 or -2 before invoking
GetFileInformationByHandle and return EBADF for those invalid descriptors.
- Around line 365-376: Update both failure paths around
GetFinalPathNameByHandleW to capture the Windows last-error value before calling
CloseHandle, then close the handle and construct the error from the captured
value. Preserve the existing successful-path cleanup and use the same
error-preservation pattern as disk_free.
- Around line 318-342: Update _getfullpathname to reject paths containing an
embedded NUL character before calling to_wide, matching the validation in
_getfinalpathname and _getdiskusage. Return the same ValueError behavior and
message used by those sibling functions, while preserving the existing Win32
path-resolution flow for valid paths.
- Around line 438-442: Update the parent-path computation around the cut match
to retain the separator when the matched separator is the root boundary, so
drive-root paths such as C:\file.txt produce C:\ and rooted paths such as
\file.txt produce \. Preserve the existing parent extraction for non-root nested
paths and ensure the resulting path remains valid for GetDiskFreeSpaceExW.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 980eca4a-0500-476f-8b55-1b940d80ba9a
📒 Files selected for processing (7)
pyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_winapi/mod.rspyre/pyre-interpreter/src/module/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/winreg/mod.rs
| let create_res = if mode.contains('x') { | ||
| std::fs::OpenOptions::new() | ||
| .write(true) | ||
| .create_new(true) | ||
| .open(&path) | ||
| .map(|_| ()) | ||
| } else if mode.contains('a') { | ||
| std::fs::OpenOptions::new() | ||
| .append(true) | ||
| .create(true) | ||
| .open(&path) | ||
| .map(|_| ()) | ||
| } else { | ||
| std::fs::write(&path, b"") | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)builtins\.rs$|pyre-interpreter/src/builtins.rs|pyre/pyre-interpreter/src/builtins.rs' || true
echo "== target range =="
if [ -f pyre/pyre-interpreter/src/builtins.rs ]; then
sed -n '11700,11810p' pyre/pyre-interpreter/src/builtins.rs | cat -n -v
fi
echo "== related wrappers/usages =="
rg -n "struct .*File|FdFile|OwnedFile|FileBacking|FileBacked|FileLike|open\(|write\(|fs::write|create_new|create\(\)|append\(|flush\(|close\(" pyre/pyre-interpreter/src/builtins.rs | sed -n '1,220p'Repository: youknowone/pyre
Length of output: 10996
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file wrapper outline =="
ast-grep outline pyre/pyre-interpreter/src/builtins.rs --view expanded 2>/dev/null | rg -A 40 -B 5 "file_wrapper|FileWrapper|__file_data__|__file_pos__|flush|close|write" || true
echo "== file wrapper definitions/usages around 9700-10350 =="
sed -n '9730,10350p' pyre/pyre-interpreter/src/builtins.rs | cat -n -v
echo "== fileio write/flush/close definitions =="
sed -n '11000,11215p' pyre/pyre-interpreter/src/builtins.rs | cat -n -v
echo "== open path fd-backed definitions =="
sed -n '11620,11695p' pyre/pyre-interpreter/src/builtins.rs | cat -n -vRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant tests =="
sed -n '12605,12625p' pyre/pyre-interpreter/src/builtins.rs | cat -n -v
echo "== parse and inspect file-wrapper state storage for writing branch =="
python3 - <<'PY'
from pathlib import Path
p=Path('pyre/pyre-interpreter/src/builtins.rs')
s=p.read_text()
start=s.find('builtin_open(args: &[PyObjectRef])')
end=s.find('\n/// Compatibility entry point used by the builtin `open()` pipeline.', start)
chunk=s[start:end]
for attr in ['__file_fd__', '__file_data__', '__file_pos__', '__file_name__', '__file_mode__', '__file_binary__', 'write(true)', 'create_new(true)', 'create(true)', 'fs::write']:
print(attr, bool(chunk.find(attr) != -1))
print('contains_write_only_open_call_mapping_to_unit=', 'OpenOptions::open(&path).map(|_| ())' in chunk or '.open(&path).\n .map(|_| ())' in chunk or '.open(&path)' in chunk and '.map(|_| ())' in chunk)
print('contains_writing_attr=', 'let _ = crate::baseobjspace::setattr_str(wrapper,\n "__file_fd__",' in chunk)
PYRepository: youknowone/pyre
Length of output: 1791
Keep the opened file handle on the returned wrapper.
OpenOptions::open(...).map(|_| ()) closes the file before open() returns, while this branch stores only __file_data__ and not __file_fd__. A later write()/flush() resolves the pathname again, so "x" can create/write a different file and "a" loses its O_APPEND descriptor semantics. Retain an owned descriptor/handle or use the fd-backed implementation.
🤖 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 `@pyre/pyre-interpreter/src/builtins.rs` around lines 11740 - 11754, Update the
file-opening logic in the builtins implementation so the handle returned by
OpenOptions::open for the “x” and “a” modes is retained by the returned file
wrapper instead of discarded via map(|_| ()). Populate the wrapper’s fd-backed
field, such as __file_fd__, or reuse the existing fd-backed implementation,
while preserving the requested create-new and append semantics for later write
and flush operations.
| pub fn _getfullpathname(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> { | ||
| let (path, as_bytes) = arg_path(args, "_getfullpathname")?; | ||
| let wide = to_wide(&path); | ||
| unsafe { | ||
| let needed = GetFullPathNameW( | ||
| wide.as_ptr(), | ||
| 0, | ||
| std::ptr::null_mut(), | ||
| std::ptr::null_mut(), | ||
| ); | ||
| if needed == 0 { | ||
| return Err(last_error(&path)); | ||
| } | ||
| let mut buf = vec![0u16; needed as usize]; | ||
| let written = | ||
| GetFullPathNameW(wide.as_ptr(), needed, buf.as_mut_ptr(), std::ptr::null_mut()); | ||
| if written == 0 || written >= needed { | ||
| return Err(last_error(&path)); | ||
| } | ||
| Ok(wrap_path( | ||
| &String::from_utf16_lossy(&buf[..written as usize]), | ||
| as_bytes, | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
_getfullpathname lacks the embedded-NUL check its siblings have.
_getfinalpathname and _getdiskusage both reject '\0' before to_wide, but here a path containing NUL is silently truncated at the NUL by the Win32 wide-string convention, so ntpath.abspath("evil\0/../x") resolves a different path than the caller asked for. Reject it like the others (CPython's path converter raises ValueError: embedded null character).
🛡️ Proposed fix
let (path, as_bytes) = arg_path(args, "_getfullpathname")?;
+ if path.contains('\0') {
+ return Err(crate::PyError::value_error("embedded null character"));
+ }
let wide = to_wide(&path);📝 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.
| pub fn _getfullpathname(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> { | |
| let (path, as_bytes) = arg_path(args, "_getfullpathname")?; | |
| let wide = to_wide(&path); | |
| unsafe { | |
| let needed = GetFullPathNameW( | |
| wide.as_ptr(), | |
| 0, | |
| std::ptr::null_mut(), | |
| std::ptr::null_mut(), | |
| ); | |
| if needed == 0 { | |
| return Err(last_error(&path)); | |
| } | |
| let mut buf = vec![0u16; needed as usize]; | |
| let written = | |
| GetFullPathNameW(wide.as_ptr(), needed, buf.as_mut_ptr(), std::ptr::null_mut()); | |
| if written == 0 || written >= needed { | |
| return Err(last_error(&path)); | |
| } | |
| Ok(wrap_path( | |
| &String::from_utf16_lossy(&buf[..written as usize]), | |
| as_bytes, | |
| )) | |
| } | |
| } | |
| pub fn _getfullpathname(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> { | |
| let (path, as_bytes) = arg_path(args, "_getfullpathname")?; | |
| if path.contains('\0') { | |
| return Err(crate::PyError::value_error("embedded null character")); | |
| } | |
| let wide = to_wide(&path); | |
| unsafe { | |
| let needed = GetFullPathNameW( | |
| wide.as_ptr(), | |
| 0, | |
| std::ptr::null_mut(), | |
| std::ptr::null_mut(), | |
| ); | |
| if needed == 0 { | |
| return Err(last_error(&path)); | |
| } | |
| let mut buf = vec![0u16; needed as usize]; | |
| let written = | |
| GetFullPathNameW(wide.as_ptr(), needed, buf.as_mut_ptr(), std::ptr::null_mut()); | |
| if written == 0 || written >= needed { | |
| return Err(last_error(&path)); | |
| } | |
| Ok(wrap_path( | |
| &String::from_utf16_lossy(&buf[..written as usize]), | |
| as_bytes, | |
| )) | |
| } | |
| } |
🤖 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 `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 318 -
342, Update _getfullpathname to reject paths containing an embedded NUL
character before calling to_wide, matching the validation in _getfinalpathname
and _getdiskusage. Return the same ValueError behavior and message used by those
sibling functions, while preserving the existing Win32 path-resolution flow for
valid paths.
| let needed = GetFinalPathNameByHandleW(handle, std::ptr::null_mut(), 0, VOLUME_NAME_DOS); | ||
| if needed == 0 { | ||
| CloseHandle(handle); | ||
| return Err(last_error(&path)); | ||
| } | ||
| let mut buf = vec![0u16; needed as usize + 1]; | ||
| let written = | ||
| GetFinalPathNameByHandleW(handle, buf.as_mut_ptr(), needed, VOLUME_NAME_DOS); | ||
| CloseHandle(handle); | ||
| if written == 0 { | ||
| return Err(last_error(&path)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CloseHandle clobbers GetLastError before last_error reads it.
Both failure paths call CloseHandle(handle) and then build the error from the live GetLastError value — a successful CloseHandle overwrites it, so the OSError carries a bogus errno (typically 0/success). The same hazard is already handled deliberately in disk_free; capture the error before closing.
🐛 Proposed fix
let needed = GetFinalPathNameByHandleW(handle, std::ptr::null_mut(), 0, VOLUME_NAME_DOS);
if needed == 0 {
+ let err = last_error(&path);
CloseHandle(handle);
- return Err(last_error(&path));
+ return Err(err);
}
let mut buf = vec![0u16; needed as usize + 1];
let written =
GetFinalPathNameByHandleW(handle, buf.as_mut_ptr(), needed, VOLUME_NAME_DOS);
+ let err = if written == 0 { Some(last_error(&path)) } else { None };
CloseHandle(handle);
- if written == 0 {
- return Err(last_error(&path));
+ if let Some(err) = err {
+ return Err(err);
}📝 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.
| let needed = GetFinalPathNameByHandleW(handle, std::ptr::null_mut(), 0, VOLUME_NAME_DOS); | |
| if needed == 0 { | |
| CloseHandle(handle); | |
| return Err(last_error(&path)); | |
| } | |
| let mut buf = vec![0u16; needed as usize + 1]; | |
| let written = | |
| GetFinalPathNameByHandleW(handle, buf.as_mut_ptr(), needed, VOLUME_NAME_DOS); | |
| CloseHandle(handle); | |
| if written == 0 { | |
| return Err(last_error(&path)); | |
| } | |
| let needed = GetFinalPathNameByHandleW(handle, std::ptr::null_mut(), 0, VOLUME_NAME_DOS); | |
| if needed == 0 { | |
| let err = last_error(&path); | |
| CloseHandle(handle); | |
| return Err(err); | |
| } | |
| let mut buf = vec![0u16; needed as usize + 1]; | |
| let written = | |
| GetFinalPathNameByHandleW(handle, buf.as_mut_ptr(), needed, VOLUME_NAME_DOS); | |
| let err = if written == 0 { Some(last_error(&path)) } else { None }; | |
| CloseHandle(handle); | |
| if let Some(err) = err { | |
| return Err(err); | |
| } |
🤖 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 `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 365 -
376, Update both failure paths around GetFinalPathNameByHandleW to capture the
Windows last-error value before calling CloseHandle, then close the handle and
construct the error from the captured value. Preserve the existing
successful-path cleanup and use the same error-preservation pattern as
disk_free.
| pub fn _getfileinformation(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> { | ||
| let Some(&arg) = args.first() else { | ||
| return Err(crate::PyError::type_error( | ||
| "_getfileinformation() missing required argument 'fd'", | ||
| )); | ||
| }; | ||
| let fd = unsafe { pyre_object::w_int_get_value(arg) } as i32; | ||
| unsafe { | ||
| let handle = _get_osfhandle(fd) as Handle; | ||
| let mut info: ByHandleFileInformation = std::mem::zeroed(); | ||
| if GetFileInformationByHandle(handle, &mut info) == 0 { | ||
| return Err(last_error("")); | ||
| } | ||
| Ok(pyre_object::w_tuple_new(vec![ | ||
| pyre_object::w_int_new(info.dw_volume_serial_number as i64), | ||
| pyre_object::w_int_new(info.n_file_index_high as i64), | ||
| pyre_object::w_int_new(info.n_file_index_low as i64), | ||
| ])) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unvalidated w_int_get_value on the fd/handle/cookie arguments.
_getfileinformation, get_handle_inheritable, set_handle_inheritable and _remove_dll_directory all read argument 0 with pyre_object::w_int_get_value without an is_int check, so nt._remove_dll_directory("x") (or any non-int) reinterprets an unrelated object's payload as a raw handle/pointer and hands it to Win32 — RemoveDllDirectory/SetHandleInformation on a fabricated pointer, instead of a TypeError. Route these through the checked int conversion (crate::baseobjspace::int_w or an is_int guard) as the rest of the module does. Also worth rejecting _get_osfhandle(fd) == -1/-2 up front so a bad fd raises EBADF rather than ERROR_INVALID_HANDLE.
🤖 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 `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 386 -
405, Validate argument 0 as an integer before calling
pyre_object::w_int_get_value in _getfileinformation, get_handle_inheritable,
set_handle_inheritable, and _remove_dll_directory, returning the standard
TypeError for non-integer inputs via crate::baseobjspace::int_w or the module’s
established is_int pattern. In _getfileinformation, also reject
_get_osfhandle(fd) results of -1 or -2 before invoking
GetFileInformationByHandle and return EBADF for those invalid descriptors.
| let cut = path.rfind('\\').or_else(|| path.rfind('/')); | ||
| let parent = match cut { | ||
| Some(index) => &path[..index], | ||
| None => path.as_str(), | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parent-directory fallback drops the root separator.
rfind cuts before the separator, so C:\file.txt → C: (a drive-relative path resolved against that drive's current directory, not its root) and \file.txt → "", which GetDiskFreeSpaceExW rejects. PyPy's interp_nt._getdiskusage retries against dirname, which retains the trailing separator for root-level paths.
🐛 Proposed fix
- let cut = path.rfind('\\').or_else(|| path.rfind('/'));
+ let cut = path.rfind(['\\', '/']);
let parent = match cut {
- Some(index) => &path[..index],
+ // Keep the separator so a root-level path stays absolute
+ // (`\file` → `\`, `C:\file` → `C:\`).
+ Some(index) => &path[..=index],
None => path.as_str(),
};📝 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.
| let cut = path.rfind('\\').or_else(|| path.rfind('/')); | |
| let parent = match cut { | |
| Some(index) => &path[..index], | |
| None => path.as_str(), | |
| }; | |
| let cut = path.rfind(['\\', '/']); | |
| let parent = match cut { | |
| // Keep the separator so a root-level path stays absolute | |
| // (`\file` → `\`, `C:\file` → `C:\`). | |
| Some(index) => &path[..=index], | |
| None => path.as_str(), | |
| }; |
🤖 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 `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 438 -
442, Update the parent-path computation around the cut match to retain the
separator when the matched separator is the root boundary, so drive-root paths
such as C:\file.txt produce C:\ and rooted paths such as \file.txt produce \.
Preserve the existing parent extraction for non-root nested paths and ensure the
resulting path remains valid for GetDiskFreeSpaceExW.
moduledef.py `applevel_name = os.name` installs the one posix module under `os.name`, which is `nt` on Windows where no module named `posix` exists. Register it under `nt` there, so os.py reports `os.name == "nt"`, selects ntpath, and runs its `os.name == "nt"` branches. Port the Windows-only entries from moduledef.py's `if os.name == 'nt'` block and interp_nt.py through direct Win32 FFI: _getfullpathname, _getfinalpathname, _getfileinformation, _getdiskusage, get_handle_inheritable/set_handle_inheritable, _add_dll_directory/_remove_dll_directory, _supports_virtual_terminal. Populate nt.environ as str -> str (the nt branch of _create_environ_mapping requires str keys, matching _convertenviron's newtext) and bind the Windows open() mode flags (O_BINARY, O_TEXT, O_NOINHERIT, O_TEMPORARY, O_SHORT_LIVED, O_RANDOM, O_SEQUENTIAL).
importlib._bootstrap_external eagerly `import winreg`s on every sys.platform == "win32" build, so the module must exist for the import machinery — and `import site` — to start. Its one bootstrap consumer, WindowsRegistryFinder, is deprecated, never installed onto sys.meta_path, and reaches winreg only inside unreached methods. Expose the integer constants (HKEY_*, KEY_*, REG_*) a caller reads; the RegOpenKeyEx family is left for a follow-up.
site.getusersitepackages -> _get_path reads sys.winver to form the per-user
site-packages directory on nt; without it `import site` raises AttributeError
at startup. Bind it to the "major.minor" tag ("3.14").
The not(unix) open() path buffers writes in memory and writes to disk only on a dirty flush, so open(p, "w").close() with no write never created the file and "x" did not enforce exclusivity. Create the file when open() returns: "x" via create_new, "a" via append+create, "w"/"w+" via a truncating write.
…ed FFI The Windows nt functions declared raw `unsafe extern "system"` Win32 bindings. Delegate to rustpython_host_env::nt (getfullpathname, getfinalpathname, getdiskusage, get/set_handle_inheritable, supports_virtual_terminal, handle_from_fd) instead, and fall back to windows-sys only for AddDllDirectory/RemoveDllDirectory and GetFileInformationByHandle, which host_env does not wrap. Gate the win_nt module and its registration on the host_env feature.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc710ff41b
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| //! `sys.meta_path`, and touches `winreg.OpenKey`/`QueryValue` only inside its | ||
| //! (unreached) methods, so the registry-access functions are not ported here; | ||
| //! this exposes the integer constants a caller reads, leaving the RegOpenKeyEx | ||
| //! family for a follow-up. |
There was a problem hiding this comment.
On Windows, the first call to mimetypes.guess_type() now fails with AttributeError because lib-python/3/mimetypes.py:31-34 treats the successful winreg import as registry support, init() invokes read_windows_registry(), and line 306 unconditionally calls the missing winreg.OpenKey (the _winapi module also lacks its alternative registry accelerator). Thus adding a constants-only module breaks common MIME-type lookup; port at least the registry API used by the checked-in stdlib rather than exposing a deliberately incomplete winreg.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pyre/pyre-interpreter/src/module/posix/interp_posix.rs (2)
252-258: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
_getfullpathnameand_add_dll_directorystill don't reject embedded NUL in the path.Both build a Win32 wide string directly from
pathwithout checking for'\0'first, unlike_getfinalpathname/_getdiskusagewhich do. A NUL-containing path silently truncates at the embedded NUL forGetFullPathNameW/AddDllDirectory, resolving/registering a different, shorter path than the caller asked for. The_getfullpathnameinstance was already flagged in a prior review round.🛡️ Proposed fix
pub fn _getfullpathname(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> { let (path, as_bytes) = arg_path(args, "_getfullpathname")?; + if path.contains('\0') { + return Err(crate::PyError::value_error("embedded null character")); + } match host_nt::getfullpathname(Path::new(&path)) {pub fn _add_dll_directory(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> { use windows_sys::Win32::System::LibraryLoader::AddDllDirectory; let (path, _) = arg_path(args, "_add_dll_directory")?; + if path.contains('\0') { + return Err(crate::PyError::value_error("embedded null character")); + } let wide: Vec<u16> = path.encode_utf16().chain(std::iter::once(0)).collect();Also applies to: 349-358
🤖 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 `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 252 - 258, Update _getfullpathname and _add_dll_directory to reject paths containing an embedded '\0' before calling host_nt::getfullpathname or AddDllDirectory. Match the existing validation and error behavior used by _getfinalpathname and _getdiskusage, ensuring NUL-containing paths fail rather than being truncated.
276-296: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnvalidated
w_int_get_valueon fd/handle/cookie args (still present).
_getfileinformation,get_handle_inheritable,set_handle_inheritable, and_remove_dll_directoryall reinterpret argument 0 (and, for the latter, argument 1) viapyre_object::w_int_get_valuewithout anis_intcheck, so a non-int argument reinterprets an unrelated object's payload as a raw handle/pointer before it reaches Win32. This mirrors a previously flagged concern on the same functions.Also applies to: 317-328, 331-343, 361-372
🤖 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 `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 276 - 296, The Windows handle argument paths in _getfileinformation, get_handle_inheritable, set_handle_inheritable, and _remove_dll_directory must validate integer arguments before conversion. Add the existing is_int/type-check behavior for argument 0, and argument 1 where applicable, before calling w_int_get_value; return the appropriate type error for non-integers while preserving the current handle operations for valid integers.
🤖 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.
Duplicate comments:
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 252-258: Update _getfullpathname and _add_dll_directory to reject
paths containing an embedded '\0' before calling host_nt::getfullpathname or
AddDllDirectory. Match the existing validation and error behavior used by
_getfinalpathname and _getdiskusage, ensuring NUL-containing paths fail rather
than being truncated.
- Around line 276-296: The Windows handle argument paths in _getfileinformation,
get_handle_inheritable, set_handle_inheritable, and _remove_dll_directory must
validate integer arguments before conversion. Add the existing is_int/type-check
behavior for argument 0, and argument 1 where applicable, before calling
w_int_get_value; return the appropriate type error for non-integers while
preserving the current handle operations for valid integers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 324348a9-7a85-465e-945a-12cdd92b30b3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyre/pyre-interpreter/Cargo.tomlpyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_winapi/mod.rspyre/pyre-interpreter/src/module/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/winreg/mod.rs
1 participant
Summary
On Windows there is no module literally named
posix; PyPy'spypy/module/posix/moduledef.pysetsapplevel_name = os.name, installing theone posix module as
nt. pyre had always registered it asposix, soos.namewas"posix"on Windows and thent/ntpathcode paths stayeddormant. This branch registers the module as
nton Windows, adds theWindows-only
ntfunctions, and clears the bootstrap prerequisites the flipsurfaces.
All changes are gated
#[cfg(windows)]/not(windows)(or Windows +host_env),so unix and wasm builds are unaffected by construction.
Commits
posix: install as nt on Windows and add the nt-only callsnton Windows (pyre_install_module!("nt"(posix)));the
posixarm becomes#[cfg(all(not(wasm32), not(windows)))].os.nameis now
"nt"andos.pyselectsntpath.#[cfg(windows)] mod win_ntporting theif os.name == 'nt'moduledefblock via raw
unsafe extern "system"Win32 FFI (no windows-sys dependency,matching the
module/_winapipattern):_getfullpathname,_getfinalpathname,_getfileinformation,_getdiskusage,get_handle_inheritable,set_handle_inheritable,_add_dll_directory,_remove_dll_directory,_supports_virtual_terminal.environis populated str->str on Windows (was bytes->bytes):os.py's nt_create_environ_mappingrequires str keys, mirroring thenewtextvsnewbytessplit in PyPy_convertenviron.O_*flags:O_BINARY,O_TEXT,O_NOINHERIT,O_TEMPORARY,O_SHORT_LIVED,O_RANDOM,O_SEQUENTIAL.winreg: register a constants-only module on Windowsimportlib._bootstrap_externaleagerlyimport winregonsys.platform == "win32". Register a constants-onlywinreg(HKEY_*,KEY_*,REG_*) sobootstrap completes.
WindowsRegistryFinderis deprecated and never added tosys.meta_path, so theRegOpenKeyExfamily is a follow-up.sys: expose sys.winver on Windowssite._get_pathreadssys.winveron nt; expose it ("3.14").open: eagerly create the file for write modes on the Windows open() pathnot(unix)open()path buffers writes in memory and only writes todisk on a dirty flush, so
open(p, "w").close()(no write) never created thefile and
"x"did not enforce exclusivity. Create the file at open time tomatch the fd-backed unix path (
W_FileIO.descr_init):"x"viacreate_new(EEXIST on exist),
"a"via append+create,"w"/"w+"via a truncatingwrite.
Validation
check.py --backend dynasm: ALL PASSED 308/308 on Windows (bothPYRE_JIT=0and JIT on).
import os(os.name == "nt"),import nt,ntpath, strenviron, all ntfunctions,
winregconstants,tempfile,import siteverified working;startup is clean.
cargo fmt --all -- --checkclean.Out of scope (pre-existing Windows gaps, not caused by this flip)
subprocessneeds anmsvcrtmodule and a fuller_winapi; missing_socket/_bz2/_lzma/_tkinterC extensions. These fail identically beforethis change and are left as follow-ups.
Summary by CodeRabbit
os/nt/ntpathbehavior, including additionalposixhelpers and correct environment typing.winregmodule with registry-related integer constants, plus Windowssys.winver.open()mode flags and improved built-in module registration for early imports.open()(with correctw,a, and exclusivexsemantics), and common OS errors are mapped to appropriateOSErrorvalues.