Skip to content

node: snapshot resizable async StringOrBuffer inputs before worker handoff#31645

Open
EffortlessSteven wants to merge 1 commit into
oven-sh:mainfrom
EffortlessSteven:claude/crypto-scrypt-rab-input
Open

node: snapshot resizable async StringOrBuffer inputs before worker handoff#31645
EffortlessSteven wants to merge 1 commit into
oven-sh:mainfrom
EffortlessSteven:claude/crypto-scrypt-rab-input

Conversation

@EffortlessSteven

Copy link
Copy Markdown

Summary

Async crypto.scrypt captured its JS-backed password and salt before scheduling the worker. Resizing a resizable ArrayBuffer backing either input then leaves the worker with a stale pointer/length. Reading it is undefined behavior.

The shared async StringOrBuffer conversion now snapshots non-shared resizable inputs before handoff, keeping fixed buffers on the pinned fast path.

Changes:

  • snapshot non-shared resizable async StringOrBuffer inputs before worker handoff
  • use the same snapshot path for encoded and non-encoded async buffer inputs
  • keep fixed buffers on the existing pin/protect path
  • add async scrypt regressions for resized password, salt, and offset-view inputs

Test approach

Each regression schedules crypto.scrypt, resizes the backing ArrayBuffer to zero, and asserts the key matches scryptSync over the submitted bytes.

Verification

  • cargo fmt --all --check
  • git diff --check -- src/runtime/node/types.rs test/js/node/crypto/scrypt.test.ts
  • cargo clippy -p bun_runtime
  • bun bd -e "console.log('ok')"
  • bun bd test test/js/node/crypto/scrypt.test.ts
  • Regression confirmed: unpatched crashes under ASAN; patched derives the key from submitted bytes

Review map

  • src/runtime/node/types.rs: shared async StringOrBuffer helper snapshots non-shared resizable buffers before worker handoff
  • test/js/node/crypto/scrypt.test.ts: resized password, salt, and offset-view inputs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6748efa2-7fa8-432f-ae8c-d193b5351bd5

📥 Commits

Reviewing files that changed from the base of the PR and between 5ac120c and 0540c32.

📒 Files selected for processing (2)
  • src/runtime/node/types.rs
  • test/js/node/crypto/scrypt.test.ts

Walkthrough

This PR refactors array-buffer decoding in the Node runtime types to centralize and unify logic for handling resizable and fixed buffers in async contexts. A new StringOrBuffer::decode_array_buffer_into helper distinguishes resizable non-shared buffers (snapshot to owned bytes) from fixed buffers (protect JS value), removing duplication from two existing conversion methods. Tests verify async snapshotting preserves original bytes when buffers are resized post-submission.

Changes

Array-buffer decoding refactoring and async snapshotting tests

Layer / File(s) Summary
Async array-buffer snapshotting helper and integration
src/runtime/node/types.rs
New StringOrBuffer::decode_array_buffer_into helper routes async vs sync array-buffer conversion, snapshotting resizable non-shared buffers into owned bytes and protecting fixed buffers. Methods from_js_maybe_async_into and from_js_with_encoding_maybe_async_into delegate array-buffer-like branches to this helper.
Crypto.scrypt async snapshotting tests
test/js/node/crypto/scrypt.test.ts
Imports crypto module and adds three tests validating snapshotting for resizable-ArrayBuffer-backed password, password view with offset, and salt. Each test resizes the buffer to zero post-submission and verifies the derived key matches the synchronous result.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: snapshotting resizable async StringOrBuffer inputs before worker handoff, which is the core fix described in the PR.
Description check ✅ Passed The PR description covers both required template sections: 'What does this PR do' (comprehensive summary and changes) and 'How did you verify your code works' (detailed verification checklist and test approach).
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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 and usage tips.

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