fix(crypto): KeyObject.from() must handle asymmetric and unregistered-buffer CryptoKeys (#6302)#6310
Conversation
📝 WalkthroughWalkthrough
ChangesCryptoKey Object Conversion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NodeCrypto
participant RuntimeKeyObject
participant WebCrypto
participant KeyObject
participant GC
NodeCrypto->>RuntimeKeyObject: call KeyObject.from(CryptoKey)
RuntimeKeyObject->>WebCrypto: dispatch keyObjectFromCryptoKey
WebCrypto->>WebCrypto: read bytes and encode surrogate
WebCrypto-->>RuntimeKeyObject: return converted key
RuntimeKeyObject-->>KeyObject: return converted KeyObject
GC->>RuntimeKeyObject: finalize CryptoKey backing buffer
RuntimeKeyObject->>WebCrypto: notify crypto key death
WebCrypto-->>RuntimeKeyObject: remove stored key material
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.
🧹 Nitpick comments (1)
test-parity/node-suite/crypto/key-object/keyobject-from-cryptokey.ts (1)
1-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an unsupported CryptoKey case. The fixture covers the supported shapes and invalid inputs, but not the
ERR_CRYPTO_UNSUPPORTED_OPERATIONpath for Ed448/X448/ML-KEM. Add onereport(...)case for an unsupported CryptoKey shape to lock that behavior in.🤖 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 `@test-parity/node-suite/crypto/key-object/keyobject-from-cryptokey.ts` around lines 1 - 119, Add a report(...) case near the other invalid-input checks using a CryptoKey generated with an unsupported algorithm such as Ed448, X448, or ML-KEM, and pass it to KeyObject.from. Ensure the fixture observes the expected ERR_CRYPTO_UNSUPPORTED_OPERATION error path without changing the existing supported-key coverage.
🤖 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.
Nitpick comments:
In `@test-parity/node-suite/crypto/key-object/keyobject-from-cryptokey.ts`:
- Around line 1-119: Add a report(...) case near the other invalid-input checks
using a CryptoKey generated with an unsupported algorithm such as Ed448, X448,
or ML-KEM, and pass it to KeyObject.from. Ensure the fixture observes the
expected ERR_CRYPTO_UNSUPPORTED_OPERATION error path without changing the
existing supported-key coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aea605e0-7645-4cfa-b1bd-75f856181bbe
📒 Files selected for processing (5)
crates/perry-runtime/src/object/native_module_crypto_key_object.rscrates/perry-stdlib/src/crypto/util.rscrates/perry-stdlib/src/webcrypto.rscrates/perry-stdlib/src/webcrypto/key_object.rstest-parity/node-suite/crypto/key-object/keyobject-from-cryptokey.ts
proggeramlug
force-pushed
the
fix/6302-keyobject-from
branch
from
d94d7cb to
e21fef7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-runtime/src/object/native_module_crypto_key_object.rs (1)
92-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFactor out the shared WebCrypto dispatch boilerplate.
keyObject.from()andkeyObject.toCryptoKey()both load/transmuteJS_NATIVE_WEBCRYPTO_DISPATCH; a small shared helper would keep theunsafesignature and argument packing in one place.🤖 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/object/native_module_crypto_key_object.rs` around lines 92 - 115, Factor the repeated WebCrypto dispatch setup from asymmetric_key_object and keyObject.toCryptoKey into a shared helper that loads and validates JS_NATIVE_WEBCRYPTO_DISPATCH, performs the unsafe function-pointer conversion, and packs the NaN-boxed arguments before invoking the requested method. Update both callers to reuse this helper while preserving their existing method names and result-specific error handling.
🤖 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/object/native_module_crypto_key_object.rs`:
- Around line 64-90: Update the CryptoKey metadata lifecycle used by
secret_key_object and crypto_key_bytes so registry entries cannot outlive their
BufferHeader. Either root each associated buffer for the lifetime of its
CryptoKey metadata, or update finalize_collected_dead_buffer to remove metadata
when the buffer is swept and handle address reuse consistently. Ensure stale
CryptoKey entries cannot be dereferenced after collection.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/native_module_crypto_key_object.rs`:
- Around line 92-115: Factor the repeated WebCrypto dispatch setup from
asymmetric_key_object and keyObject.toCryptoKey into a shared helper that loads
and validates JS_NATIVE_WEBCRYPTO_DISPATCH, performs the unsafe function-pointer
conversion, and packs the NaN-boxed arguments before invoking the requested
method. Update both callers to reuse this helper while preserving their existing
method names and result-specific error handling.
🪄 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: d0f36114-3d70-4b7d-8fb2-369381558d2e
📒 Files selected for processing (5)
crates/perry-runtime/src/object/native_module_crypto_key_object.rscrates/perry-stdlib/src/crypto/util.rscrates/perry-stdlib/src/webcrypto.rscrates/perry-stdlib/src/webcrypto/key_object.rstest-parity/node-suite/crypto/key-object/keyobject-from-cryptokey.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/perry-stdlib/src/webcrypto.rs
- test-parity/node-suite/crypto/key-object/keyobject-from-cryptokey.ts
…-buffer CryptoKeys (#6302) crypto.KeyObject.from() only accepted a secret CryptoKey whose backing BufferHeader was in the calling thread's buffer registry (kind == 1 && is_registered_buffer). Every other CryptoKey — including a perfectly valid public/private key from subtle.generateKey()/importKey() — fell through to invalid_key() and never produced a KeyObject. - runtime: dispatch on the CryptoKey kind. Secret keys copy into a secret-key Buffer (no buffer-registry precondition: CryptoKey metadata is only ever attached to a BufferHeader, and it also lives in a process-global registry, so the bytes are readable regardless). Public/private keys route to the WebCrypto bridge. - stdlib: new keyObjectFromCryptoKey bridge re-encodes asymmetric key material (RSA SPKI/PKCS#8 DER, EC SEC1/scalar, Ed25519/X25519 raw) into the PEM/surrogate strings Perry's KeyObject surrogates use, and flags them with mark_as_asymmetric_key, so type / asymmetricKeyType / export / equals / toCryptoKey / sign / verify all work. - key material Perry cannot model (Ed448, X448, ML-KEM) now throws instead of silently yielding a non-KeyObject. - fixture: test-parity/node-suite/crypto/key-object/keyobject-from-cryptokey.ts
…swept `finalize_collected_dead_buffer` exists to "drop every registry/side-table entry keyed by a dead buffer's address" so a recycled address cannot inherit the dead buffer's identity (the #6080 ABA class). The WebCrypto/KeyObject tables were missing from that list: * CRYPTO_KEY_META_REGISTRY (thread-local addr -> CryptoKeyMeta) * SECRET_KEY_REGISTRY (thread-local) * UINT8ARRAY_FROM_CTOR (thread-local) * external_buffers/uint8arrays/crypto_keys (process-global; all three are written together by js_buffer_mark_as_crypto_key_external) * perry-stdlib's CRYPTO_KEY_REGISTRY (addr -> CryptoKeyMaterial), the map lookup_crypto_key consults first None of them root the BufferHeader, so both an unbounded leak and a stale- identity hazard followed. Measured with a temporary probe in the sweep: a run creating 60k HMAC CryptoKeys and dropping every JS reference swept 59,998 of their buffers while retaining 59,998 CryptoKey metadata entries. The identity hazard is not hypothetical: the old arena resets a fully-empty block's offset to 0 while keeping its base pointer (arena_reset_empty_blocks plus the block-reuse forward scan in Arena::alloc), so a reset block hands out the same addresses again. crypto_key_meta/is_secret_key gate `instanceof CryptoKey`, util.types.isCryptoKey/isKeyObject, the [object CryptoKey] tag, the .algorithm/.type/.usages property surface, KeyObject.from() and .export() — an unrelated fresh Buffer landing on a dead key's address would answer to all of them, and lookup_crypto_key would hand it to the subtle sign/verify paths. perry-runtime cannot call into perry-stdlib, so the stdlib map is pruned via a death hook installed at startup, mirroring the existing js_set_native_*_dispatch pattern. The callback only removes a HashMap entry — no allocation, so it is safe to run inside the sweep. Pruning is driven by the existing post-full-trace dead-buffer collection, so a live CryptoKey (which is reachable precisely because the JS value IS the BufferHeader pointer) is never affected.
proggeramlug
force-pushed
the
fix/6302-keyobject-from
branch
from
e21fef7 to
573ad38
Compare
CodeRabbit "Root CryptoKey metadata or prune it with buffer GC" — verified REAL, fixed in 573ad38Rebased onto What I verifiedThe factual core of the finding is correct.
None of them root the Reproduced (not just reasoned about)Temporary probe in the sweep, 60k HMAC 59,998 of 60,000 key buffers were collected while their CryptoKey metadata stayed in the registry — an unbounded leak (one entry per CryptoKey ever created, in three maps, for the life of the process). After the fix the same probe shows the registry draining One correction to the finding's framingCodeRabbit describes it as The real hazard is address reuse (ABA), which is exactly what this finalizer was written to prevent — and it is not hypothetical: the old arena resets a fully-empty block's offset to 0 while keeping its base pointer ( Pre-existing vs. newThe leak and the ABA are pre-existing on Fix (573ad38, code-only)Added the missing tables to I deliberately did not reach for Not touched (same ABA class, but outside this finding and this PR's code path): Validation
Re the nitpick on |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/buffer/header.rs`:
- Around line 545-562: Optimize finalize_collected_dead_buffer by gating each
external_* mutex lock with its corresponding nonempty atomic latch, following
the pattern used by is_registered_buffer. Capture the removal result from
external_crypto_keys() and call notify_crypto_key_death(addr) only when an entry
was actually removed, while continuing to clear all relevant tables for real
CryptoKey buffers.
- Around line 519-544: Update finalize_collected_dead_buffer to remove the
address from ASYMMETRIC_KEY_REGISTRY alongside CRYPTO_KEY_META_REGISTRY and
SECRET_KEY_REGISTRY. Ensure mark_as_asymmetric_key entries are cleared when the
buffer is finalized, preventing stale KeyObject metadata on recycled addresses.
🪄 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: 33a1daa2-96c2-4829-a998-8179d71967e0
📒 Files selected for processing (9)
crates/perry-runtime/src/buffer/header.rscrates/perry-runtime/src/buffer/mod.rscrates/perry-runtime/src/object/native_module_crypto_key_object.rscrates/perry-stdlib/src/common/dispatch/init.rscrates/perry-stdlib/src/crypto/util.rscrates/perry-stdlib/src/webcrypto.rscrates/perry-stdlib/src/webcrypto/key_object.rscrates/perry-stdlib/src/webcrypto/util.rstest-parity/node-suite/crypto/key-object/keyobject-from-cryptokey.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/perry-stdlib/src/crypto/util.rs
- crates/perry-runtime/src/object/native_module_crypto_key_object.rs
- test-parity/node-suite/crypto/key-object/keyobject-from-cryptokey.ts
- crates/perry-stdlib/src/webcrypto/key_object.rs
| // The WebCrypto/KeyObject side tables were missing from this list. They are | ||
| // plain `addr -> metadata` maps that do not root the `BufferHeader`, so a | ||
| // collected CryptoKey/secret-key buffer left its entries behind forever. | ||
| // Two consequences, both real: | ||
| // | ||
| // * an unbounded leak — every CryptoKey ever created kept an entry in the | ||
| // thread-local map AND in the process-global one (a 60k-key run leaked | ||
| // 59,998 of them); | ||
| // * the #6080 ABA class this very function exists to prevent: the old | ||
| // arena resets a fully-empty block's offset to 0 while keeping its base | ||
| // pointer (`arena_reset_empty_blocks` + the block-reuse forward scan in | ||
| // `Arena::alloc`), so a recycled address inherits CryptoKey identity. | ||
| // `crypto_key_meta`/`is_secret_key` gate `instanceof CryptoKey`, | ||
| // `util.types.isCryptoKey`/`isKeyObject`, the `[object CryptoKey]` tag, | ||
| // the `.algorithm`/`.type`/`.usages` property surface, `KeyObject.from` | ||
| // and `.export()` — an unrelated fresh Buffer landing on a dead key's | ||
| // address would answer to all of them. | ||
| CRYPTO_KEY_META_REGISTRY.with(|r| { | ||
| r.borrow_mut().remove(&addr); | ||
| }); | ||
| SECRET_KEY_REGISTRY.with(|r| { | ||
| r.borrow_mut().remove(&addr); | ||
| }); | ||
| UINT8ARRAY_FROM_CTOR.with(|r| { | ||
| r.borrow_mut().remove(&addr); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "ASYMMETRIC_KEY_REGISTRY" -C4 --type=rust
rg -n "fn mark_as_asymmetric_key|fn is_asymmetric_key|asymmetric_key_meta" -C6 --type=rustRepository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files and references to key registries / asymmetric key helpers.
git ls-files | rg 'crates/perry-runtime/src/(buffer|crypto|webcrypto|key|mod).*\.rs$|crates/perry-runtime/src/.+\.rs$'
rg -n "ASYMMETRIC_KEY_REGISTRY|CRYPTO_KEY_META_REGISTRY|SECRET_KEY_REGISTRY|UINT8ARRAY_FROM_CTOR|mark_as_asymmetric_key|is_asymmetric_key|asymmetric" crates/perry-runtime/src -C 3
# Inspect the buffer header cleanup area and the asymmetric-key related definitions/usages if any.
ast-grep outline crates/perry-runtime/src/buffer/header.rs --view expanded
ast-grep outline crates/perry-runtime/src/buffer/mod.rs --view expandedRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the prior search finds no asymmetric registry symbol, inspect the key-related API surface to see
# whether any asymmetric identity/type checks are implemented under different names.
rg -n "KeyObject|CryptoKey|secret key|asymmetric key|public key|private key|export\\(|instanceof CryptoKey|isCryptoKey|isKeyObject|util.types.isCryptoKey|util.types.isKeyObject" crates/perry-runtime/src -C 2
# Show the cleanup function around the cited lines.
sed -n '500,560p' crates/perry-runtime/src/buffer/header.rsRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any other address-keyed registries in buffer/header.rs and adjacent modules.
rg -n "thread_local!|addr ->|remove\\(&addr\\)|insert\\(addr|HashMap<usize|HashMap<u64|BTreeMap<usize|BTreeMap<u64" crates/perry-runtime/src/buffer -C 2
# Enumerate nearby files in the buffer module.
find crates/perry-runtime/src/buffer -maxdepth 2 -type f | sortRepository: PerryTS/perry
Length of output: 15415
Remove ASYMMETRIC_KEY_REGISTRY in finalize_collected_dead_buffer
mark_as_asymmetric_key populates the same addr-keyed side table, and it gates KeyObject brand/type/property dispatch (instanceof, util.types.isKeyObject, toCryptoKey, .export(), type/asymmetricKeyType/asymmetricKeyDetails). Leaving it behind leaks entries and lets a recycled address inherit stale KeyObject identity.
🤖 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/buffer/header.rs` around lines 519 - 544, Update
finalize_collected_dead_buffer to remove the address from
ASYMMETRIC_KEY_REGISTRY alongside CRYPTO_KEY_META_REGISTRY and
SECRET_KEY_REGISTRY. Ensure mark_as_asymmetric_key entries are cleared when the
buffer is finalized, preventing stale KeyObject metadata on recycled addresses.
| // `js_buffer_mark_as_crypto_key_external` writes all three global maps, and | ||
| // `is_registered_buffer`/`is_uint8array_buffer` consult them, so a dead | ||
| // external key buffer has to be dropped from every one of them. | ||
| if let Ok(mut r) = external_buffers().lock() { | ||
| r.remove(&addr); | ||
| } | ||
| if let Ok(mut r) = external_uint8arrays().lock() { | ||
| r.remove(&addr); | ||
| } | ||
| if let Ok(mut r) = external_crypto_keys().lock() { | ||
| r.remove(&addr); | ||
| } | ||
| // perry-stdlib keeps its own `addr -> CryptoKeyMaterial` map (the primary | ||
| // one `lookup_crypto_key` consults; the runtime table above is only its | ||
| // fallback), and this crate cannot call into perry-stdlib. Notify it | ||
| // through the hook it installs at startup. The callback only removes a | ||
| // HashMap entry — no allocation, so it is safe to run inside the sweep. | ||
| notify_crypto_key_death(addr); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
New unconditional global-mutex locks + cross-crate FFI hook run on every dead buffer, not just CryptoKeys.
Lines 548-556 add three brand-new external_buffers()/external_uint8arrays()/external_crypto_keys() .lock() calls to finalize_collected_dead_buffer, and notify_crypto_key_death(addr) unconditionally invokes the perry-stdlib hook (which itself locks CRYPTO_KEY_REGISTRY) — for every dead buffer, including the overwhelming majority that were never CryptoKeys. This runs on the GC sweep's per-dead-object hot path.
is_registered_buffer already shows the cheaper pattern for exactly this: it gates the external_buffers().lock() behind the EXTERNAL_BUFFERS_NONEMPTY atomic latch to skip the lock entirely when the map has never been populated. None of that reuse happens here, and the removals' own return values (HashMap::remove → Option, HashSet::remove → bool) are discarded even though they'd tell you for free whether this address was ever a crypto key — which is exactly the condition needed to skip the stdlib hook call.
⚡ Proposed fix — skip the expensive paths for non-crypto buffers
- CRYPTO_KEY_META_REGISTRY.with(|r| {
- r.borrow_mut().remove(&addr);
- });
- SECRET_KEY_REGISTRY.with(|r| {
- r.borrow_mut().remove(&addr);
- });
+ let was_crypto_key_meta =
+ CRYPTO_KEY_META_REGISTRY.with(|r| r.borrow_mut().remove(&addr).is_some());
+ let was_secret_key = SECRET_KEY_REGISTRY.with(|r| r.borrow_mut().remove(&addr));
UINT8ARRAY_FROM_CTOR.with(|r| {
r.borrow_mut().remove(&addr);
});
// `js_buffer_mark_as_crypto_key_external` writes all three global maps, and
// `is_registered_buffer`/`is_uint8array_buffer` consult them, so a dead
// external key buffer has to be dropped from every one of them.
- if let Ok(mut r) = external_buffers().lock() {
- r.remove(&addr);
+ if EXTERNAL_BUFFERS_NONEMPTY.load(std::sync::atomic::Ordering::Acquire) {
+ if let Ok(mut r) = external_buffers().lock() {
+ r.remove(&addr);
+ }
}
if let Ok(mut r) = external_uint8arrays().lock() {
r.remove(&addr);
}
- if let Ok(mut r) = external_crypto_keys().lock() {
- r.remove(&addr);
- }
- // perry-stdlib keeps its own `addr -> CryptoKeyMaterial` map ...
- notify_crypto_key_death(addr);
+ let was_external_crypto_key = external_crypto_keys()
+ .lock()
+ .map(|mut r| r.remove(&addr).is_some())
+ .unwrap_or(false);
+ // perry-stdlib keeps its own `addr -> CryptoKeyMaterial` map ...
+ if was_crypto_key_meta || was_secret_key || was_external_crypto_key {
+ notify_crypto_key_death(addr);
+ }This still fully clears every table for actual CryptoKey buffers; it just avoids the mutex/FFI overhead for the common case of ordinary Buffers.
🤖 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/buffer/header.rs` around lines 545 - 562, Optimize
finalize_collected_dead_buffer by gating each external_* mutex lock with its
corresponding nonempty atomic latch, following the pattern used by
is_registered_buffer. Capture the removal result from external_crypto_keys() and
call notify_crypto_key_death(addr) only when an entry was actually removed,
while continuing to clear all relevant tables for real CryptoKey buffers.
… buffer death (#6337) (#6341) * fix(gc): prune DATA_VIEW_REGISTRY and SHARED_ARRAY_BUFFER_REGISTRY on 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)`. * test(gc): root the live-survival buffers in a real shadow frame (#6337) 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). * perf(gc): snapshot the SAB registry once per dead-buffer scan (#6337) 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. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Fixes #6302 (split out of #2565, whose three headline deliverables shipped).
Root cause
native_module_crypto_key_object::key_object_fromaccepted exactly one shape:Anything that was not a secret CryptoKey backed by a buffer present in the
calling thread's buffer registry fell through to
invalid_key(). A publicor private CryptoKey straight out of
subtle.generateKey()/importKey()isa perfectly valid input for Node's
KeyObject.from(), but Perry rejected it —on
mainthe asymmetric cases die with:The
is_registered_bufferhalf of the guard was wrong too: CryptoKey metadatais only ever attached to a
BufferHeaderallocated by the WebCrypto keyfactories, and that metadata also lives in a process-global registry, so the
key bytes are readable even when the thread-local buffer registry has no entry.
The checked-in fixture
keyobject-class-from.tsonly passes because it importsfrom
secret.export()— an already-registered secret buffer — so the gap wasuncovered.
Fix
Perry models the two KeyObject shapes differently:
BufferHeaderflaggedmark_as_secret_keymark_as_asymmetric_keyso
key_object_fromnow dispatches on the CryptoKey'skind:kind == secret→ copy into a secret-key Buffer (buffer-registryprecondition dropped).
kind == public | private→ route through the WebCrypto dispatch hook to anew
keyObjectFromCryptoKeybridge in perry-stdlib (the same bridgekeyObject.toCryptoKey()uses in the opposite direction). It re-encodes theWebCrypto key material — RSA SPKI/PKCS#8 DER, EC SEC1 point / raw scalar
(P-256/384/521), raw 32-byte Ed25519/X25519 — into exactly the
PEM/surrogate strings
createPublicKey()/createPrivateKey()/generateKeyPairSync()produce, and flags them withmark_as_asymmetric_key.type,asymmetricKeyType,export(),equals(),toCryptoKey()and thecrypto.sign/crypto.verifypathstherefore all work on the result.
throws
Error [ERR_CRYPTO_UNSUPPORTED_OPERATION]instead of yieldingsomething unusable. Node returns a KeyObject there — a remaining gap, but a
loud one.
invalid_key()has no other callers, so there is no other silent-undefinedsite in this module to fix.
Before / after vs
node --experimental-strip-types(node 26.3)mainKeyObject.from(<Ed25519 public CryptoKey>)publicERR_INVALID_ARG_TYPEpublicKeyObject.from(<Ed25519 private CryptoKey>)privateERR_INVALID_ARG_TYPEprivateKeyObject.from(<ECDSA P-256 pair>)public/private,asymmetricKeyType: ec, sign+verify round-tripsKeyObject.from(<RSASSA-PKCS1-v1_5 pair>)public/private,asymmetricKeyType: rsaKeyObject.from(<X25519 pair>)/<ECDH P-384>public/privateKeyObject.from(<HMAC CryptoKey from Buffer.from(hex)>)secretsecretsecretKeyObject.from(undefined | null | {} | "x" | 7 | Buffer | KeyObject)TypeError ERR_INVALID_ARG_TYPETests
New fixture
test-parity/node-suite/crypto/key-object/keyobject-from-cryptokey.tscovers the unregistered-buffer secret keys (HMAC + AES imported from
Buffer.from(hex)), the asymmetric Ed25519 / ECDSA P-256 / RSA cases includinga
crypto.sign+crypto.verifyround-trip through the produced KeyObjects,and the seven invalid-input throws. It fails on
main(the asymmetric blockthrows) and is byte-identical to Node with this change.
Verified locally against
node --experimental-strip-types:test-parity/node-suite/crypto/key-object/fixtures byte-identicalcrypto/{subtle,webcrypto,asymmetric,secret-key,keygen}byte-identicalcargo fmt --all -- --check,scripts/check_file_size.sh,scripts/gc_store_site_inventory.pyandscripts/addr_class_inventory.pyclean for the touched files (the
addr_classratchet failure oncrates/perry-runtime/src/map.rsis pre-existing onmain— PR fix(runtime): Map/BigInt bare-address branches must reject the handle bands (unblocks main) #6297).Summary by CodeRabbit
crypto.KeyObject.from(cryptoKey)for asymmetric CryptoKeys (in addition to secret keys), covering common algorithms such as HMAC, AES-GCM, Ed25519, ECDSA (P-256), X25519, and RSA.