add ArrayBuffer, ArrayBufferView, SharedArrayBuffer, and BackingStore to Rust JSG by anonrig · Pull Request #6398 · cloudflare/workerd

anonrig · 2026-03-24T15:34:00Z

Built on top of #6386 branch

ask-bonk

I'm Bonk, and I've done a quick review of your PR.

This PR adds ArrayBuffer, ArrayBufferView, SharedArrayBuffer, and BackingStore Rust bindings to the JSG layer, with extensive test coverage.

Issues

[High] SharedArrayBuffer::as_slice / as_mut_slice and BackingStore::as_mut_slice for shared backing stores are unsound — SharedArrayBuffer memory can be concurrently modified by other threads or by JS (via Atomics or typed array writes from another agent). Creating &[u8] / &mut [u8] from shared memory is undefined behavior in Rust because it violates the aliasing/provenance model. The &mut [u8] case is especially dangerous: Rust assumes exclusive access through &mut references. Even the &[u8] case is technically UB if another thread writes during the borrow. The doc comments mention this but "caller is responsible for synchronization" isn't enforceable. This may be acceptable as a pragmatic choice for now since workerd is single-threaded per-isolate, but the API should either (a) make these methods unsafe to signal the invariant, or (b) return raw pointers / use UnsafeCell semantics instead. Adding unsafe would be the minimal fix. See inline comment.
[Medium] new_uninit is misleading — the buffer IS zero-initialized — ArrayBuffer::new_uninit and SharedArrayBuffer::new_uninit call NewBackingStore(isolate, length) which zero-initializes the memory. The name new_uninit strongly implies the contents are uninitialized (as in MaybeUninit), but the test itself asserts the memory is zeroed. This is a naming footgun that could lead callers to skip initialization thinking it's redundant, or worse, treat the memory as potentially uninitialized. Consider new_zeroed or just with_byte_length. See inline comment.
[Low] Doc comment references non-existent method names — BackingStore doc says "Obtained via Local<ArrayBuffer>::get_backing_store" but the actual method is called backing_store() (no get_ prefix).

src/rust/jsg/v8.rs

ask-bonk · 2026-03-24T15:37:19Z

Review posted successfully. Here's a summary of the findings:

[High] Soundness issue with SharedArrayBuffer slice methods — as_slice()/as_mut_slice() on SharedArrayBuffer and BackingStore create Rust references (&[u8]/&mut [u8]) to potentially concurrently-accessed memory, which is UB under Rust's aliasing model. Suggested making these unsafe fn.
[Medium] new_uninit naming is misleading — The memory is actually zero-initialized by V8, but the name implies MaybeUninit semantics. Suggested renaming to new_zeroed or with_byte_length.
[Low] Doc comment typo — BackingStore docs reference get_backing_store but the method is actually backing_store().

github run

… to Rust JSG

jasnell · 2026-03-25T14:01:00Z

src/rust/jsg/ffi.c++

+
+size_t local_array_buffer_get_backing_store(Isolate* isolate, const Local& buffer) {
+  return reinterpret_cast<size_t>(
+      new BackingStorePtr(local_as_ref_from_ffi<v8::ArrayBuffer>(buffer)->GetBackingStore()));


The fact that this is heap allocating a std::shared_ptr gets lost behind the using alias. newing a std::shared_ptr is itself a bit of an unsafe antipattern and it's unfortunate that we're having to resort to that here, especially under the guise of making things safer by porting them to rust.

In nothing else tho, I'd drop the using alias and just use the std::shared_ptr<v8::BackingStore> directly just so it's clearer at a glance what is happening.

jasnell · 2026-03-25T14:03:22Z

src/rust/jsg/ffi.c++

+  auto* ptr = static_cast<uint8_t*>(ab->Data());
+  for (size_t i = 0; i < byteLen; i++) {
+    result.push_back(ptr[i]);
+  }


is there no memcpy equivalent? Does the rust compiler optimize this away? For large array buffers this O(n) loop is going to be slow.

jasnell · 2026-03-25T14:04:22Z

src/rust/jsg/ffi.c++

+    result.push_back(ptr[i]);
+  }
+  return result;
+}


There's a fair amount of duplication between these unwrap methods. Can they can condensed into one?

jasnell · 2026-03-25T14:06:55Z

src/rust/jsg/ffi.c++

+
+::rust::Vec<uint8_t> unwrap_array_buffer_view(Isolate* isolate, Local value) {
+  auto v8Val = local_from_ffi<v8::Value>(kj::mv(value));
+  KJ_REQUIRE(v8Val->IsArrayBufferView());


This will only work for Uint8Array, Uint8ClampedArray, and DataView. All the remaining TypedArray types are multi-byte, not to mention the Floating point and Bigint types that will have completely different intepretations.

jasnell · 2026-03-25T14:07:39Z

src/rust/jsg/ffi.c++

+}
+
+// Unwrappers
+::rust::Vec<uint8_t> unwrap_array_buffer(Isolate* isolate, Local value) {


This isn't really "unwrap". It's really "copy"

jasnell · 2026-03-25T14:12:48Z

src/rust/jsg/ffi.c++

+kj::Maybe<Local> array_buffer_maybe_new(
+    Isolate* isolate, size_t byte_length, BackingStoreInitializationMode mode) {
+  auto maybe = v8::ArrayBuffer::MaybeNew(
+      isolate, byte_length, static_cast<v8::BackingStoreInitializationMode>(mode));


We should actually drop our MaybeNew patch in favor of the newer version of v8::ArrayBuffer::New that accepts the flag telling it how to handle allocation failure. This MaybeNew is redundant now.

jasnell · 2026-03-25T14:15:18Z

src/rust/jsg/v8.rs

+// `ArrayBufferView`-specific implementations
+// =============================================================================
+
+impl<'a> Local<'a, ArrayBufferView> {


For streams use cases, this will need element size and whether the underlying view is an integer type or not.

jasnell · 2026-03-25T14:16:29Z

src/rust/jsg/v8.rs

+    pub unsafe fn buffer_data(&self) -> *mut u8 {
+        // SAFETY: handle is valid within the current HandleScope.
+        unsafe { ffi::local_array_buffer_view_buffer_data(self.isolate.as_ffi(), &self.handle) }
+    }


we'll need the ability to get a view of the elements, not just the bytes. If we can't do that reasonably in the ArrayBufferView, then we might need separate rust classes for each of the TypedArray kinds.

jasnell · 2026-03-25T14:17:46Z

src/rust/jsg/v8.rs

+}
+
+impl Local<'_, ArrayBuffer> {
+    /// Returns the byte length of this `ArrayBuffer`.


For streams use cases we'll need a detach / is_detached / is_detachable API.

jasnell · 2026-03-25T14:19:21Z

src/rust/jsg/v8.rs

+    }
+}
+
+impl Local<'_, SharedArrayBuffer> {


Honestly, I'm not sure there's enough value in this as a separate class given the duplication/overlap with ArrayBuffer. We could likely simplify and have ArrayBuffer encapsulate this and just add a is_shared API there.

guybedford · 2026-03-27T22:13:59Z

src/rust/jsg/v8.rs

+    /// # Safety
+    /// For `SharedArrayBuffer` backing stores (`is_shared() == true`) the
+    /// underlying memory may be concurrently read or written by other agents
+    /// (JS threads, `Atomics` operations). The caller must ensure no concurrent
+    /// writes occur for the duration of the borrow, or use atomic operations.


This is returning a non-mutable slice, so why does the safety not mention mutability?

guybedford · 2026-03-27T22:15:00Z

src/rust/jsg/v8.rs

+    ///
+    /// Zero-copy. The caller must ensure no other live slice aliases this region.
+    #[inline]
+    pub fn as_mut_slice(&mut self) -> &mut [u8] {


Why isn't this unsafe?

guybedford · 2026-03-27T22:15:36Z

src/rust/jsg/v8.rs

+
+    /// Returns a mutable byte slice view into the `ArrayBuffer`'s backing store.
+    ///
+    /// Zero-copy. The caller must ensure no other live slice aliases this region.


The caller must ensure no other live slice aliases this region.

This very much goes against the concept of singular ownership in Rust... do we not have any better guarantees ever?

guybedford · 2026-03-27T22:16:29Z

src/rust/jsg/v8.rs

+    ///
+    /// Zero-copy. The caller must ensure no other live slice aliases this region.
+    #[inline]
+    pub fn as_mut_slice(&mut self) -> &mut [u8] {


Again, it is unclear to me why BackingStore::as_mut_slice is unsafe, while ArrayBuffer::as_mut_slice is? Would be good to understand better the difference in ownership between these modes.

guybedford

Shouldn't we require a lock to obtain a mutable slice, so that the mutable slice is only valid for the duration of the lock? It seems this is necessary to properly support ownership semantics?

add several TypedArray methods to Rust JSG

8e79e53

anonrig requested review from guybedford and jasnell March 24, 2026 15:34

anonrig requested review from a team as code owners March 24, 2026 15:34

anonrig force-pushed the yagiz/arraybuffer-arraybufferview branch from 77dedb2 to 2ccf890 Compare March 24, 2026 15:36

ask-bonk bot reviewed Mar 24, 2026

View reviewed changes

src/rust/jsg/v8.rs Show resolved Hide resolved

src/rust/jsg/v8.rs Show resolved Hide resolved

src/rust/jsg/v8.rs Show resolved Hide resolved

src/rust/jsg/v8.rs Show resolved Hide resolved

add ArrayBuffer, ArrayBufferView, SharedArrayBuffer, and BackingStore…

22dbb60

… to Rust JSG

anonrig force-pushed the yagiz/arraybuffer-arraybufferview branch from 2ccf890 to 22dbb60 Compare March 24, 2026 16:05

jasnell reviewed Mar 25, 2026

View reviewed changes

anonrig force-pushed the yagiz/add-typed-array-methods branch 2 times, most recently from 9b527bb to d88cf6b Compare March 26, 2026 19:23

Base automatically changed from yagiz/add-typed-array-methods to main March 26, 2026 19:59

guybedford reviewed Mar 27, 2026

View reviewed changes

guybedford requested changes Mar 27, 2026

View reviewed changes

Conversation

anonrig commented Mar 24, 2026

Uh oh!

ask-bonk bot left a comment

Choose a reason for hiding this comment

Issues

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ask-bonk bot commented Mar 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jasnell Mar 25, 2026 •

edited

Loading