ffi: reject detached ArrayBuffers as pointers

Project: nodejs/node | Language: JavaScript, C++ | Concepts: memory-management, error-handling, testing-strategy | Difficulty: mid
Link: https://github.com/nodejs/node/pull/65083

The problem

Detached ArrayBuffers and their views (such as TypedArray and DataView) could be passed to native FFI (Foreign Function Interface) calls. When this happened, the underlying memory was no longer valid, but Node.js would silently convert them to null pointers instead of throwing an error. This could lead to subtle bugs or unsafe behavior in native code expecting a valid pointer.

Why it's non-trivial

A naive fix might only check for detached buffers in one place, missing other paths (e.g., optimized fast calls, different argument types, or different FFI APIs). It's important to ensure that all ways of passing pointers through FFI are covered, and that errors are thrown consistently and early, both in JavaScript and in native code.

The approach

1. Centralized check in JavaScript argument conversion

A new helper function, getRawPointerArg, was introduced to check if an argument is a detached ArrayBuffer or a view backed by one:

function getRawPointerArg(value, index) {
  let buffer;
  let isView = false;
  if (isArrayBuffer(value)) {
    buffer = value;
  } else if (isArrayBufferView(value)) {
    isView = true;
    buffer = isDataView(value) ?
      DataViewPrototypeGetBuffer(value) :
      TypedArrayPrototypeGetBuffer(value);
  }

  if (buffer !== undefined && isArrayBuffer(buffer) &&
      ArrayBufferPrototypeGetDetached(buffer)) {
    throwFFIArgError(isView ?
      `Argument ${index} is an ArrayBufferView backed by a detached ArrayBuffer` :
      `Argument ${index} is a detached ArrayBuffer`);
  }

  return getRawPointer(value);
}

Annotation: This function checks if the value is an ArrayBuffer or a view. If so, it checks if the buffer is detached and throws a descriptive error if it is.

The FFI pointer argument conversion now uses this function:

-    return getRawPointer(value);
+    return getRawPointerArg(value, index);

Annotation: All pointer conversions now go through the detached check.

2. Checks in native code (C++)

In the C++ FFI implementation, similar checks were added:

For getRawPointer:

if (args[0]->IsArrayBuffer()) {
  Local<ArrayBuffer> buffer = args[0].As<ArrayBuffer>();
  if (buffer->WasDetached()) {
    THROW_ERR_INVALID_ARG_VALUE(env, "ArrayBuffer is detached");
    return;
  }
  store = buffer->GetBackingStore();
} else if (args[0]->IsArrayBufferView()) {
  Local<ArrayBufferView> view = args[0].As<ArrayBufferView>();
  if (view->Buffer()->WasDetached()) {
    THROW_ERR_INVALID_ARG_VALUE(
        env, "ArrayBufferView is backed by a detached ArrayBuffer");
    return;
  }
  store = view->Buffer()->GetBackingStore();
  offset = view->ByteOffset();
}

Annotation: Before accessing the backing store, the code checks if the buffer is detached and throws an error if so.

For fast FFI calls:

if (value->IsArrayBufferView()) {
  v8::Local<v8::ArrayBufferView> view = value.As<v8::ArrayBufferView>();
  if (view->Buffer()->WasDetached()) {
    // ...throw error...
    return kInvalidBuffer;
  }
  return PointerFromValue(value);
}
if (value->IsArrayBuffer()) {
  if (value.As<v8::ArrayBuffer>()->WasDetached()) {
    // ...throw error...
    return kInvalidBuffer;
  }
  return PointerFromValue(value);
}

Annotation: The fast path also checks for detachment and throws an error, returning an invalid pointer if so.

3. Improved and expanded tests

New and updated tests assert that passing a detached buffer or view throws the correct error:

const arrayBuffer = new ArrayBuffer(8);
const typedArray = new Uint8Array(arrayBuffer);
const dataView = new DataView(arrayBuffer);

arrayBuffer.transfer();

assert.throws(() => ffi.exportArrayBuffer(arrayBuffer, 0n, 0), {
  code: 'ERR_INVALID_ARG_VALUE',
  message: 'ArrayBuffer is detached',
});

for (const [value, rawPointerMessage, argumentMessage] of [
  [
    arrayBuffer,
    'ArrayBuffer is detached',
    'Argument 0 is a detached ArrayBuffer',
  ],
  ...[typedArray, dataView].map((view) => [
    view,
    'ArrayBufferView is backed by a detached ArrayBuffer',
    'Argument 0 is an ArrayBufferView backed by a detached ArrayBuffer',
  ]),
]) {
  assert.throws(() => ffi.getRawPointer(value), {
    code: 'ERR_INVALID_ARG_VALUE',
    message: rawPointerMessage,
  });
  assert.throws(() => symbols.pointer_to_usize(value), {
    code: 'ERR_INVALID_ARG_VALUE',
    message: argumentMessage,
  });
}

Annotation: The tests cover both direct and fast-path FFI calls, for both ArrayBuffer and its views.

From the review thread

No notable review discussion on this PR.

Takeaways

• Always validate memory object state before passing pointers to native code. Detached or invalid buffers should never be silently accepted, as this can lead to unsafe or unpredictable behavior.
• Centralize and reuse validation logic across all code paths. Implement checks in a single helper and ensure all argument conversions use it, reducing the risk of missing edge cases.
• Comprehensive tests should cover both expected and error cases, including all relevant data types and API paths. This ensures that fixes are robust and regressions are caught early.