Repository navigation
fix(libkernel): map Windows shared memory with the regular file checks - #12296
Conversation
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the Windows-only mapping change against the regular-file path and the new regression test. Descriptor lookup and access-mode checks now run for shared-memory handles as well, while disk-type refusal stays scoped to other descriptors; shared mappings still use the existing section mapper and private mappings use the copy path. The test covers shared visibility through two descriptors and pread/pwrite, nonzero offsets, private copy isolation, read-only access enforcement, and post-unlink lifetime. Current Windows CI and conventions pass; the author reports the relevant Windows mmap suite passing three times. Linux/macOS were not tested, but the code change is confined to _WIN32. No blocking issue found.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The Windows shared-memory path now takes the same descriptor lookup, access checks, and private-copy path as regular files, and the added test covers shared mappings across descriptors, file I/O coherence, offsets, read-only access, EACCES, unlink lifetime, and private-copy isolation. Hosted conventions and Windows checks pass; the author reports the Windows behavior tests, while the other platform jobs are skipped. One repository-rule item remains: the overlap check explicitly reports a conflict with open PR #11625 in PosixMemory.cpp, and #11625 is still open. CONTRIBUTING.md asks authors to agree which overlapping PR lands first and record the merge order with Depends on; the current description says no dependency. Please coordinate with #11625’s author and update the merge-order line, then rebase after the selected PR lands so the combined change preserves both behaviors.
|
The Windows mapping implementation and its hosted test run look good. One integration-rule item remains: the PR body identifies a same-line rename in #11625 and an adjacent debt-entry overlap with #10645, then says either can land first. CONTRIBUTING asks the PR authors to agree on the order and record it with |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The implementation and Windows test coverage look sound, and the Windows hosted build passes. I am requesting one contribution-rule correction before approval: this PR itself identifies overlaps with #11625 (the mapping-descriptor line) and #10645 (the libkernel debt entry), but the description says “Either can land first” without showing agreement from those authors or declaring an order under Depends on. CONTRIBUTING requires the overlapping PR authors to agree on order and record it. Please coordinate and update the checklist; I found no new code defect in this pass.
|
Windows run of this head
AI-assisted: yes (Claude Code) |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Checked the PosixMemory.cpp change and guest_named_shm_windows_mapping: shared-memory descriptors now take the same handle lookup, access checks and private copy as regular files, the directory/random-device/non-disk refusal stays limited to other descriptors, and the test pins EACCES (13) for the writable shared mapping of a read-only descriptor and private-store isolation. I found no code change to ask for, and the Windows run from @babyhuey shows no failures from this PR.
- The only open item is the landing order with #11625 and #10645 that @MarcZX10's change request is holding. Neither author has answered the questions yet. Once one of them confirms an order (or says it does not matter), record it under Depends on if it puts another PR first, and ask @MarcZX10 to re-review; no other change is needed.
|
Overlap with #12339 (mine): I agree this PR lands first. Both PRs refuse a writable shared mapping of a read-only shared-memory descriptor with |
On Windows, mmap sent a shared memory descriptor straight to the shared file mapper, skipping the checks and the private path that a regular file descriptor takes. A private mapping of a named or anonymous object then failed with EOPNOTSUPP, and a writable shared mapping of a read-only descriptor failed with EIO instead of EACCES. Shared memory objects are files on Windows, so they now take the same access checks and the same private copy as a regular file; only the directory, device and file-type refusals stay limited to other descriptors. guest_named_shm_windows_mapping covers shared mappings through two descriptors, pread and pwrite against them, an offset mapping, a private mapping, read-only mappings and the mappings after shm_unlink. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ba5f6c7 to
dc1d801
Compare
|
Rebased onto Linux: libc/libkernel build, and five memory/shared-memory groups pass three repetitions (15 passes, zero skips). The actual Windows production translation unit also passes MinGW GCC16.2.0 cross syntax checking on Linux. That is not Windows execution: the existing Windows pause remains in force, so earlier actual Windows tests retain their historical source pins. Conventions and whitespace pass. The #11625/#10645 author-order questions remain unanswered, so no agreement is claimed. #12339 has now merged; its production check and debt details are preserved in this refresh. AI-assisted: yes. Evidence: immutable Linux qualification bundle. Its SHA-256 is |
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Re-checked after the rebase: |
What
On Windows,
mmapsent a shared memory descriptor straight to the shared file mapper (DoMapFileBacked). It skipped the access checks and the private path that a regular file descriptor takes. Shared memory objects are files there: anonymous objects are delete-on-close temporary files, and named objects are files underAnyPS5/shm. Skipping those steps produced two errors:MAP_PRIVATEof a named or anonymous object failed withEOPNOTSUPP(45), becauseDoMapFileBackedrefuses private mappings on Windows. FreeBSD maps them, and a private mapping of a regular file on Windows already works as a copy of the file read atmmap(MapPrivateFileCopy).MAP_SHAREDmapping of a read-only descriptor failed withEIO(5): the section was created read-only and the writable view then failed. FreeBSD and the regular-file path returnEACCES.Shared memory descriptors now take the same handle lookup, access checks and private copy as a regular file. Only the refusal of directories, the random device and non-disk handles stays limited to other descriptors. Shared mappings still go through the same section code as before.
A new Windows test,
guest_named_shm_windows_mapping(GuestNamedShmWindowsMapping.cpp), maps a named object:preadandpwrite;shm_unlink.Until now no Windows test mapped a named object; the named-object mapping tests are Linux-only. The libkernel debt entry that said so is updated, and is now shorter than on
main.Tested
Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Ninja, Release,
BUILD_TESTING=ON, Vulkan drivers hidden. Basemain660a0b306d.main'sPosixMemory.cpp): it aborts at "a private mapping of the named object failed (guest errno 45)". With the private case moved last, it aborts at "a writable shared mapping of a read-only descriptor was not refused with EACCES (guest errno 5)". The shared, offset andpread/pwritechecks already pass onmain.mmap, plus the named shared-memory Windows tests:guest_named_shm_windows_mapping,guest_named_shm_windows(with itsshort_tempandlong_short_tempvariants),guest_shm_open,guest_jit_shared_memory,guest_memory,guest_mmap_fixed_anonymous,guest_low_address_memory,guest_mprotect_unmapped,guest_msync,guest_kernel_stack_protect,guest_posix_filesystem,guest_system_configurationand theguest_kernel_app_archive*tests.Not tested on Linux or macOS. The change is inside the
_WIN32branch only.Overlap.
tools/pr_overlap.pyreports two textual conflicts with no shared behaviour:mappingDescriptor = sourceDescriptor); merged, the line takes that name and keeps this block.Proposed merge order: this PR first, then #11625 and #10645 take the small textual merge; asked on #11625 and #10645. If either author prefers to go first, this PR will declare Depends on that PR and be rebased after it lands. #12339's author agreed that this PR lands before #12339, whose copy of the same check then becomes unreachable.
Current Linux qualification — 2026-10-29
Rebased onto
main5454cd2e455asdc1d801c4ce47fe1083ac473619f60400ad560f6. Production, regression and CMake topic additions/removals are preserved exactly. The debt resolution keeps main’s #12339 read/execute permission details alongside this PR’s regular-file mapping statement; all recursive submodule pointers match main.Linux: libc/libkernel build, and five memory/shared-memory groups pass three repetitions (15 passes, zero skips). The actual Windows production translation unit also passes MinGW GCC16.2.0 cross syntax checking on Linux. That is not Windows execution: the existing Windows pause remains in force, so earlier actual Windows tests retain their historical source pins. Conventions and whitespace pass.
The #11625/#10645 author-order questions remain unanswered, so no agreement is claimed. #12339 has now merged; its production check and debt details are preserved in this refresh. AI-assisted: yes.
Evidence: immutable Linux qualification bundle. Its SHA-256 is
ca6b35926bcc6d3f21a9650517894c86b65594e2e2b653c5c32cd1c1de89c835. No fresh Windows runtime execution is claimed; older measurements retain their recorded source pins.AI-assisted: yes (Codex).
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code