fix(codegen,runtime): crypto natives accept an SSO string argument; hash.update(String(n)) no longer segfaults (#11430 follow-up) - #11486
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (17)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change adds runtime conversion for NaN-boxed SSO string arguments and uses it in crypto code generation and runtime pointer handling. It also adds a regression test covering short-string inputs to several crypto APIs. ChangesCrypto short-string arguments
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The inspected short-string crypto paths do not show a remaining issue that should block merging. Proceed with normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change addresses a crash when short strings reach crypto functions. Reviewed consumers copy those strings before retaining data or starting asynchronous work. The temporary storage has a finite lifetime, so the remaining risk is whether every affected caller follows that contract. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 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 |
fbc4614 to
19d2296
Compare
|
Merge queue: CI was green. #11485 landed and conflicted in |
Follow-up to #11468 (#11430).
hash.update(<short string>)segfaulted inbytes_from_ptr; the #11453 agent reported it. #11468 did not cover it.What was wrong
This is the same SSO bug as #11468, but on the crypto call sites. Since #10762,
String(n),`${n}`andn.toString()return small values as SSO strings, whose characters live inline in the NaN-box. The crypto natives receive a string argument as ani64*const StringHeader, and both sides unboxed it withbits & POINTER_MASK:expr/calls/crypto_{hash,kdf,keys,misc}.rs(48 sites) did this throughunbox_to_i64.arg_ptrand open-coded masks incrypto/{cipher,ecdh,hash_handles}.rs.An SSO argument therefore became a garbage address that
bytes_from_ptrdereferenced.createHash("sha256").update(String(7))bytes_from_ptr←decode_hash_update_value←dispatch_hashcreateHmac("sha256", String(1))js_crypto_create_hmacpbkdf2Sync(String(9), String(8), …),hkdfSync(…, String(1), …),cipher.update(String(5))The fix
js_ffi_arg_ptr(f64) -> i64inperry-runtime/src/value/nanbox.rs. It copies an SSO argument's bytes into a per-thread ring of 16 non-GC,StringHeader-shaped slots and returns the slot's address. Every other value unboxes exactly asbits & POINTER_MASKdid.bytes_from_ptrcopies them into aVec.js_ffi_arg_ptrthrough a newunbox_ffi_str_arghelper in place ofunbox_to_i64.crypto::x509::arg_ptr, the three open-coded masks andhash_handles::unbox_to_i64all use the same entry.FFI_SSO_RINGis recorded inscripts/gc_runtime_root_holders.jsonasnot_a_gc_pointer. The slots are ownedBoxmemory with no GC pointers.Evidence
Arm A is main with #11468 and arm B is this branch. Both were built the same way (
cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static, codegen-units 16,PERRY_NO_AUTO_OPTIMIZE=1) on Linux x86_64. The oracle is Node 26.5.1.test_gap_11430_crypto_short_string_args: SSO values throughhash.updatewith and without an encoding, a template string, chained updates, an HMAC key and data,pbkdf2Sync,hkdfSync, and an AES-256-CBC round trip. A segfaults; B is byte-identical to Node.test_gap_11430_buffer_write_short_string(from fix(runtime): Buffer write/indexOf accept an SSO string; parameterised pg queries no longer segfault (#11430) #11468) still matches Node on B.Pooland a pool-client transaction), with both the scram-sha-256 and trust users: all 20/20 on B.Checks run
cargo test --release -p perry-codegen: 2267 passed, 0 failed.cargo test --release -p perry-stdlib --lib crypto: 26 passed.RUSTFLAGS=-D warnings cargo check -p perry-runtime -p perry-stdlib -p perry-codegen --all-targets: clean.cargo fmt --check: clean.gc_runtime_root_holders.py,string_payload_access_inventory.pyandcheck_file_size.sh: all OK.Not run
run_lint_gates.shon this final commit. The individual gates above were run.and.Not changed
Other native families that take a string as a masked
i64may have the same SSO exposure. This PR covers the crypto family only.Summary by CodeRabbit