fix(gc): prune DATA_VIEW_REGISTRY and SHARED_ARRAY_BUFFER_REGISTRY on buffer death (#6337)#6341
Merged
Merged
Conversation
added 3 commits
July 13, 2026 07:29
… buffer death (#6337) `finalize_collected_dead_buffer` exists to drop "every registry/side-table entry keyed by a dead buffer's address" — its doc comment cites preventing the #6080 ABA class — but it only pruned BUFFER_REGISTRY, ARRAY_BUFFER_REGISTRY and BUFFER_AB_ALIAS. `DATA_VIEW_REGISTRY` and `SHARED_ARRAY_BUFFER_REGISTRY` had zero `.remove`/`.retain` sites anywhere in the tree. Two consequences, the same pair proven for the WebCrypto tables in #6302/#6310: * unbounded leak — one permanent entry per DataView / SAB-flagged buffer ever created, for the life of the process; * ABA / address reuse — `arena_reset_empty_blocks` resets a fully-empty block's offset to 0 while keeping its base pointer, so a reset block re-issues the same addresses and a stale entry answers for an unrelated object. `is_data_view`/`is_shared_array_buffer` gate `util.types.isDataView` / `isSharedArrayBuffer`, `ArrayBuffer.isView`, the `[object DataView]` tag, and the structuredClone / `.slice()` re-marking paths. Both are now pruned in the post-full-trace sweep, alongside the existing three. SharedArrayBuffer needed a second, separate fix. A process-global SAB backing (`shared_sab::alloc_shared_sab`) is NOT a GC allocation: it comes straight from `alloc_zeroed`, carries no GcHeader, and is never freed — that is what lets the bytes alias across `perry/thread` agents (#4913). But `js_shared_array_buffer_new` does `register_buffer` it, so it reaches `registered_buffer_is_dead_post_trace` on every full trace, where `try_read_gc_header` reads the 8 bytes BEFORE the malloc block and interprets the allocator's own metadata as a GcHeader: one arbitrary byte compared against GC_TYPE_BUFFER (10), the next against the mark/pin/forward bits. A chance match declares a LIVE, never-freed SAB dead — and `finalize_collected_dead_buffer` then runs `view::remove_entries_for_dead_buffer`, which retains on `info.backing != addr` and so unregisters EVERY live typed-array view over that SAB: exactly the views cross-agent `Atomics` wait/notify resolve their absolute slot addresses through. So the backing is now vetoed as a dead candidate before any header is sniffed, rather than pruned. The entries that DO get pruned from SHARED_ARRAY_BUFFER_REGISTRY are the arena-allocated SAB-flagged copies (`SharedArrayBuffer.prototype.slice`, structuredClone), which are ordinary GC-heap buffers that genuinely die and whose addresses genuinely get recycled. Cross-thread aliasing, the process-global registry and the Atomics futex table are untouched. The veto sits behind a `SHARED_SAB_NONEMPTY` latch mirroring `EXTERNAL_BUFFERS_NONEMPTY`, so processes that never allocate a SAB — nearly all of them — answer from one relaxed atomic load and never take the registry lock. That also removes an unconditional mutex acquisition from `is_registered_buffer`'s fallback, which JSON.stringify runs per serialized pointer (#6009). Tests (`gc/tests/buffer_side_tables.rs`): dead DataView / SAB-flagged buffer pruned on full GC; live (rooted) ones survive; a 4096-entry leak regression that asserts both registries DRAIN to baseline after their owners are swept; and two SAB-liveness tests, one seeding the shared-SAB registry to prove the veto deterministically (the malloc-metadata coincidence cannot be forced without writing outside the allocation), one end-to-end over a real `new SharedArrayBuffer(64)`.
GcTestIsolationGuard does not push a shadow frame, so js_shadow_slot_set under it roots nothing — the buffers stayed unreachable and the safety inverse would have passed for the wrong reason. Use CopyingNurseryTestGuard, which pushes the frame (same as the dead_owner_side_tables precedent).
The veto in registered_buffer_is_dead_post_trace called is_shared_sab per registered buffer, taking the process-global registry mutex once per buffer per full trace in any program that allocates a SharedArrayBuffer. The set is tiny and the GC is stop-the-world here, so lock it once per scan and hand the filter a plain set. Returns None — and skips the check entirely, allocating nothing — for the processes that never allocate a SAB.
📝 WalkthroughWalkthroughThe change adds process-global SharedArrayBuffer registry snapshots, prevents SAB-backed buffers from being misclassified during GC scanning, and prunes DataView and SharedArrayBuffer identity registries during dead-buffer finalization. New tests cover cleanup, retention, registry draining, and process-global SAB survival. ChangesBuffer GC cleanup
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 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 |
![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
🤖 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 `@crates/perry-runtime/src/shared_sab.rs`:
- Around line 90-98: Update is_shared_sab and snapshot_shared_sabs to recover
poisoned registry mutexes using the poisoned lock’s into_inner() guard, matching
alloc_shared_sab. Remove the unwrap_or(false) and .ok() paths so GC checks
continue using the registry after lock poisoning.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 92eaab2b-0faf-4710-b998-bab758f9832f
📒 Files selected for processing (5)
crates/perry-runtime/src/buffer/header.rscrates/perry-runtime/src/buffer/mod.rscrates/perry-runtime/src/gc/tests/buffer_side_tables.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/shared_sab.rs
Comment on lines
90
to
98
| pub fn is_shared_sab(addr: usize) -> bool { | ||
| if !SHARED_SAB_NONEMPTY.load(Ordering::Acquire) { | ||
| return false; | ||
| } | ||
| registry() | ||
| .lock() | ||
| .map(|r| r.contains(&addr)) | ||
| .unwrap_or(false) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' crates/perry-runtime/src/shared_sab.rs | cat -nRepository: PerryTS/perry
Length of output: 7738
🏁 Script executed:
rg -n "snapshot_shared_sabs|is_shared_sab" crates/perry-runtime crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 2251
🏁 Script executed:
sed -n '430,520p' crates/perry-runtime/src/buffer/header.rs | cat -nRepository: PerryTS/perry
Length of output: 5072
Recover poisoned SAB registry locks
is_shared_sab() and snapshot_shared_sabs() still treat a poisoned mutex as “no SABs exist” (unwrap_or(false) / .ok()), while alloc_shared_sab() already recovers with into_inner(). If a panic ever poisons this lock, the GC scan can stop vetoing live SAB backings and misclassify them as dead. Mirror the same recovery here instead of swallowing the poison.
🤖 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 `@crates/perry-runtime/src/shared_sab.rs` around lines 90 - 98, Update
is_shared_sab and snapshot_shared_sabs to recover poisoned registry mutexes
using the poisoned lock’s into_inner() guard, matching alloc_shared_sab. Remove
the unwrap_or(false) and .ok() paths so GC checks continue using the registry
after lock poisoning.
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.
Fixes #6337.
This PR edits the same function as open PR #6310 (issue #6302,
KeyObject.from()):finalize_collected_dead_bufferincrates/perry-runtime/src/buffer/header.rs. #6310 adds the WebCrypto/KeyObject tables to it; this PR adds the buffer-identity tables. Please merge #6310 first, then this one.Verified: the two do NOT textually conflict. I deliberately anchored my prune block after
ARRAY_BUFFER_REGISTRY, rather than afterBUFFER_AB_ALIASwhere #6310 inserts, specifically to keep the hunks apart.git merge-treeagainst #6310's head (573ad38ce) auto-merges clean, and the mergedfinalize_collected_dead_buffercontains both sets of prunes:So no rebase should be needed either way — but if anything does drift, this branch is trivially rebasable onto #6310. This PR does not duplicate any of #6310's work.
The bug
finalize_collected_dead_bufferexists to drop "every registry/side-table entry keyed by a dead buffer's address" — its own doc comment cites preventing the #6080 ABA class. But it only prunedBUFFER_REGISTRY,ARRAY_BUFFER_REGISTRYandBUFFER_AB_ALIAS.DATA_VIEW_REGISTRYandSHARED_ARRAY_BUFFER_REGISTRYhad zero.remove/.retainsites anywhere in the tree. Same pair of consequences already proven for the WebCrypto tables in #6302/#6310:DataView/ SAB-flagged buffer ever created, for the life of the process.arena_reset_empty_blocksresets a fully-empty block's offset to 0 while keeping its base pointer, so a reset block re-issues the same addresses. A stale entry then answers for an unrelated object.is_data_view/is_shared_array_buffergateutil.types.isDataView/isSharedArrayBuffer,ArrayBuffer.isView, the[object DataView]tag, and the structuredClone /.slice()re-marking paths — a fresh Buffer landing on a recycled address would answer to all of them.Both are now pruned in the post-full-trace sweep, alongside the existing three. (Buffers are TENURED old-gen residents; minor traces never mark the old generation, so only a full trace can prove them dead. Pruning rides the same
CollectionSideBufferssubphase #6310 uses.)The SharedArrayBuffer wrinkle — a second, separate bug
Investigating the SAB side surfaced a soundness bug that is not just hygiene, and the fix is the opposite of pruning.
A process-global SAB backing (
shared_sab::alloc_shared_sab) is not a GC allocation: it comes straight fromalloc_zeroed, carries noGcHeader, and is never freed — that is exactly what lets its bytes alias acrossperry/threadagents (#4913). Butjs_shared_array_buffer_newdoesregister_bufferit, so it lands inBUFFER_REGISTRYand reachesregistered_buffer_is_dead_post_traceon every full trace.There,
try_read_gc_headerreads the 8 bytes before the malloc block — the allocator's own metadata — and interprets them as aGcHeader.obj_typeandgc_flagsare bothu8, so this is one arbitrary byte compared againstGC_TYPE_BUFFER(10) and the next against the mark/pin/forward bits. A chance match declares a live, never-freed SAB dead.finalize_collected_dead_bufferthen runs on it — includingview::remove_entries_for_dead_buffer, which doesretain(|_, info| info.backing != addr)and so unregisters every live typed-array view over that SAB: precisely the views cross-agentAtomics.wait/notifyresolve their absolute slot addresses through.So the backing is now vetoed as a dead candidate before any header is sniffed, rather than pruned. This also removes an out-of-bounds read on a raw malloc block.
Nothing about cross-thread aliasing changes. The process-global registry (
shared_sab), the shared backing store, and theatomics_futextable are untouched. The entries that do get pruned fromSHARED_ARRAY_BUFFER_REGISTRYare the arena-allocated SAB-flagged copies —SharedArrayBuffer.prototype.slice(object/buffer_dispatch.rs), structuredClone (builtins/globals.rs) — which are ordinary GC-heap buffers that genuinely die and whose addresses genuinely get recycled. Those are the only SAB-registry entries that were ever collectable, and they were the leak.Per the issue's warning, the fix does not pin anything:
GC_FLAG_PINNEDobjects are not traced (gc/cycle.rsonly enqueues whenflags & (MARKED|PINNED) == 0), so pinning would trade a stale entry for an untraced-reference walk and make the leak permanent by design.Incidental: the veto is free
The veto sits behind a
SHARED_SAB_NONEMPTYlatch mirroring the existingEXTERNAL_BUFFERS_NONEMPTYidiom, so processes that never allocate a SAB — nearly all of them — answer from one relaxed atomic load and never take the registry lock. That also removes an unconditional mutex acquisition fromis_registered_buffer's final fallback, which JSON.stringify runs per serialized pointer (#6009) — the existingEXTERNAL_BUFFERS_NONEMPTYlatch was being partly defeated by theis_shared_sabcall right after it.Leak numbers
New test
test_data_view_and_sab_registries_drain_after_owners_die: allocate 4096DataViews + 4096 SAB-flagged buffers, drop every reference, run one full collection.DATA_VIEW_REGISTRYSHARED_ARRAY_BUFFER_REGISTRYTests
New
crates/perry-runtime/src/gc/tests/buffer_side_tables.rs(6 tests):test_dead_data_view_registry_entry_pruned_on_full_gctest_dead_shared_array_buffer_flag_pruned_on_full_gctest_data_view_and_sab_registries_drain_after_owners_die— the leak regression abovetest_live_data_view_and_shared_array_buffer_flags_survive_full_gc— safety inverse; live (rooted) views keep their entriestest_seeded_shared_sab_is_never_a_dead_buffer_candidate— proves the SAB veto deterministically. The malloc-metadata coincidence cannot be forced without writing outside the allocation, so this seeds an ordinary GC buffer (whose real header genuinely says "dead") into the shared-SAB registry and asserts the dead-buffer scan still refuses to report it.test_process_global_sab_backing_survives_full_gc_unrooted— end-to-end over a realnew SharedArrayBuffer(64).All four bug-specific tests fail without the fix (verified by reverting the prune + veto: 4 FAILED, and the two safety-inverse tests still pass — so they aren't vacuous in either direction).
Validation
PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1.PERRY_GC_DIAG=1confirms collections actually ran (freed_bytes=262144/48, andlonglived=4 (1 live)on the rooted case) rather than being silently vacuous.perry-runtimelib suite: 1291 passed, 0 failed.cargo fmt --all -- --checkclean;scripts/check_file_size.shclean.