fix(windows): use tryProtect in BufferMemoryHandle destructor to prevent mprotect crash#170
Open
robobun wants to merge 1 commit intooven-sh:mainfrom
Open
fix(windows): use tryProtect in BufferMemoryHandle destructor to prevent mprotect crash#170robobun wants to merge 1 commit intooven-sh:mainfrom
robobun wants to merge 1 commit intooven-sh:mainfrom
Conversation
…ent mprotect crash On Windows, the BufferMemoryHandle destructor calls OSAllocator::protect() to restore page permissions before freeing memory. This calls VirtualAlloc(MEM_COMMIT) which fails with ERROR_INVALID_ADDRESS (487) if the underlying virtual memory reservation was already released by the allocator (libpas recycling virtual pages). This manifests as a flaky crash in Bun's buffer tests on Windows: mprotect failed: 487 (exit code 0xC0000409 / STATUS_STACK_BUFFER_OVERRUN) The protect-before-free pattern is unnecessary on Windows because VirtualFree(MEM_RELEASE) releases memory regardless of page protection state (unlike POSIX where some free paths use madvise which may require accessible pages). Replace OSAllocator::protect() with OSAllocator::tryProtect() in both the Signaling and Shared BoundsChecking destructor paths on Windows. Non-Windows platforms continue to use the fatal protect() call. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
WalkthroughBufferMemoryHandle.cpp destructor on Windows builds now uses OSAllocator::tryProtect() instead of OSAllocator::protect() for memory protection (fast and growable bounds memory). Non-Windows platforms retain existing behavior. CMakeLists.txt updated with 16 new lines. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OSAllocator::tryProtect()instead ofOSAllocator::protect()inBufferMemoryHandle::~BufferMemoryHandle()on Windows for both the Signaling and Shared BoundsChecking destructor pathsmprotect failed: 487/ exit code0xC0000409) in Bun's buffer tests on WindowsProblem
On Windows, the
BufferMemoryHandledestructor callsOSAllocator::protect()to restore page permissions (fromPAGE_NOACCESSguard pages back toPAGE_READWRITE) before freeing memory. This translates toVirtualAlloc(addr, size, MEM_COMMIT, PAGE_READWRITE).This call fails with
ERROR_INVALID_ADDRESS(487) when the underlying virtual memory reservation has already been released by the allocator (libpas recycling virtual pages). The fatalprotect()then callsRELEASE_ASSERT_NOT_REACHED(), crashing the process.The crash manifests in Bun CI as:
This is flaky because it depends on GC timing and allocator behavior after running hundreds of tests.
Why the fix is correct
The protect-before-free exists to make
PAGE_NOACCESSguard pages accessible before releasing them. On POSIX, some free paths usemadvisewhich may require accessible pages. On Windows,VirtualFree(MEM_RELEASE)releases memory regardless of page protection state, so restoring permissions before freeing is not necessary.By using
tryProtect()(which returnsfalseon failure instead of crashing), the destructor gracefully handles the case where the reservation is already gone, and proceeds to free the memory normally.Non-Windows platforms continue to use the fatal
protect()call, preserving existing behavior.Test plan
buffer.test.jsno longer crashes on Windows x64-baseline CI#if OS(WINDOWS)guarded)🤖 Generated with Claude Code