Repository navigation
fix(libkernel): replace anonymous Windows stack mappings - #6534
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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.
|
jotitadev
left a comment
There was a problem hiding this comment.
Linux run (Ubuntu 26.04, Intel i7-8700, GCC 15.2, full build, non-GPU tests), head 57c1ca733 merged onto main ce991f1ec. The description says Linux runtime was not tested.
- Builds; the 35 tests matched by the description's
ctest -Rfilter pass. That includesguest_kernel_stack_protect, whose non-Windows part is nowCheckGuardCleanup(GuardMode::Protect): the pthread-key destructor finds the guard pages writable again. It passes 30 of 30 repeated runs. - With the five libkernel files from
main(the PR's test kept), the same 35 tests also pass. That is expected: the new behaviour is inside_WIN32, and the Linux part of the test only moved into the key destructor. - Merged together with #6542, which also changes
mmap_nid_postfixandDirectMemory.cpp(the overlap bot does not list this pair): the merge is clean, it builds, the same filter plus theshmtests (42) passes, and the non-GPU suite gives the same result asmain. This covers only the Linux side of the combination.
Non-GPU suite (ctest -E "^agc"): same result as main; only file_writer_binary and file_writer_text fail, and they fail on main too on this machine (Ubuntu 26.04 coreutils).
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
karimibrahim07
left a comment
There was a problem hiding this comment.
Windows check of head 57c1ca733, merged onto main 5d9dcc42d (clean merge; it also merges cleanly with #6443). Windows 11 Pro (26300), Ryzen 5 3600, RTX 3060, WinLibs GCC 15.2.0 r7, Ninja Release.
Title: Java 25 JVM Startup (PPSA99865, 01.000.023), relinked with --windows --to-intel --trap-sdk-syscall-stubs --registry, all four rows:
- On
main,DestroyJavaVMprintsAttempt to deallocate stack guard pages failedthree times (the main thread at0x410000here, and two VM threads). - With this PR it prints none.
JNI_CreateJavaVM returned 0,Hello25 returned normallyandDestroyJavaVM completeas before.
Tests: guest_kernel_stack_protect, guest_thread_self and the other memory tests I selected pass, 5 of 5.
Code:
- I checked the lock order.
ReplaceGuestThreadStackholdsstackLockwhile the callback takesg_protectionLock. Every other holder ofg_protectionLock(RecordProtection,GuestProtection,GuestPageTableUsage) is a leaf, andDoMprotecttakesstackLock(inNoteGuestStackProtection) andg_protectionLockone after the other, not nested. So I see no inversion. - One question about the peer-stack case. When the range is on another live thread's stack, the callback
MEM_DECOMMITs and recommits it while that thread may be running, and the description says peer-stack callers "must coordinate with the peer". HotSpot only does this on a thread's own stack at teardown, so this title doesn't exercise it. Would it be safer to refuse a peer's stack (EINVAL) unless the peer is known to be stopped, rather than rely on the caller?
AI-assisted: yes. I ran the build, the tests and the title above.
57c1ca7 to
95751ec
Compare
What
Fix the
DestroyJavaVMstack-guard cleanup warnings in Java 25 LAN / DNS, PPSA99869 (#5501), reported in #5918. HotSpot requestsmmap(addr, size, PROT_NONE, MAP_PRIVATE | MAP_ANON | MAP_FIXED, -1, 0)to uncommit the guard range. The current path rejects native Windows stacks outside the guest arena.Permit fixed anonymous POSIX mappings within one current or registered native Windows stack. Decommit and recommit the pages while retaining the reservation, so a later readable mapping sees fresh zero-filled memory. Check ownership, allocation boundaries and native
PAGE_GUARDpages before mutation; reject current-stack ranges ending within 64 KiB of the active host frame. Other mapping APIs keep their existing behavior. Preserve protection intervals on either side of a replacement and recommit tracked pages during thread cleanup.The pinned OpenJDK BSD implementation and the homebrew's reservation-preservation patch establish the request; disassembly of the public release confirms flags
0x1012. Changing protection alone would retain old contents.This is separate from #5918's stack-bound reporting and #6443's
mprotectchange. The repository overlap checker reports two textual conflicts: #6091 adds<memory>where this patch adds<functional>inPthread.hpp(keep both), and #6469 edits the same fixed-mmap debt paragraph for its independent low-mapping-hint change (preserve both descriptions). Neither is a production dependency. #5703 is already merged. #6452 is a Windows full-build test-fixture prerequisite, not part of this production patch.Tested
Windows 11 Pro 26300, i7-14700K, WinLibs GCC 15.2.0 r7, Release,
BUILD_TESTING=ON.44df21632125dbe3113a19173e7ebb879da53003plus this commit (57c1ca733) and the existing test(agc): align the indirect arguments block at run time #6452 AGC test-fixture patch (4fe92c9c2). All 34 selected memory/thread tests pass, no skips, in 6.56 seconds. The seven built source/documentation files match this commit after line-ending normalization; test(agc): align the indirect arguments block at run time #6452 is the only additional change in the test tree. Conventions and whitespace checks pass.Attempt to deallocate stack guard pages failedwarnings; patched records none. Both return 0 fromJNI_CreateJavaVMand completeDestroyJavaVM. Onlylibkernel.prxdiffers across 174 runtime files. These before/after runs usedfa9365ca7plus test(agc): align the indirect arguments block at run time #6452; all affected source base blobs match current main44df21632and the tested patched sources match this patch. Full reproduction and results.Targeted test command:
Checklist
main; no other open PR implements this stack-replacement behavior (related overlaps listed above).mdfiles or images added to the repository