quic: copy arraybuffers/views instead of detach

Author: jasnell

doc/api/quic.md

@@ -798,11 +798,12 @@ the datagram ID.
 
 If `datagram` is a string, it will be encoded using the specified `encoding`.
 
-If `datagram` is an `ArrayBufferView`, the underlying `ArrayBuffer` will be
-transferred if possible (taking ownership to prevent mutation after send).
-If the buffer is not transferable (e.g., a `SharedArrayBuffer` or a view
-over a subset of a larger buffer such as a pooled `Buffer`), the data will
-be copied instead.
+If `datagram` is an `ArrayBufferView`, the bytes are copied into an
+internal buffer; the caller's source buffer is unchanged and may be reused
+or mutated immediately after the call returns. Callers that want to ensure
+their source cannot be mutated after the call (for example, when handing
+the buffer off to another async consumer) can call
+`ArrayBuffer.prototype.transfer()` themselves before passing the buffer.
 
 If `datagram` is a `Promise`, it will be awaited before sending. If the
 session closes while awaiting, `0n` is returned silently (datagrams are
@@ -1644,6 +1645,13 @@ The Writer has the following methods:
   the readable side via `STOP_SENDING`.
 * `desiredSize` — Available capacity in bytes, or `null` if closed/errored.
 
+The bytes from each `writeSync()` / `writevSync()` / `write()` / `writev()`
+input chunk are copied into an internal buffer, so the caller's source
+buffer is unchanged and may be reused or mutated immediately after the
+call returns. Callers that want to ensure a source buffer cannot be
+mutated after handing it off can call `ArrayBuffer.prototype.transfer()`
+themselves before passing the buffer.
+
 ### `stream.setBody(body)`
 
 <!-- YAML
@@ -1661,8 +1669,11 @@ The following body source types are supported:
 * `null` — The writable side is closed immediately (FIN sent with no data).
 * `string` — UTF-8 encoded and sent as a single chunk.
 * `ArrayBuffer`, `SharedArrayBuffer`, `ArrayBufferView` — Sent as a single
-  chunk. `ArrayBuffer` and `ArrayBufferView` are detached (zero-copy
-  transfer) when possible; `SharedArrayBuffer` is always copied.
+  chunk. The bytes are copied into an internal buffer, so the caller's
+  source buffer is unchanged and may be reused or mutated immediately
+  after the call returns. Callers wanting to ensure their source cannot
+  be mutated after handing it off can call
+  `ArrayBuffer.prototype.transfer()` themselves before passing the buffer.
 * `Blob` — Sent from the Blob's underlying data queue.
 * {FileHandle} — The file contents are read asynchronously via an
   fd-backed data source. The `FileHandle` must be opened for reading

lib/internal/quic/quic.js

@@ -5,14 +5,10 @@
 /* c8 ignore start */
 
 const {
-  ArrayBufferPrototypeGetByteLength,
-  ArrayBufferPrototypeTransfer,
   ArrayIsArray,
   ArrayPrototypePush,
   BigInt,
-  DataViewPrototypeGetBuffer,
   DataViewPrototypeGetByteLength,
-  DataViewPrototypeGetByteOffset,
   FunctionPrototypeBind,
   Number,
   ObjectDefineProperties,
@@ -26,10 +22,7 @@ const {
   SymbolAsyncIterator,
   SymbolDispose,
   SymbolIterator,
-  TypedArrayPrototypeGetBuffer,
   TypedArrayPrototypeGetByteLength,
-  TypedArrayPrototypeGetByteOffset,
-  TypedArrayPrototypeSlice,
   Uint8Array,
 } = primordials;
 
@@ -1073,53 +1066,24 @@ function invokeOnerror(fn, error) {
 
 function validateBody(body) {
   if (body === undefined) return body;
-  // Transfer ArrayBuffers...
-  if (isArrayBuffer(body)) {
-    return ArrayBufferPrototypeTransfer(body);
-  }
-  // With a SharedArrayBuffer, we always copy. We cannot transfer
-  // and it's likely unsafe to use the underlying buffer directly.
-  if (isSharedArrayBuffer(body)) {
-    return TypedArrayPrototypeSlice(new Uint8Array(body));
-  }
-  if (isArrayBufferView(body)) {
-    let size, offset, buffer;
-    if (isDataView(body)) {
-      size = DataViewPrototypeGetByteLength(body);
-      offset = DataViewPrototypeGetByteOffset(body);
-      buffer = DataViewPrototypeGetBuffer(body);
-    } else {
-      size = TypedArrayPrototypeGetByteLength(body);
-      offset = TypedArrayPrototypeGetByteOffset(body);
-      buffer = TypedArrayPrototypeGetBuffer(body);
-    }
-    // We have to be careful in this case. If the ArrayBufferView is a
-    // subset of the underlying ArrayBuffer, transferring the entire
-    // ArrayBuffer could be incorrect if other views are also using it.
-    // So if offset > 0 or size != buffer.byteLength, we'll copy the
-    // subset into a new ArrayBuffer instead of transferring.
-    if (isSharedArrayBuffer(buffer) ||
-        offset !== 0 ||
-        size !== ArrayBufferPrototypeGetByteLength(buffer)) {
-      return TypedArrayPrototypeSlice(
-        new Uint8Array(buffer, offset, size));
-    }
-    // It's still possible that the ArrayBuffer is being used elsewhere,
-    // but we really have no way of knowing. We'll just have to trust
-    // the caller in this case.
-    return new Uint8Array(
-      ArrayBufferPrototypeTransfer(buffer), offset, size);
+  // ArrayBuffers, SharedArrayBuffers, and ArrayBufferViews are passed
+  // through to the C++ layer which copies the bytes into its own
+  // BackingStore. Callers can therefore safely reuse or mutate their
+  // input buffers after the call returns. Callers that want to ensure
+  // their buffer cannot be mutated after handing it off (for example,
+  // when sharing the source with another async consumer) can call
+  // ArrayBuffer.prototype.transfer() themselves before passing the
+  // buffer.
+  if (isArrayBuffer(body) ||
+      isSharedArrayBuffer(body) ||
+      isArrayBufferView(body)) {
+    return body;
   }
   if (isBlob(body)) return body[kBlobHandle];
 
   // Strings are encoded as UTF-8.
   if (typeof body === 'string') {
-    body = Buffer.from(body, 'utf8');
-    // Fall through -- Buffer is an ArrayBufferView, handled above.
-    return TypedArrayPrototypeSlice(new Uint8Array(
-      TypedArrayPrototypeGetBuffer(body),
-      TypedArrayPrototypeGetByteOffset(body),
-      TypedArrayPrototypeGetByteLength(body)));
+    return Buffer.from(body, 'utf8');
   }
 
   // FileHandle -- lock it and pass the C++ handle to GetDataQueueFromSource
@@ -1911,11 +1875,12 @@ class QuicStream {
     }
     // When destroying with an error, ensure the peer learns about
     // it via RESET_STREAM. The writer.fail path inside [kFinishClose]
-    // emits RESET_STREAM only when a writer was previously created;
-    // streams that destroyed without ever accessing stream.writer
-    // (e.g. used setBody or never wrote at all) previously left their
-    // write side dangling on the wire. The condition gates the
-    // emission to error-path destroys with a still-open writable side.
+    // emits RESET_STREAM only when a writer has been created;
+    // streams that destroy without ever accessing stream.writer
+    // (e.g. used setBody or never wrote at all) need an explicit
+    // RESET_STREAM here so the write side does not dangle on the
+    // wire. The condition gates the emission to error-path destroys
+    // with a still-open writable side.
     // Direction model for the writable side:
     //   * bidi: always has a writable side.
     //   * uni + #reader === undefined: locally-initiated, write-only.
@@ -3165,8 +3130,12 @@ class QuicSession {
    *
    * If a string is given it will be encoded using the specified encoding.
    *
-   * If an ArrayBufferView is given, the underlying ArrayBuffer will be
-   * transferred if possible, otherwise the data will be copied.
+   * If an ArrayBufferView is given, the bytes are copied into an internal
+   * buffer; the caller's source buffer is unchanged and may be reused
+   * immediately. Callers that want to ensure their source cannot be
+   * mutated after the call (for example, when handing the buffer off to
+   * another async consumer) can call ArrayBuffer.prototype.transfer()
+   * themselves before passing it.
    *
    * If a Promise is given, it will be awaited before sending. If the
    * session closes while awaiting, 0n is returned silently.
@@ -3179,7 +3148,6 @@ class QuicSession {
     if (this.#isClosedOrClosing) {
       throw new ERR_INVALID_STATE('Session is closed');
     }
-    let offset, length, buffer;
 
     const maxDatagramSize = this.#state.maxDatagramSize;
 
@@ -3196,45 +3164,21 @@ class QuicSession {
     }
 
     if (typeof datagram === 'string') {
-      // Buffer.from may return a pooled buffer that can't be transferred.
-      // Slice to get an independent copy.
       datagram = new Uint8Array(Buffer.from(datagram, encoding));
-      length = TypedArrayPrototypeGetByteLength(datagram);
-      if (length === 0) return kNilDatagramId;
-    } else {
-      if (!isArrayBufferView(datagram)) {
-        throw new ERR_INVALID_ARG_TYPE('datagram',
-                                       ['ArrayBufferView', 'string'],
-                                       datagram);
-      }
-      if (isDataView(datagram)) {
-        offset = DataViewPrototypeGetByteOffset(datagram);
-        length = DataViewPrototypeGetByteLength(datagram);
-        buffer = DataViewPrototypeGetBuffer(datagram);
-      } else {
-        offset = TypedArrayPrototypeGetByteOffset(datagram);
-        length = TypedArrayPrototypeGetByteLength(datagram);
-        buffer = TypedArrayPrototypeGetBuffer(datagram);
-      }
-
-      // If the view has zero length (e.g. detached buffer), there's
-      // nothing to send.
-      if (length === 0) return kNilDatagramId;
-
-      if (isSharedArrayBuffer(buffer) ||
-          offset !== 0 ||
-          length !== ArrayBufferPrototypeGetByteLength(buffer)) {
-        // Copy if the buffer is not transferable (SharedArrayBuffer)
-        // or if the view is over a subset of the buffer (e.g. a
-        // Node.js Buffer from the pool).
-        datagram = TypedArrayPrototypeSlice(
-          new Uint8Array(buffer), offset, offset + length);
-      } else {
-        datagram = new Uint8Array(
-          ArrayBufferPrototypeTransfer(buffer), offset, length);
-      }
+    } else if (!isArrayBufferView(datagram)) {
+      throw new ERR_INVALID_ARG_TYPE('datagram',
+                                     ['ArrayBufferView', 'string'],
+                                     datagram);
     }
 
+    const length = isDataView(datagram) ?
+      DataViewPrototypeGetByteLength(datagram) :
+      TypedArrayPrototypeGetByteLength(datagram);
+
+    // If the view has zero length (e.g. detached buffer), there's
+    // nothing to send.
+    if (length === 0) return kNilDatagramId;
+
     // The peer max datagram size is less than the datagram we want to send,
     // so... don't send it.
     if (length > maxDatagramSize) return kNilDatagramId;

src/quic/data.cc

@@ -88,31 +88,45 @@ Store::Store(std::unique_ptr<BackingStore> store, size_t length, size_t offset)
   CHECK_LE(length_, store_->ByteLength() - offset_);
 }
 
-Maybe<Store> Store::From(Local<ArrayBuffer> buffer, Local<Value> detach_key) {
-  if (!buffer->IsDetachable()) {
-    return Nothing<Store>();
-  }
-  bool res;
-  auto backing = buffer->GetBackingStore();
+Maybe<Store> Store::From(Local<ArrayBuffer> buffer) {
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  Environment* env = Environment::GetCurrent(isolate->GetCurrentContext());
   auto length = buffer->ByteLength();
-  if (!buffer->Detach(detach_key).To(&res) || !res) {
+  auto dest = ArrayBuffer::NewBackingStore(
+      isolate,
+      length,
+      v8::BackingStoreInitializationMode::kUninitialized,
+      v8::BackingStoreOnFailureMode::kReturnNull);
+  if (!dest) {
+    THROW_ERR_MEMORY_ALLOCATION_FAILED(env);
     return Nothing<Store>();
   }
-  return Just(Store(std::move(backing), length, 0));
+  if (length > 0) {
+    memcpy(dest->Data(), buffer->Data(), length);
+  }
+  return Just(Store(std::move(dest), length, 0));
 }
 
-Maybe<Store> Store::From(Local<ArrayBufferView> view, Local<Value> detach_key) {
-  if (!view->Buffer()->IsDetachable()) {
-    return Nothing<Store>();
-  }
-  bool res;
-  auto backing = view->Buffer()->GetBackingStore();
+Maybe<Store> Store::From(Local<ArrayBufferView> view) {
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  Environment* env = Environment::GetCurrent(isolate->GetCurrentContext());
   auto length = view->ByteLength();
   auto offset = view->ByteOffset();
-  if (!view->Buffer()->Detach(detach_key).To(&res) || !res) {
+  auto dest = ArrayBuffer::NewBackingStore(
+      isolate,
+      length,
+      v8::BackingStoreInitializationMode::kUninitialized,
+      v8::BackingStoreOnFailureMode::kReturnNull);
+  if (!dest) {
+    THROW_ERR_MEMORY_ALLOCATION_FAILED(env);
     return Nothing<Store>();
   }
-  return Just(Store(std::move(backing), length, offset));
+  if (length > 0) {
+    memcpy(dest->Data(),
+           static_cast<const uint8_t*>(view->Buffer()->Data()) + offset,
+           length);
+  }
+  return Just(Store(std::move(dest), length, 0));
 }
 
 Store Store::CopyFrom(Local<ArrayBuffer> buffer) {

src/quic/data.h

@@ -54,28 +54,28 @@ class Store final : public MemoryRetainer {
         size_t length,
         size_t offset = 0);
 
-  // Creates a Store from the contents of an ArrayBuffer, always detaching
-  // it in the process. An empty Maybe will be returned if the ArrayBuffer
-  // is not detachable or detaching failed (likely due to a detach key
-  // mismatch).
-  static v8::Maybe<Store> From(
-      v8::Local<v8::ArrayBuffer> buffer,
-      v8::Local<v8::Value> detach_key = v8::Local<v8::Value>());
-
-  // Creates a Store from the contents of an ArrayBufferView, always detaching
-  // it in the process. An empty Maybe will be returned if the ArrayBuffer
-  // is not detachable or detaching failed (likely due to a detach key
-  // mismatch).
-  static v8::Maybe<Store> From(
-      v8::Local<v8::ArrayBufferView> view,
-      v8::Local<v8::Value> detach_key = v8::Local<v8::Value>());
+  // Creates a Store by copying the contents of an ArrayBuffer into a fresh
+  // BackingStore. The caller's buffer is not modified, so callers can safely
+  // reuse or mutate it after the call returns. Returns an empty Maybe on
+  // allocation failure, in which case an `ERR_MEMORY_ALLOCATION_FAILED`
+  // exception will have been scheduled on the isolate.
+  static v8::Maybe<Store> From(v8::Local<v8::ArrayBuffer> buffer);
+
+  // Creates a Store by copying the contents of an ArrayBufferView into a
+  // fresh BackingStore. The caller's view (and its underlying ArrayBuffer)
+  // is not modified. Returns an empty Maybe on allocation failure, in which
+  // case an `ERR_MEMORY_ALLOCATION_FAILED` exception will have been
+  // scheduled on the isolate.
+  static v8::Maybe<Store> From(v8::Local<v8::ArrayBufferView> view);
 
   // Creates a Store from the contents of an ArrayBuffer, always copying the
-  // content.
+  // content. Equivalent to `From()` but returns a Store directly without
+  // surfacing allocation failure.
   static Store CopyFrom(v8::Local<v8::ArrayBuffer> buffer);
 
-  // Creates a Store from the contents of an ArrayBufferView, always copying the
-  // content.
+  // Creates a Store from the contents of an ArrayBufferView, always copying
+  // the content. Equivalent to `From()` but returns a Store directly without
+  // surfacing allocation failure.
   static Store CopyFrom(v8::Local<v8::ArrayBufferView> view);
 
   v8::Local<v8::Uint8Array> ToUint8Array(Environment* env) const;

src/quic/streams.cc

@@ -154,31 +154,23 @@ STAT_STRUCT(Stream, STREAM)
 // ============================================================================
 
 namespace {
-// Creates an in-memory DataQueue entry from an ArrayBuffer by either
-// detaching it (zero-copy) or copying its contents if detach is not
-// possible (e.g., SharedArrayBuffer-backed or non-detachable).
-// Returns nullptr on failure (error already thrown if allocation failed).
+// Creates an in-memory DataQueue entry by copying the requested range of
+// the given ArrayBuffer into a fresh BackingStore. The caller's buffer is
+// not detached or otherwise modified, so callers can safely reuse or
+// mutate it after the call returns. Callers that want to ensure their
+// buffer cannot be mutated after handing it off can call
+// `ArrayBuffer.prototype.transfer()` themselves before calling into the
+// QUIC API.
+// Returns nullptr on zero length or allocation failure.
 std::unique_ptr<DataQueue::Entry> CreateEntryFromBuffer(
     Environment* env, Local<ArrayBuffer> buffer, size_t offset, size_t length) {
   if (length == 0) return nullptr;
-  std::shared_ptr<BackingStore> backing;
-  if (buffer->IsDetachable()) {
-    backing = buffer->GetBackingStore();
-    if (buffer->Detach(Local<Value>()).IsNothing()) {
-      backing.reset();
-    }
-  }
-  if (!backing) {
-    // Buffer is not detachable or detach failed. Copy the data.
-    JS_TRY_ALLOCATE_BACKING_OR_RETURN(env, copy, length, nullptr);
-    memcpy(copy->Data(),
-           static_cast<const uint8_t*>(buffer->Data()) + offset,
-           length);
-    offset = 0;
-    backing = std::move(copy);
-  }
+  JS_TRY_ALLOCATE_BACKING_OR_RETURN(env, copy, length, nullptr);
+  memcpy(copy->Data(),
+         static_cast<const uint8_t*>(buffer->Data()) + offset,
+         length);
   return DataQueue::CreateInMemoryEntryFromBackingStore(
-      std::move(backing), offset, length);
+      std::move(copy), 0, length);
 }
 }  // namespace