Skip to content

fix(node): preserve v8 deserializer view offsets - #36460

Merged
nathanwhit merged 2 commits into
denoland:mainfrom
nathanwhit:fix/v8-deserializer-view-offsets
Aug 17, 2026
Merged

fix(node): preserve v8 deserializer view offsets#36460
nathanwhit merged 2 commits into
denoland:mainfrom
nathanwhit:fix/v8-deserializer-view-offsets

Conversation

@nathanwhit

Copy link
Copy Markdown
Member

Summary

  • compute raw-byte positions relative to the supplied ArrayBufferView
  • reject failed or out-of-range raw-byte reads
  • apply the view offset consistently when constructing host-object views
  • cover nonzero-offset, truncated, and empty-input cases

Problem

The deserializer initialized V8 with bytes from the supplied view but calculated returned positions from the start of the underlying ArrayBuffer. The JavaScript layer then added the view offset again, so host objects deserialized from slices could use the wrong bytes. Failed raw-byte reads also returned zero, making them indistinguishable from a valid offset at the beginning of the view.

Validation

  • ./tools/format.js ext/node/ops/v8.rs ext/node/polyfills/v8.ts tests/unit_node/v8_test.ts
  • ./tools/lint.js --js ext/node/polyfills/v8.ts tests/unit_node/v8_test.ts
  • cargo check -p deno_node --features deno_core/v8
  • cargo build --bin deno
  • cargo build -p test_server
  • cargo test -p unit_node_tests --test unit_node v8_test -- --nocapture

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Confirmed the bug by reading the surrounding code. op_v8_new_deserializer built buf_slice from data.add(offset) but stored ptr: Some(data.cast()) β€” the backing-store base, not the view start. So op_v8_read_raw_bytes returned an offset measured from the ArrayBuffer, and the JS side then did getViewByteOffset(view) + byteOffset, adding the view offset a second time. Any host object deserialized from a view with a nonzero byteOffset read from the wrong place. NonNull::new(data_ptr) fixes the base, and the JS addition then becomes correct.

The second half is a separate real bug in the same function: the unaligned fallback did Buffer.from(getViewBuffer(view), byteOffset, byteLength) β€” indexing the whole ArrayBuffer with a view-relative offset. Switching it to offset lines it up with the aligned branch above.

The -1 sentinel is worth having on its own: returning 0 on a failed read_raw_bytes was indistinguishable from a legitimate read at offset 0, so a truncated payload silently deserialized from the start of the buffer instead of erroring. The checked_sub/checked_add/end > deser.buf.len chain and the zero-length-backing-store branch look right.

One nit: Node throws ERR_BUFFER_OUT_OF_BOUNDS here (node_serdes.cc: THROW_ERR_BUFFER_OUT_OF_BOUNDS(env, "ReadRawBytes() failed")). The message matches but the code doesn't, so err.code is undefined for anyone branching on it. Worth using the node error class. Not blocking. LGTM.

@nathanwhit
nathanwhit enabled auto-merge (squash) August 17, 2026 19:55
@nathanwhit
nathanwhit merged commit 8c76f75 into denoland:main Aug 17, 2026
136 checks passed
bartlomieju pushed a commit that referenced this pull request Aug 27, 2026
## Summary

- compute raw-byte positions relative to the supplied ArrayBufferView
- reject failed or out-of-range raw-byte reads
- apply the view offset consistently when constructing host-object views
- cover nonzero-offset, truncated, and empty-input cases

## Problem

The deserializer initialized V8 with bytes from the supplied view but
calculated returned positions from the start of the underlying
ArrayBuffer. The JavaScript layer then added the view offset again, so
host objects deserialized from slices could use the wrong bytes. Failed
raw-byte reads also returned zero, making them indistinguishable from a
valid offset at the beginning of the view.

## Validation

- ./tools/format.js ext/node/ops/v8.rs ext/node/polyfills/v8.ts
tests/unit_node/v8_test.ts
- ./tools/lint.js --js ext/node/polyfills/v8.ts
tests/unit_node/v8_test.ts
- cargo check -p deno_node --features deno_core/v8
- cargo build --bin deno
- cargo build -p test_server
- cargo test -p unit_node_tests --test unit_node v8_test -- --nocapture
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.

2 participants