buffer: reject swaps on immutable arraybuffers

lib/buffer.js

@@ -1374,7 +1374,16 @@ Buffer.prototype.swap16 = function swap16() {
       swap(this, i, i + 1);
     return this;
   }
-  _swap16(this);
+  const buffer = TypedArrayPrototypeGetBuffer(this);
+  const byteOffset = TypedArrayPrototypeGetByteOffset(this);
+  const byteLength = TypedArrayPrototypeGetByteLength(this);
+  // A length-tracking shared view may have grown since the length check above.
+  if (byteLength % 2 !== 0)
+    throw new ERR_INVALID_BUFFER_SIZE('16-bits');
+  if (!_swap16(buffer, byteOffset, byteLength)) {
+    throw new ERR_INVALID_ARG_VALUE(
+      'buffer', this, 'must not be backed by an immutable ArrayBuffer');
+  }
   return this;
 };
 
@@ -1393,7 +1402,15 @@ Buffer.prototype.swap32 = function swap32() {
     }
     return this;
   }
-  _swap32(this);
+  const buffer = TypedArrayPrototypeGetBuffer(this);
+  const byteOffset = TypedArrayPrototypeGetByteOffset(this);
+  const byteLength = TypedArrayPrototypeGetByteLength(this);
+  if (byteLength % 4 !== 0)
+    throw new ERR_INVALID_BUFFER_SIZE('32-bits');
+  if (!_swap32(buffer, byteOffset, byteLength)) {
+    throw new ERR_INVALID_ARG_VALUE(
+      'buffer', this, 'must not be backed by an immutable ArrayBuffer');
+  }
   return this;
 };
 
@@ -1416,7 +1433,15 @@ Buffer.prototype.swap64 = function swap64() {
     }
     return this;
   }
-  _swap64(this);
+  const buffer = TypedArrayPrototypeGetBuffer(this);
+  const byteOffset = TypedArrayPrototypeGetByteOffset(this);
+  const byteLength = TypedArrayPrototypeGetByteLength(this);
+  if (byteLength % 8 !== 0)
+    throw new ERR_INVALID_BUFFER_SIZE('64-bits');
+  if (!_swap64(buffer, byteOffset, byteLength)) {
+    throw new ERR_INVALID_ARG_VALUE(
+      'buffer', this, 'must not be backed by an immutable ArrayBuffer');
+  }
   return this;
 };
 

src/node_buffer.cc

@@ -39,6 +39,7 @@
 #include <stdint.h>
 #include <climits>
 #include <cstring>
+#include <type_traits>
 #include "nbytes.h"
 
 #define THROW_AND_RETURN_UNLESS_BUFFER(env, obj)                            \
@@ -1299,56 +1300,87 @@ int64_t FastIndexOfNumber(Local<Value>,
 
 static CFunction fast_index_of_number(CFunction::Make(FastIndexOfNumber));
 
+using FastSwapSize =
+    std::conditional_t<sizeof(size_t) == sizeof(uint64_t), uint64_t, uint32_t>;
+
+// Materialize backing buffers in JS to keep allocation out of value-returning
+// fast API calls.
+template <bool (*swap_bytes)(char*, size_t), typename T>
+V8_INLINE bool SwapBytes(Local<Value> buffer_obj,
+                         T byte_offset,
+                         T byte_length) {
+  size_t buffer_length;
+  char* data;
+  if (buffer_obj->IsArrayBuffer()) {
+    Local<ArrayBuffer> buffer = buffer_obj.As<ArrayBuffer>();
+    if (buffer->IsImmutable()) return false;
+    buffer_length = buffer->ByteLength();
+    data = static_cast<char*>(buffer->Data());
+  } else {
+    CHECK(buffer_obj->IsSharedArrayBuffer());
+    Local<SharedArrayBuffer> buffer = buffer_obj.As<SharedArrayBuffer>();
+    buffer_length = buffer->ByteLength();
+    data = static_cast<char*>(buffer->Data());
+  }
+  CHECK_GE(byte_offset, 0);
+  CHECK_LE(byte_offset, buffer_length);
+  CHECK_GE(byte_length, 0);
+  CHECK_LE(byte_length, buffer_length - byte_offset);
+  const size_t offset = static_cast<size_t>(byte_offset);
+  const size_t length = static_cast<size_t>(byte_length);
+  if (length == 0) return true;
+  CHECK_NOT_NULL(data);
+  CHECK(swap_bytes(data + offset, length));
+  return true;
+}
+
 void Swap16(const FunctionCallbackInfo<Value>& args) {
-  DCHECK(args[0]->IsArrayBufferView());
-  SPREAD_BUFFER_ARG(args[0], ts_obj);
-  CHECK(nbytes::SwapBytes16(ts_obj_data, ts_obj_length));
+  CHECK(args[1]->IsNumber());
+  CHECK(args[2]->IsNumber());
+  args.GetReturnValue().Set(SwapBytes<nbytes::SwapBytes16>(
+      args[0], args[1].As<Number>()->Value(), args[2].As<Number>()->Value()));
 }
 
-void FastSwap16(Local<Value> receiver,
+bool FastSwap16(Local<Value> receiver,
                 Local<Value> buffer_obj,
-                // NOLINTNEXTLINE(runtime/references)
-                FastApiCallbackOptions& options) {
+                FastSwapSize byte_offset,
+                FastSwapSize byte_length) {
   TRACK_V8_FAST_API_CALL("buffer.swap16");
-  HandleScope scope(options.isolate);
-  SPREAD_BUFFER_ARG(buffer_obj, ts_obj);
-  CHECK(nbytes::SwapBytes16(ts_obj_data, ts_obj_length));
+  return SwapBytes<nbytes::SwapBytes16>(buffer_obj, byte_offset, byte_length);
 }
 
 static CFunction fast_swap16(CFunction::Make(FastSwap16));
 
 void Swap32(const FunctionCallbackInfo<Value>& args) {
-  DCHECK(args[0]->IsArrayBufferView());
-  SPREAD_BUFFER_ARG(args[0], ts_obj);
-  CHECK(nbytes::SwapBytes32(ts_obj_data, ts_obj_length));
+  CHECK(args[1]->IsNumber());
+  CHECK(args[2]->IsNumber());
+  args.GetReturnValue().Set(SwapBytes<nbytes::SwapBytes32>(
+      args[0], args[1].As<Number>()->Value(), args[2].As<Number>()->Value()));
 }
 
-void FastSwap32(Local<Value> receiver,
+bool FastSwap32(Local<Value> receiver,
                 Local<Value> buffer_obj,
-                // NOLINTNEXTLINE(runtime/references)
-                FastApiCallbackOptions& options) {
+                FastSwapSize byte_offset,
+                FastSwapSize byte_length) {
   TRACK_V8_FAST_API_CALL("buffer.swap32");
-  HandleScope scope(options.isolate);
-  SPREAD_BUFFER_ARG(buffer_obj, ts_obj);
-  CHECK(nbytes::SwapBytes32(ts_obj_data, ts_obj_length));
+  return SwapBytes<nbytes::SwapBytes32>(buffer_obj, byte_offset, byte_length);
 }
 
 static CFunction fast_swap32(CFunction::Make(FastSwap32));
 
 void Swap64(const FunctionCallbackInfo<Value>& args) {
-  DCHECK(args[0]->IsArrayBufferView());
-  SPREAD_BUFFER_ARG(args[0], ts_obj);
-  CHECK(nbytes::SwapBytes64(ts_obj_data, ts_obj_length));
+  CHECK(args[1]->IsNumber());
+  CHECK(args[2]->IsNumber());
+  args.GetReturnValue().Set(SwapBytes<nbytes::SwapBytes64>(
+      args[0], args[1].As<Number>()->Value(), args[2].As<Number>()->Value()));
 }
 
-void FastSwap64(Local<Value> receiver,
+bool FastSwap64(Local<Value> receiver,
                 Local<Value> buffer_obj,
-                // NOLINTNEXTLINE(runtime/references)
-                FastApiCallbackOptions& options) {
+                FastSwapSize byte_offset,
+                FastSwapSize byte_length) {
   TRACK_V8_FAST_API_CALL("buffer.swap64");
-  HandleScope scope(options.isolate);
-  SPREAD_BUFFER_ARG(buffer_obj, ts_obj);
-  CHECK(nbytes::SwapBytes64(ts_obj_data, ts_obj_length));
+  return SwapBytes<nbytes::SwapBytes64>(buffer_obj, byte_offset, byte_length);
 }
 
 static CFunction fast_swap64(CFunction::Make(FastSwap64));

test/parallel/test-buffer-swap-immutable.js

@@ -0,0 +1,153 @@
+// Flags: --js-immutable-arraybuffer --allow-natives-syntax --expose-internals --no-warnings
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const { internalBinding } = require('internal/test/binding');
+const binding = internalBinding('buffer');
+const { arrayBufferViewHasBuffer } = internalBinding('util');
+
+if (typeof ArrayBuffer.prototype.transferToImmutable !== 'function')
+  common.skip('ArrayBuffer.prototype.transferToImmutable is not available');
+
+let debug;
+if (common.isDebug) {
+  debug = internalBinding('debug');
+}
+
+function assertImmutableError(fn, native = false) {
+  // Constructing the error must not invoke an inherited code setter.
+  let setterCalls = 0;
+  Object.defineProperty(Error.prototype, 'code', {
+    configurable: true,
+    get() { return undefined; },
+    set() { setterCalls++; },
+  });
+  try {
+    assert.throws(fn, native ? {
+      name: 'TypeError',
+      code: 'ERR_INVALID_ARG_VALUE',
+    } : TypeError);
+  } finally {
+    delete Error.prototype.code;
+  }
+  assert.strictEqual(setterCalls, 0);
+}
+
+for (const [method, swapped] of [
+  ['swap16', [2, 1, 4, 3, 6, 5, 8, 7]],
+  ['swap32', [4, 3, 2, 1, 8, 7, 6, 5]],
+  ['swap64', [8, 7, 6, 5, 4, 3, 2, 1]],
+]) {
+  const expected64 = Array.from({ length: 64 },
+                                (_, i) => swapped[i % 8] + 8 * Math.floor(i / 8));
+
+  // Exercise both the JavaScript and native paths, including unaligned views.
+  for (const length of [8, 64]) {
+    const bytes = Array.from({ length }, (_, i) => i + 1);
+    for (const offset of [0, 1]) {
+      const original = offset === 0 ? bytes : [0xaa, ...bytes, 0xbb];
+      const ab = Uint8Array.from(original).buffer.transferToImmutable();
+      const target = Buffer.from(ab, offset, length);
+      assertImmutableError(() => target[method](), length === 64);
+      assert.deepStrictEqual([...new Uint8Array(ab)], original);
+    }
+
+    const mutable = Buffer.from([0xaa, ...bytes, 0xbb]);
+    const target = mutable.subarray(1, length + 1);
+    for (const name of ['buffer', 'byteOffset', 'byteLength']) {
+      Object.defineProperty(target, name, { get: common.mustNotCall() });
+    }
+    assert.strictEqual(target[method](), target);
+    assert.deepStrictEqual([...mutable],
+                           [0xaa, ...expected64.slice(0, length), 0xbb]);
+  }
+
+  // Invalid sizes take precedence over immutable backing stores.
+  for (const length of [7, 65]) {
+    const bytes = Array.from({ length }, (_, i) => i);
+    const ab = Uint8Array.from(bytes).buffer.transferToImmutable();
+    assert.throws(() => Buffer.from(ab)[method](), {
+      code: 'ERR_INVALID_BUFFER_SIZE',
+    });
+    assert.deepStrictEqual([...new Uint8Array(ab)], bytes);
+  }
+
+  // Empty buffers and empty views do not write to their backing store.
+  {
+    const empty = Buffer.from(new ArrayBuffer(0).transferToImmutable());
+    assert.strictEqual(empty[method](), empty);
+    const ab = Uint8Array.from([1, 2, 3]).buffer.transferToImmutable();
+    const view = Buffer.from(ab, 1, 0);
+    assert.strictEqual(view[method](), view);
+    assert.deepStrictEqual([...new Uint8Array(ab)], [1, 2, 3]);
+  }
+
+  // The binding returns success, or false without touching immutable bytes.
+  {
+    const bytes = Array.from({ length: 66 }, (_, i) => i);
+    for (const backing of [new ArrayBuffer(66), new SharedArrayBuffer(66)]) {
+      new Uint8Array(backing).set(bytes);
+      assert.strictEqual(binding[method](backing, 1, 64), true);
+      assert.deepStrictEqual([...new Uint8Array(backing)],
+                             [0, ...expected64, 65]);
+    }
+    const backing = Uint8Array.from(bytes).buffer.transferToImmutable();
+    assert.strictEqual(binding[method](backing, 1, 64), false);
+    assert.deepStrictEqual([...new Uint8Array(backing)], bytes);
+  }
+
+  // Shared and resized backing stores retain the view's offset and length.
+  for (const backing of [
+    new SharedArrayBuffer(66),
+    new ArrayBuffer(72, { maxByteLength: 80 }),
+  ]) {
+    const target = Buffer.from(backing, 1, 64);
+    if (backing.resizable) backing.resize(66);
+    new Uint8Array(backing).set(Array.from({ length: 66 }, (_, i) => i));
+    assert.strictEqual(target[method](), target);
+    assert.deepStrictEqual([...new Uint8Array(backing)],
+                           [0, ...expected64, 65]);
+  }
+
+  // A fresh on-heap TypedArray must materialize its backing ArrayBuffer.
+  {
+    const target = new Uint8Array(64);
+    for (let i = 0; i < target.length; i++) target[i] = i + 1;
+    assert.strictEqual(arrayBufferViewHasBuffer(target), false);
+    assert.strictEqual(Buffer.prototype[method].call(target), target);
+    assert.strictEqual(arrayBufferViewHasBuffer(target), true);
+    assert.deepStrictEqual([...target], expected64);
+  }
+
+  // The optimized native path must reject immutable targets before writing.
+  {
+    const bytes = Array.from({ length: 66 }, (_, i) => i);
+    const ab = Uint8Array.from(bytes).buffer.transferToImmutable();
+    const target = Buffer.from(ab, 1, 64);
+    eval('%PrepareFunctionForOptimization(Buffer.prototype[method])');
+    Buffer.alloc(64)[method]();
+    const fastCallsBefore = debug?.getV8FastApiCallCount(`buffer.${method}`);
+    eval('%OptimizeFunctionOnNextCall(Buffer.prototype[method])');
+    assertImmutableError(() => target[method](), true);
+    if (debug) {
+      assert.strictEqual(debug.getV8FastApiCallCount(`buffer.${method}`),
+                         fastCallsBefore + 1);
+    }
+    assert.deepStrictEqual([...new Uint8Array(ab)], bytes);
+
+    // Materializing a fresh receiver must also happen before the fast call.
+    const fresh = new Uint8Array(64);
+    for (let i = 0; i < fresh.length; i++) fresh[i] = i + 1;
+    assert.strictEqual(arrayBufferViewHasBuffer(fresh), false);
+    const freshFastCallsBefore = debug?.getV8FastApiCallCount(`buffer.${method}`);
+    eval('%OptimizeFunctionOnNextCall(Buffer.prototype[method])');
+    assert.strictEqual(Buffer.prototype[method].call(fresh), fresh);
+    assert.strictEqual(arrayBufferViewHasBuffer(fresh), true);
+    if (debug) {
+      assert.strictEqual(debug.getV8FastApiCallCount(`buffer.${method}`),
+                         freshFastCallsBefore + 1);
+    }
+    assert.deepStrictEqual([...fresh], expected64);
+  }
+}

typings/internalBinding/buffer.d.ts

@@ -16,9 +16,9 @@ export interface BufferBinding {
 
   copyArrayBuffer(destination: ArrayBuffer | SharedArrayBuffer, destinationOffset: number, source: ArrayBuffer | SharedArrayBuffer, sourceOffset: number, bytesToCopy: number): void;
 
-  swap16(buf: ArrayBufferView): ArrayBufferView;
-  swap32(buf: ArrayBufferView): ArrayBufferView;
-  swap64(buf: ArrayBufferView): ArrayBufferView;
+  swap16(buffer: ArrayBuffer | SharedArrayBuffer, byteOffset: number, byteLength: number): boolean;
+  swap32(buffer: ArrayBuffer | SharedArrayBuffer, byteOffset: number, byteLength: number): boolean;
+  swap64(buffer: ArrayBuffer | SharedArrayBuffer, byteOffset: number, byteLength: number): boolean;
 
   isUtf8(input: ArrayBufferView | ArrayBuffer | SharedArrayBuffer): boolean;
   isAscii(input: ArrayBufferView | ArrayBuffer | SharedArrayBuffer): boolean;