Skip to content

fix(codegen,runtime): crypto natives accept an SSO string argument; hash.update(String(n)) no longer segfaults (#11430 follow-up) - #11486

Merged
proggeramlug merged 3 commits into
mainfrom
fix-11430-crypto-sso-args
Sep 27, 2026
Merged

proggeramlug merged 3 commits into
mainfrom
fix-11430-crypto-sso-args

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #11468 (#11430). hash.update(<short string>) segfaulted in bytes_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}` and n.toString() return small values as SSO strings, whose characters live inline in the NaN-box. The crypto natives receive a string argument as an i64 *const StringHeader, and both sides unboxed it with bits & POINTER_MASK:

  • The codegen arms in expr/calls/crypto_{hash,kdf,keys,misc}.rs (48 sites) did this through unbox_to_i64.
  • The stdlib dispatch side did it through arg_ptr and open-coded masks in crypto/{cipher,ecdh,hash_handles}.rs.

An SSO argument therefore became a garbage address that bytes_from_ptr dereferenced.

on main (the merged #11468 included) Node Perry before
createHash("sha256").update(String(7)) digest SIGSEGV in bytes_from_ptr ← decode_hash_update_value ← dispatch_hash
createHmac("sha256", String(1)) digest SIGSEGV ← js_crypto_create_hmac
pbkdf2Sync(String(9), String(8), …), hkdfSync(…, String(1), …), cipher.update(String(5)) values SIGSEGV

The fix

  • Runtime: a new js_ffi_arg_ptr(f64) -> i64 in perry-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 as bits & POINTER_MASK did.
    • It copies rather than materializing onto the GC heap. A heap copy would be an unrooted temporary: the next argument's materialization (for example HMAC's algorithm, then its key) can collect or move it before the native call runs.
    • The natives only read these bytes during the call; bytes_from_ptr copies them into a Vec.
  • Codegen: the crypto arms call js_ffi_arg_ptr through a new unbox_ffi_str_arg helper in place of unbox_to_i64.
  • Stdlib: crypto::x509::arg_ptr, the three open-coded masks and hash_handles::unbox_to_i64 all use the same entry.
  • Inventory: FFI_SSO_RING is recorded in scripts/gc_runtime_root_holders.json as not_a_gc_pointer. The slots are owned Box memory 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.

  • New gap test test_gap_11430_crypto_short_string_args: SSO values through hash.update with 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.
  • pg on the main thread, 20 runs each against PostgreSQL 16.15 (a parameterised query, and the full probe covering connect, params, transactions, Pool and 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.py and check_file_size.sh: all OK.

Not run

  • A full gap sweep.
  • run_lint_gates.sh on this final commit. The individual gates above were run.
  • macOS.
  • An instruction-count A/B. Each crypto string argument now costs an extern call instead of an and.

Not changed

Other native families that take a string as a masked i64 may have the same SSO exposure. This PR covers the crypto family only.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed crashes in crypto operations when short inline strings were used as algorithms, keys, data, salts, encodings, or other string arguments.
    • Added regression coverage for short-string inputs across hashing, HMAC, key derivation, and cipher operations.

proggeramlug pushed a commit that referenced this pull request Sep 27, 2026
@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 02d4f6f3-9795-442f-a84e-dc1f11c8e776

📥 Commits

Reviewing files that changed from the base of the PR and between fbc4614 and 19d2296.

📒 Files selected for processing (3)
  • crates/perry-stdlib/src/crypto/ecdh.rs
  • crates/perry-stdlib/src/crypto/hash_handles.rs
  • scripts/gc_runtime_root_holders.json
 ________________________________________________________________________
< OpenAI said I could be anything I wanted, so I became a code reviewer. >
 ------------------------------------------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 7cf7a221-fa57-4b12-9a17-02ddc18f6847

📥 Commits

Reviewing files that changed from the base of the PR and between d83d15e and fbc4614.

📒 Files selected for processing (17)
  • changelog.d/11486-crypto-sso-args.md
  • crates/perry-codegen/src/expr/calls.rs
  • crates/perry-codegen/src/expr/calls/crypto_hash.rs
  • crates/perry-codegen/src/expr/calls/crypto_kdf.rs
  • crates/perry-codegen/src/expr/calls/crypto_keys.rs
  • crates/perry-codegen/src/expr/calls/crypto_misc.rs
  • crates/perry-codegen/src/expr/helpers.rs
  • crates/perry-codegen/src/expr/mod.rs
  • crates/perry-codegen/src/runtime_decls/strings.rs
  • crates/perry-runtime/src/value/mod.rs
  • crates/perry-runtime/src/value/nanbox.rs
  • crates/perry-stdlib/src/crypto/cipher.rs
  • crates/perry-stdlib/src/crypto/ecdh.rs
  • crates/perry-stdlib/src/crypto/hash_handles.rs
  • crates/perry-stdlib/src/crypto/x509.rs
  • scripts/gc_runtime_root_holders.json
  • test-files/test_gap_11430_crypto_short_string_args.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Crypto short-string arguments

Layer / File(s) Summary
Runtime string argument conversion
crates/perry-runtime/src/value/nanbox.rs, crates/perry-runtime/src/value/mod.rs, crates/perry-codegen/src/expr/helpers.rs, crates/perry-codegen/src/expr/mod.rs, crates/perry-codegen/src/runtime_decls/strings.rs, crates/perry-stdlib/src/crypto/x509.rs, scripts/gc_runtime_root_holders.json
The runtime adds js_ffi_arg_ptr, which copies SSO bytes into per-thread scratch headers and returns their addresses. Codegen declares and wraps the helper. The crypto runtime delegates argument-pointer conversion to it, and the GC-holder census records the scratch ring as non-GC storage.
Crypto codegen argument conversion
crates/perry-codegen/src/expr/calls.rs, crates/perry-codegen/src/expr/calls/crypto_hash.rs, crates/perry-codegen/src/expr/calls/crypto_kdf.rs, crates/perry-codegen/src/expr/calls/crypto_keys.rs, crates/perry-codegen/src/expr/calls/crypto_misc.rs
Crypto call generation uses unbox_ffi_str_arg instead of unbox_to_i64 for the listed string arguments. Argument counts, native calls, and return handling remain unchanged.
Crypto runtime handling and regression test
crates/perry-stdlib/src/crypto/cipher.rs, crates/perry-stdlib/src/crypto/ecdh.rs, crates/perry-stdlib/src/crypto/hash_handles.rs, test-files/test_gap_11430_crypto_short_string_args.ts, changelog.d/11486-crypto-sso-args.md
Crypto runtime paths use arg_ptr instead of masking NaN-box bits directly for affected inputs. The regression test covers short-string arguments in hash, HMAC, KDF, and cipher calls. The changelog records the change.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to fbc46

The inspected short-string crypto paths do not show a remaining issue that should block merging. Proceed with normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to fbc46

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
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — Untrusted crypto string arguments can reach native pointer reads in a compiled process. The reviewed change affects that process-local boundary; the supplied evidence does not establish a new tenant, service, credential, or deployment boundary.

Trust Boundaries and Controls

  • observed — The reviewed hash creation path validates its algorithm as a string before conversion. The native byte reader copies the pointed-to bytes into owned memory rather than returning a borrowed slice.

Resilience and Maintainability Implications

  • inferred — Thread-local storage prevents another thread from reusing a slot, but does not protect a retained pointer from same-thread wraparound. The inspected consumers' immediate copies are the strongest counterevidence to a currently reachable overwrite path.

Hardening Proposals

  • proposed — Make the copy-before-retention contract explicit for native consumers, and exercise repeated or reentrant conversions beyond one ring cycle when validating it.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the codegen and runtime fix and the resolved SSO string crash in crypto calls. It is specific and related to the main change, although somewhat long.
Description check ✅ Passed The description is detailed and covers the problem, implementation, affected areas, related issues, test evidence, checks run, checks not run, and scope limits. It does not use the exact template head…
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 15 files. (2 skipped: 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug
proggeramlug force-pushed the fix-11430-crypto-sso-args branch from fbc4614 to 19d2296 Compare September 27, 2026 09:38
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Merge queue: CI was green. #11485 landed and conflicted in scripts/gc_runtime_root_holders.json, where both PRs added a new entry (main's CONSOLE_CHANNEL_IDS, this PR's FFI_SSO_RING). I kept both, and the gc_runtime_root_holders, thread_exit_address_globals and thread-local gates plus fmt pass on the rebase. Force-merging per the rebased-green rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant