Windows: fix resource exhaustion in libpas/WTF (HANDLE leak, commit charge never released)#181
Merged
Jarred-Sumner merged 6 commits intomainfrom Apr 13, 2026
Merged
Conversation
- pas_thread.c: CloseHandle after _beginthreadex (scavenger thread leaked one HANDLE per shutdown/restart cycle); pass arg through; store FlsAlloc index into *key so the TLC destructor actually fires on thread exit - OSAllocatorWin.cpp: decommit() now VirtualFree(MEM_DECOMMIT) instead of MEM_RESET — MEM_RESET frees physical pages but keeps commit charge, so long-running processes hit ERROR_COMMITMENT_LIMIT. All callers already pair with commit() before reuse - pas_page_malloc.c: decommit_impl always MEM_DECOMMIT on Windows (do_mprotect=false path no longer leaks commit charge) - pas_expendable_memory.c: on Windows, recommit pages via header->states before reading payload in commit_if_necessary, since MEM_DECOMMIT makes them inaccessible Addresses Sentry crashes: pas_allocation_result_crash_on_error and pas_scavenger.c:514 PAS_ASSERT(!pthread_create) on Windows.
Try a single VirtualAlloc(MEM_COMMIT)/VirtualFree(MEM_DECOMMIT) first; only fall back to the VirtualQuery per-region loop if it fails (range spans VADs). Matches the POSIX single-madvise pattern in the common case.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughSwitches Windows decommit to VirtualFree(MEM_DECOMMIT); refactors PAS page commit/decommit to prefer single-call commits and conditional decommit flows; fixes pthread helpers to pass args/IDs and adjust once semantics; adds per-thread TLS flag for thread-local-cache teardown and adjusts destructor logic. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/bmalloc/libpas/src/libpas/pas_page_malloc.c (1)
449-465:⚠️ Potential issue | 🟡 MinorSame
VirtualQueryreturn value check needed in decommit fallback.Similar to
commit_impl, theVirtualQuerycall in the decommit fallback loop should verify success before usingmemInfo.🛡️ Proposed fix
if (!VirtualFree(ptr, size, MEM_DECOMMIT)) { size_t totalSeen = 0; void* currentPtr = ptr; while (totalSeen < size) { MEMORY_BASIC_INFORMATION memInfo; - VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); + SIZE_T queryResult = VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); + PAS_ASSERT(queryResult != 0); PAS_ASSERT(memInfo.RegionSize > 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/bmalloc/libpas/src/libpas/pas_page_malloc.c` around lines 449 - 465, The decommit fallback loop in pas_page_malloc.c uses VirtualQuery(currentPtr, &memInfo, ...) without checking its return; update the loop in the decommit branch (where VirtualFree is called in a per-region loop) to verify VirtualQuery succeeded before using memInfo (e.g. check return != 0 and assert/fail if it returns 0), then proceed to assert memInfo.RegionSize > 0 and call VirtualFree with PAS_MIN(memInfo.RegionSize, size - totalSeen); ensure you reference the existing symbols VirtualQuery, VirtualFree, memInfo, PAS_MIN, PAS_ASSERT (or a similar failure path) so the code safely handles VirtualQuery failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/bmalloc/libpas/src/libpas/pas_page_malloc.c`:
- Around line 382-397: The fallback loop after a failed VirtualAlloc calls
VirtualQuery without checking its return; update the loop in pas_page_malloc.c
so the result of VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)) is checked
and handled before using memInfo. If VirtualQuery returns 0, fail fast (e.g.,
PAS_ASSERT or return an error) or break the loop and propagate an error so
memInfo isn't used uninitialized; otherwise proceed to call
virtual_alloc_with_retry with PAS_MIN(memInfo.RegionSize, size - totalSeen) and
update currentPtr/totalSeen as now done. Ensure you reference the variables and
functions VirtualQuery, memInfo, currentPtr, totalSeen, and
virtual_alloc_with_retry when locating the fix.
---
Outside diff comments:
In `@Source/bmalloc/libpas/src/libpas/pas_page_malloc.c`:
- Around line 449-465: The decommit fallback loop in pas_page_malloc.c uses
VirtualQuery(currentPtr, &memInfo, ...) without checking its return; update the
loop in the decommit branch (where VirtualFree is called in a per-region loop)
to verify VirtualQuery succeeded before using memInfo (e.g. check return != 0
and assert/fail if it returns 0), then proceed to assert memInfo.RegionSize > 0
and call VirtualFree with PAS_MIN(memInfo.RegionSize, size - totalSeen); ensure
you reference the existing symbols VirtualQuery, VirtualFree, memInfo, PAS_MIN,
PAS_ASSERT (or a similar failure path) so the code safely handles VirtualQuery
failures.
🪄 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
Run ID: 62cf42a3-d49e-4373-bd24-2da2a4a84caa
📒 Files selected for processing (4)
Source/WTF/wtf/win/OSAllocatorWin.cppSource/bmalloc/libpas/src/libpas/pas_expendable_memory.cSource/bmalloc/libpas/src/libpas/pas_page_malloc.cSource/bmalloc/libpas/src/libpas/pas_thread.c
Comment on lines
+382
to
397
| /* Common case: range is within one MEM_RESERVE region, so a single MEM_COMMIT | ||
| works (matches POSIX's single madvise). Fall back to a per-region loop only | ||
| if it fails (range spans VADs, or OOM which the loop retries). */ | ||
| if (!VirtualAlloc(ptr, size, MEM_COMMIT, PAGE_READWRITE)) { | ||
| size_t totalSeen = 0; | ||
| void *currentPtr = ptr; | ||
| while (totalSeen < size) { | ||
| MEMORY_BASIC_INFORMATION memInfo; | ||
| VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); | ||
| PAS_ASSERT(memInfo.State != 0x10000); | ||
| PAS_ASSERT(memInfo.RegionSize > 0); | ||
| PAS_ASSERT(virtual_alloc_with_retry(currentPtr, PAS_MIN(memInfo.RegionSize, size - totalSeen), MEM_COMMIT, PAGE_READWRITE)); | ||
| currentPtr = (void*) ((uintptr_t) currentPtr + memInfo.RegionSize); | ||
| totalSeen += memInfo.RegionSize; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing error check on VirtualQuery return value.
The fallback loop calls VirtualQuery without checking its return value. If VirtualQuery fails (returns 0), memInfo will contain uninitialized/stale data, and the loop may behave incorrectly or crash on the subsequent assertions.
🛡️ Proposed fix to check VirtualQuery result
if (!VirtualAlloc(ptr, size, MEM_COMMIT, PAGE_READWRITE)) {
size_t totalSeen = 0;
void *currentPtr = ptr;
while (totalSeen < size) {
MEMORY_BASIC_INFORMATION memInfo;
- VirtualQuery(currentPtr, &memInfo, sizeof(memInfo));
+ SIZE_T queryResult = VirtualQuery(currentPtr, &memInfo, sizeof(memInfo));
+ PAS_ASSERT(queryResult != 0);
PAS_ASSERT(memInfo.State != 0x10000);
PAS_ASSERT(memInfo.RegionSize > 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
| /* Common case: range is within one MEM_RESERVE region, so a single MEM_COMMIT | |
| works (matches POSIX's single madvise). Fall back to a per-region loop only | |
| if it fails (range spans VADs, or OOM which the loop retries). */ | |
| if (!VirtualAlloc(ptr, size, MEM_COMMIT, PAGE_READWRITE)) { | |
| size_t totalSeen = 0; | |
| void *currentPtr = ptr; | |
| while (totalSeen < size) { | |
| MEMORY_BASIC_INFORMATION memInfo; | |
| VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); | |
| PAS_ASSERT(memInfo.State != 0x10000); | |
| PAS_ASSERT(memInfo.RegionSize > 0); | |
| PAS_ASSERT(virtual_alloc_with_retry(currentPtr, PAS_MIN(memInfo.RegionSize, size - totalSeen), MEM_COMMIT, PAGE_READWRITE)); | |
| currentPtr = (void*) ((uintptr_t) currentPtr + memInfo.RegionSize); | |
| totalSeen += memInfo.RegionSize; | |
| } | |
| } | |
| /* Common case: range is within one MEM_RESERVE region, so a single MEM_COMMIT | |
| works (matches POSIX's single madvise). Fall back to a per-region loop only | |
| if it fails (range spans VADs, or OOM which the loop retries). */ | |
| if (!VirtualAlloc(ptr, size, MEM_COMMIT, PAGE_READWRITE)) { | |
| size_t totalSeen = 0; | |
| void *currentPtr = ptr; | |
| while (totalSeen < size) { | |
| MEMORY_BASIC_INFORMATION memInfo; | |
| SIZE_T queryResult = VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); | |
| PAS_ASSERT(queryResult != 0); | |
| PAS_ASSERT(memInfo.State != 0x10000); | |
| PAS_ASSERT(memInfo.RegionSize > 0); | |
| PAS_ASSERT(virtual_alloc_with_retry(currentPtr, PAS_MIN(memInfo.RegionSize, size - totalSeen), MEM_COMMIT, PAGE_READWRITE)); | |
| currentPtr = (void*) ((uintptr_t) currentPtr + memInfo.RegionSize); | |
| totalSeen += memInfo.RegionSize; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/bmalloc/libpas/src/libpas/pas_page_malloc.c` around lines 382 - 397,
The fallback loop after a failed VirtualAlloc calls VirtualQuery without
checking its return; update the loop in pas_page_malloc.c so the result of
VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)) is checked and handled
before using memInfo. If VirtualQuery returns 0, fail fast (e.g., PAS_ASSERT or
return an error) or break the loop and propagate an error so memInfo isn't used
uninitialized; otherwise proceed to call virtual_alloc_with_retry with
PAS_MIN(memInfo.RegionSize, size - totalSeen) and update currentPtr/totalSeen as
now done. Ensure you reference the variables and functions VirtualQuery,
memInfo, currentPtr, totalSeen, and virtual_alloc_with_retry when locating the
fix.
Preview Builds
|
pas_expendable_memory consumers (e.g. pas_segregated_heap_size_directory_for_index, hit on every malloc) read payload directly and rely on seeing zeros after decommit (MADV_FREE semantics). MEM_DECOMMIT makes those reads AV before commit_if_necessary is ever reached. Keep DiscardVirtualMemory/MEM_RESET for the do_mprotect=false path so pages stay accessible. The do_mprotect=true path (main heap pages) keeps the optimistic MEM_DECOMMIT and now skips the redundant DiscardVirtualMemory pre-call. Reverts pas_expendable_memory.c to main.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Source/bmalloc/libpas/src/libpas/pas_page_malloc.c (1)
385-395:⚠️ Potential issue | 🟠 MajorStill guard
VirtualQuerybefore usingmemInfo.Line 390 can return 0 here. If that happens,
memInfo.StateandmemInfo.RegionSizeare undefined, so this fallback can assert on garbage or commit the wrong span on the exact failure path it is meant to harden.Proposed fix
while (totalSeen < size) { MEMORY_BASIC_INFORMATION memInfo; - VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); + SIZE_T queryResult = VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); + PAS_ASSERT(queryResult); PAS_ASSERT(memInfo.State != 0x10000); PAS_ASSERT(memInfo.RegionSize > 0); PAS_ASSERT(virtual_alloc_with_retry(currentPtr, PAS_MIN(memInfo.RegionSize, size - totalSeen), MEM_COMMIT, PAGE_READWRITE));#!/bin/bash rg -n -C2 'VirtualQuery\(currentPtr, &memInfo, sizeof\(memInfo\)\)|memInfo\.State|memInfo\.RegionSize' Source/bmalloc/libpas/src/libpas/pas_page_malloc.c🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/bmalloc/libpas/src/libpas/pas_page_malloc.c` around lines 385 - 395, The fallback loop after VirtualAlloc calls VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)) but does not check its return value before reading memInfo; modify the loop in pas_page_malloc.c (inside the VirtualAlloc failure path) to verify VirtualQuery returned nonzero and handle failure (e.g., PAS_ASSERT(returned != 0) or break/return an error) before accessing memInfo.State or memInfo.RegionSize, and ensure virtual_alloc_with_retry is only called with a validated RegionSize; reference the loop variables currentPtr, totalSeen, memInfo, and the call to virtual_alloc_with_retry to locate and fix the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/bmalloc/libpas/src/libpas/pas_page_malloc.c`:
- Around line 454-479: The VirtualQuery calls in the decommit and
DiscardVirtualMemory branches must have their return values checked before using
memInfo.RegionSize to avoid undefined behavior or infinite loops; update both
call sites (the VirtualQuery near the VirtualFree loop and the VirtualQuery near
the VirtualAlloc loop) to verify VirtualQuery(...) != 0 (or PAS_ASSERT on the
return) and handle failure (e.g., break the loop or assert) rather than
proceeding to use memInfo.RegionSize, so VirtualFree/VirtualAlloc are only
called with a valid RegionSize.
---
Duplicate comments:
In `@Source/bmalloc/libpas/src/libpas/pas_page_malloc.c`:
- Around line 385-395: The fallback loop after VirtualAlloc calls
VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)) but does not check its
return value before reading memInfo; modify the loop in pas_page_malloc.c
(inside the VirtualAlloc failure path) to verify VirtualQuery returned nonzero
and handle failure (e.g., PAS_ASSERT(returned != 0) or break/return an error)
before accessing memInfo.State or memInfo.RegionSize, and ensure
virtual_alloc_with_retry is only called with a validated RegionSize; reference
the loop variables currentPtr, totalSeen, memInfo, and the call to
virtual_alloc_with_retry to locate and fix the code.
🪄 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
Run ID: 3bf82eef-ffd3-4de3-a622-a25f6afbb522
📒 Files selected for processing (1)
Source/bmalloc/libpas/src/libpas/pas_page_malloc.c
Comment on lines
+454
to
+479
| if (!VirtualFree(ptr, size, MEM_DECOMMIT)) { | ||
| size_t totalSeen = 0; | ||
| void* currentPtr = ptr; | ||
| while (totalSeen < size) { | ||
| MEMORY_BASIC_INFORMATION memInfo; | ||
| VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); | ||
| PAS_ASSERT(memInfo.RegionSize > 0); | ||
| PAS_ASSERT(VirtualFree(currentPtr, PAS_MIN(memInfo.RegionSize, size - totalSeen), MEM_DECOMMIT)); | ||
| currentPtr = (void*)((uintptr_t)currentPtr + memInfo.RegionSize); | ||
| totalSeen += memInfo.RegionSize; | ||
| } | ||
| } | ||
| } else { | ||
| /* do_mprotect=false callers (pas_expendable_memory) read payload directly | ||
| and rely on seeing zeros after decommit, like MADV_FREE. MEM_DECOMMIT | ||
| would make those reads AV, so use DiscardVirtualMemory which frees | ||
| physical RAM but keeps pages accessible. This does not release commit | ||
| charge, but expendable memory is bounded metadata. */ | ||
| if (DiscardVirtualMemory(ptr, size)) { | ||
| size_t totalSeen = 0; | ||
| void* currentPtr = ptr; | ||
| while (totalSeen < size) { | ||
| MEMORY_BASIC_INFORMATION memInfo; | ||
| VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); | ||
| PAS_ASSERT(memInfo.RegionSize > 0); | ||
| PAS_ASSERT(VirtualAlloc(currentPtr, PAS_MIN(memInfo.RegionSize, size - totalSeen), MEM_RESET, PAGE_READWRITE)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'VirtualQuery\(currentPtr, &memInfo, sizeof\(memInfo\)\)|VirtualFree\(currentPtr|VirtualAlloc\(currentPtr' Source/bmalloc/libpas/src/libpas/pas_page_malloc.cRepository: oven-sh/WebKit
Length of output: 1307
🏁 Script executed:
#!/bin/bash
# Get full context around lines 454-481
sed -n '450,485p' Source/bmalloc/libpas/src/libpas/pas_page_malloc.c | cat -nRepository: oven-sh/WebKit
Length of output: 2210
🏁 Script executed:
#!/bin/bash
# Also check the context around line 390 to see if it has the same issue
sed -n '385,400p' Source/bmalloc/libpas/src/libpas/pas_page_malloc.c | cat -nRepository: oven-sh/WebKit
Length of output: 921
🏁 Script executed:
#!/bin/bash
# Check for any error handling or memset patterns around VirtualQuery
rg -n 'VirtualQuery' Source/bmalloc/libpas/src/libpas/pas_page_malloc.c -B2 -A2Repository: oven-sh/WebKit
Length of output: 1029
🏁 Script executed:
#!/bin/bash
# Search for all VirtualQuery calls in the file
rg -n 'VirtualQuery' Source/bmalloc/libpas/src/libpas/pas_page_malloc.cRepository: oven-sh/WebKit
Length of output: 272
Check both decommit-side VirtualQuery calls before consuming memInfo.
Lines 459 and 477 lack return value validation before using memInfo.RegionSize. When VirtualQuery fails (returns 0), memInfo remains uninitialized, causing undefined behavior in subsequent VirtualFree/VirtualAlloc calls or infinite loops.
Proposed fix
if (!VirtualFree(ptr, size, MEM_DECOMMIT)) {
size_t totalSeen = 0;
void* currentPtr = ptr;
while (totalSeen < size) {
MEMORY_BASIC_INFORMATION memInfo;
- VirtualQuery(currentPtr, &memInfo, sizeof(memInfo));
+ SIZE_T queryResult = VirtualQuery(currentPtr, &memInfo, sizeof(memInfo));
+ PAS_ASSERT(queryResult);
PAS_ASSERT(memInfo.RegionSize > 0);
PAS_ASSERT(VirtualFree(currentPtr, PAS_MIN(memInfo.RegionSize, size - totalSeen), MEM_DECOMMIT));
currentPtr = (void*)((uintptr_t)currentPtr + memInfo.RegionSize);
totalSeen += memInfo.RegionSize;
}
@@
if (DiscardVirtualMemory(ptr, size)) {
size_t totalSeen = 0;
void* currentPtr = ptr;
while (totalSeen < size) {
MEMORY_BASIC_INFORMATION memInfo;
- VirtualQuery(currentPtr, &memInfo, sizeof(memInfo));
+ SIZE_T queryResult = VirtualQuery(currentPtr, &memInfo, sizeof(memInfo));
+ PAS_ASSERT(queryResult);
PAS_ASSERT(memInfo.RegionSize > 0);
PAS_ASSERT(VirtualAlloc(currentPtr, PAS_MIN(memInfo.RegionSize, size - totalSeen), MEM_RESET, PAGE_READWRITE));
currentPtr = (void*)((uintptr_t)currentPtr + memInfo.RegionSize);
totalSeen += memInfo.RegionSize;
}📝 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
| if (!VirtualFree(ptr, size, MEM_DECOMMIT)) { | |
| size_t totalSeen = 0; | |
| void* currentPtr = ptr; | |
| while (totalSeen < size) { | |
| MEMORY_BASIC_INFORMATION memInfo; | |
| VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); | |
| PAS_ASSERT(memInfo.RegionSize > 0); | |
| PAS_ASSERT(VirtualFree(currentPtr, PAS_MIN(memInfo.RegionSize, size - totalSeen), MEM_DECOMMIT)); | |
| currentPtr = (void*)((uintptr_t)currentPtr + memInfo.RegionSize); | |
| totalSeen += memInfo.RegionSize; | |
| } | |
| } | |
| } else { | |
| /* do_mprotect=false callers (pas_expendable_memory) read payload directly | |
| and rely on seeing zeros after decommit, like MADV_FREE. MEM_DECOMMIT | |
| would make those reads AV, so use DiscardVirtualMemory which frees | |
| physical RAM but keeps pages accessible. This does not release commit | |
| charge, but expendable memory is bounded metadata. */ | |
| if (DiscardVirtualMemory(ptr, size)) { | |
| size_t totalSeen = 0; | |
| void* currentPtr = ptr; | |
| while (totalSeen < size) { | |
| MEMORY_BASIC_INFORMATION memInfo; | |
| VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); | |
| PAS_ASSERT(memInfo.RegionSize > 0); | |
| PAS_ASSERT(VirtualAlloc(currentPtr, PAS_MIN(memInfo.RegionSize, size - totalSeen), MEM_RESET, PAGE_READWRITE)); | |
| if (!VirtualFree(ptr, size, MEM_DECOMMIT)) { | |
| size_t totalSeen = 0; | |
| void* currentPtr = ptr; | |
| while (totalSeen < size) { | |
| MEMORY_BASIC_INFORMATION memInfo; | |
| SIZE_T queryResult = VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); | |
| PAS_ASSERT(queryResult); | |
| PAS_ASSERT(memInfo.RegionSize > 0); | |
| PAS_ASSERT(VirtualFree(currentPtr, PAS_MIN(memInfo.RegionSize, size - totalSeen), MEM_DECOMMIT)); | |
| currentPtr = (void*)((uintptr_t)currentPtr + memInfo.RegionSize); | |
| totalSeen += memInfo.RegionSize; | |
| } | |
| } | |
| } else { | |
| /* do_mprotect=false callers (pas_expendable_memory) read payload directly | |
| and rely on seeing zeros after decommit, like MADV_FREE. MEM_DECOMMIT | |
| would make those reads AV, so use DiscardVirtualMemory which frees | |
| physical RAM but keeps pages accessible. This does not release commit | |
| charge, but expendable memory is bounded metadata. */ | |
| if (DiscardVirtualMemory(ptr, size)) { | |
| size_t totalSeen = 0; | |
| void* currentPtr = ptr; | |
| while (totalSeen < size) { | |
| MEMORY_BASIC_INFORMATION memInfo; | |
| SIZE_T queryResult = VirtualQuery(currentPtr, &memInfo, sizeof(memInfo)); | |
| PAS_ASSERT(queryResult); | |
| PAS_ASSERT(memInfo.RegionSize > 0); | |
| PAS_ASSERT(VirtualAlloc(currentPtr, PAS_MIN(memInfo.RegionSize, size - totalSeen), MEM_RESET, PAGE_READWRITE)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/bmalloc/libpas/src/libpas/pas_page_malloc.c` around lines 454 - 479,
The VirtualQuery calls in the decommit and DiscardVirtualMemory branches must
have their return values checked before using memInfo.RegionSize to avoid
undefined behavior or infinite loops; update both call sites (the VirtualQuery
near the VirtualFree loop and the VirtualQuery near the VirtualAlloc loop) to
verify VirtualQuery(...) != 0 (or PAS_ASSERT on the return) and handle failure
(e.g., break the loop or assert) rather than proceeding to use
memInfo.RegionSize, so VirtualFree/VirtualAlloc are only called with a valid
RegionSize.
…ead exit Storing the FLS index makes the libpas TLC destructor fire on thread exit for the first time on Windows. The destroy() path (pas_thread_local_cache_shrink / node_deallocate) crashes at 0x0 from inside the ntdll FLS callback, reproducibly on NAPI/Worker/spawn tests across x64 and arm64. Restore the original behavior (key unset, FlsSetValue silently fails, destructor never runs) until the destroy() path is hardened for Windows. This leaves the per-dead-thread TLC leak in place; tracked separately.
…down On Windows, FLS callbacks fire before C++ thread_local destructors. The old destructor poisoned the __thread TLC pointer to (void*)1 before calling destroy(), so any later thread_local destructor that freed via bmalloc dereferenced (void*)1 and faulted near 0x0. Replace the in-band sentinel with a separate __thread bool. The destructor now sets is_exiting=true and TLC=NULL, so: - can_set() returns false (no new TLC created during teardown) - raw __thread reads see NULL and take the safe slow path - destroy() and any subsequent thread_local destructors run cleanly Affects all non-Darwin platforms (semantically equivalent on Linux where ordering already happened to work). Re-enables *key=FlsAlloc(...) so the TLC destructor actually fires on Windows thread exit.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/bmalloc/libpas/src/libpas/pas_thread_local_cache.c`:
- Around line 156-166: The teardown clears the TLS slot but creation still
occurs because
pas_thread_local_cache_get_with_heap_lock_hold_mode()/pas_thread_local_cache_get_slow()
will create a new TLC when the slot is NULL; update the creation path so it
first checks pas_thread_local_cache_can_set() (or the
pas_thread_local_cache_is_exiting flag) and refuses to allocate a new TLC if
false. Specifically, add a guard in
pas_thread_local_cache_get_with_heap_lock_hold_mode (or inside
pas_thread_local_cache_get_slow where new TLC is allocated) that returns the
slow/safe path (or NULL) instead of calling the TLC creation routine when
!pas_thread_local_cache_can_set(), ensuring
pas_thread_local_cache_set_impl(NULL) and pas_thread_local_cache_is_exiting
effectively prevent recreating the cache during teardown.
🪄 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
Run ID: e1fa0f75-7967-455b-963a-6081a082806f
📒 Files selected for processing (2)
Source/bmalloc/libpas/src/libpas/pas_thread_local_cache.cSource/bmalloc/libpas/src/libpas/pas_thread_local_cache.h
Comment on lines
+156
to
166
| /* Mark the thread as exiting so can_set() returns false and no new TLC is created, | ||
| and clear the TLC pointer to NULL so any alloc/dealloc that runs during destroy() | ||
| or in later thread_local destructors (which on Windows run after FLS callbacks) | ||
| takes the safe slow path instead of dereferencing a poisoned pointer. */ | ||
| pas_thread_local_cache_is_exiting = true; | ||
| pas_thread_local_cache_set_impl(NULL); | ||
| PAS_ASSERT(!pas_thread_local_cache_can_set()); | ||
| #endif | ||
|
|
||
| if (((uintptr_t)thread_local_cache) != PAS_THREAD_LOCAL_CACHE_DESTROYED) | ||
| if (thread_local_cache && ((uintptr_t)thread_local_cache) != PAS_THREAD_LOCAL_CACHE_DESTROYED) | ||
| destroy(thread_local_cache, pas_lock_is_not_held); |
There was a problem hiding this comment.
Gate TLC creation itself once teardown starts.
Line 161 clears the TLS slot, but that still lets pas_thread_local_cache_get_with_heap_lock_hold_mode() fall through to pas_thread_local_cache_get_slow() whenever the slot is NULL. So any teardown-time caller that reaches pas_thread_local_cache_get() can recreate the TLC after we've marked the thread as exiting here, which undermines the Windows-exit fix. Please enforce !pas_thread_local_cache_can_set() in the creation path itself, not just in the _if_can_set_cache_... helper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/bmalloc/libpas/src/libpas/pas_thread_local_cache.c` around lines 156 -
166, The teardown clears the TLS slot but creation still occurs because
pas_thread_local_cache_get_with_heap_lock_hold_mode()/pas_thread_local_cache_get_slow()
will create a new TLC when the slot is NULL; update the creation path so it
first checks pas_thread_local_cache_can_set() (or the
pas_thread_local_cache_is_exiting flag) and refuses to allocate a new TLC if
false. Specifically, add a guard in
pas_thread_local_cache_get_with_heap_lock_hold_mode (or inside
pas_thread_local_cache_get_slow where new TLC is allocated) that returns the
slow/safe path (or NULL) instead of calling the TLC creation routine when
!pas_thread_local_cache_can_set(), ensuring
pas_thread_local_cache_set_impl(NULL) and pas_thread_local_cache_is_exiting
effectively prevent recreating the cache during teardown.
…he struct) pthread_once_t was PINIT_ONCE (= INIT_ONCE*), so PTHREAD_ONCE_INIT initialized it to a NULL pointer and InitOnceExecuteOnce(*once_control) passed NULL. Never reached before because the only Windows-reachable caller (pas_baseline_allocator_table_initialize_if_necessary) is on the no-TLC allocation path, which only fires after the TLC is destroyed. Now that the FLS destructor runs (and on Windows, thread_local destructors run AFTER it), WTF::Thread::didExit -> ThreadSafeWeakHashSet::remove -> HashTable::rehash -> fastZeroedMalloc allocates with no TLC -> baseline path -> pthread_once(NULL) -> fault at 0x0. Fix: typedef INIT_ONCE (the struct), pass once_control directly, return 0 on success. Also make the callback wrapper static WINAPI.
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
1 participant
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.
Problem
Windows Bun processes hit resource exhaustion over time — Sentry shows
pas_allocation_result_crash_on_error(OOM) across unrelated callers (ParkingLot, DFG compile, GC, code cache) pluspas_scavenger.c:514 PAS_ASSERT(!pthread_create). The unrelated call sites point at process-wide commit/handle exhaustion, not a single-site bug.Root causes & fixes
1. Scavenger HANDLE leak —
pas_thread.c_beginthreadexreturns a HANDLE that was never closed;pthread_detachwas a no-op. Withis_shut_down_enabled=true(default;Options::libpasScavengeContinuouslydefaults to false), the scavenger exits after ~10s idle and is recreated on the next alloc burst — leaking one kernel HANDLE per cycle. Fix:CloseHandle(hThread)immediately (closing a thread handle doesn't terminate the thread). Also passargthrough and store the thread ID in*tid.2. FLS index dropped —
pas_thread.cpthread_key_creatediscarded theFlsAllocresult, sopthread_setspecific(fast_tls->key, ...)wrote to a garbage slot and the TLC destructor never fired on thread exit. Fix:*key = result.3.
OSAllocator::decommitnever released commit charge —OSAllocatorWin.cppMEM_RESET+VirtualUnlockfrees physical RAM but keeps commit charge. JSC's GC block decommits all go through here, so commit grows monotonically untilERROR_COMMITMENT_LIMIT. All callers (PageReservation,StructureAlignedMemoryAllocator,ExecutableAllocator) already pair withcommit()before reuse, soVirtualFree(MEM_DECOMMIT)is safe.4.
pas_page_mallocdecommitdo_mprotect=falsepath leaked commit —pas_page_malloc.c+pas_expendable_memory.cThe
do_mprotect=falsepath used onlyDiscardVirtualMemory/MEM_RESET. Now alwaysMEM_DECOMMIT. The only Windows-reachable caller ispas_expendable_memory(TLC shrink is Darwin-only), which previously assumed touch-after-decommit was legal. Added a Windows-only pre-check incommit_if_necessarythat scansheader->states[first..last](separate non-decommitted allocation) forKIND_DECOMMITTEDand recommits before readingobject[-1]from payload.note_useonly touchesheader->statesso needs no change.Notes
OfferVirtualMemory/DiscardVirtualMemory/VirtualAlloc2all preserve commit charge by design — onlyVirtualFree(MEM_DECOMMIT)releases it.