src,lib: refactor unsafe buffer creation to remove zero-fill toggle

ChALkeR · RafaelGSS · marco-ippolito · commit 3cdb1cd437f6 · 2026-01-13T17:06:20.000+01:00
This removes the zero-fill toggle mechanism that allowed JavaScript to control ArrayBuffer initialization via shared memory. Instead, unsafe buffer creation now uses a dedicated C++ API. Refs: https://hackerone.com/reports/3405778 Co-Authored-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com> PR-URL: nodejs-private/node-private#759 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> CVE-ID: CVE-2025-55131
diff --git a/lib/internal/buffer.js b/lib/internal/buffer.js
@@ -30,7 +30,7 @@ const {
   hexWrite,
   ucs2Write,
   utf8WriteStatic,
-  getZeroFillToggle,
+  createUnsafeArrayBuffer,
 } = internalBinding('buffer');
 
 const {
@@ -39,13 +39,6 @@ const {
   },
 } = internalBinding('util');
 
-const {
-  namespace: {
-    isBuildingSnapshot,
-  },
-  addAfterUserSerializeCallback,
-} = require('internal/v8/startup_snapshot');
-
 // Temporary buffers to convert numbers.
 const float32Array = new Float32Array(1);
 const uInt8Float32Array = new Uint8Array(float32Array.buffer);
@@ -1086,28 +1079,14 @@ function isMarkedAsUntransferable(obj) {
   return obj[untransferable_object_private_symbol] !== undefined;
 }
 
-// A toggle used to access the zero fill setting of the array buffer allocator
-// in C++.
-// |zeroFill| can be undefined when running inside an isolate where we
-// do not own the ArrayBuffer allocator.  Zero fill is always on in that case.
-let zeroFill;
 function createUnsafeBuffer(size) {
-  if (!zeroFill) {
-    zeroFill = getZeroFillToggle();
-    if (isBuildingSnapshot()) {
-      // Reset the toggle so that after serialization, we'll re-create a real
-      // toggle connected to the C++ one via getZeroFillToggle().
-      addAfterUserSerializeCallback(() => {
-        zeroFill = undefined;
-      });
-    }
-  }
-  zeroFill[0] = 0;
-  try {
+  if (size <= 64) {
+    // Allocated in heap, doesn't call backing store anyway
+    // This is the same that the old impl did implicitly, but explicit now
     return new FastBuffer(size);
-  } finally {
-    zeroFill[0] = 1;
   }
+
+  return new FastBuffer(createUnsafeArrayBuffer(size));
 }
 
 module.exports = {
diff --git a/src/api/environment.cc b/src/api/environment.cc
@@ -113,13 +113,8 @@ MaybeLocal<Value> PrepareStackTraceCallback(Local<Context> context,
 
 void* NodeArrayBufferAllocator::Allocate(size_t size) {
   void* ret;
-  if (zero_fill_field_ || per_process::cli_options->zero_fill_all_buffers) {
-    COUNT_GENERIC_USAGE("NodeArrayBufferAllocator.Allocate.ZeroFilled");
-    ret = allocator_->Allocate(size);
-  } else {
-    COUNT_GENERIC_USAGE("NodeArrayBufferAllocator.Allocate.Uninitialized");
-    ret = allocator_->AllocateUninitialized(size);
-  }
+  COUNT_GENERIC_USAGE("NodeArrayBufferAllocator.Allocate.ZeroFilled");
+  ret = allocator_->Allocate(size);
   if (ret != nullptr) [[likely]] {
     total_mem_usage_.fetch_add(size, std::memory_order_relaxed);
   }
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
@@ -80,7 +80,6 @@ using v8::Object;
 using v8::SharedArrayBuffer;
 using v8::String;
 using v8::Uint32;
-using v8::Uint32Array;
 using v8::Uint8Array;
 using v8::Value;
 
@@ -1244,45 +1243,6 @@ void SetBufferPrototype(const FunctionCallbackInfo<Value>& args) {
   realm->set_buffer_prototype_object(proto);
 }
 
-void GetZeroFillToggle(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args);
-  NodeArrayBufferAllocator* allocator = env->isolate_data()->node_allocator();
-  Local<ArrayBuffer> ab;
-  // It can be a nullptr when running inside an isolate where we
-  // do not own the ArrayBuffer allocator.
-  if (allocator == nullptr || env->isolate_data()->is_building_snapshot()) {
-    // Create a dummy Uint32Array - the JS land can only toggle the C++ land
-    // setting when the allocator uses our toggle. With this the toggle in JS
-    // land results in no-ops.
-    // When building a snapshot, just use a dummy toggle as well to avoid
-    // introducing the dynamic external reference. We'll re-initialize the
-    // toggle with a real one connected to the C++ allocator after snapshot
-    // deserialization.
-
-    ab = ArrayBuffer::New(env->isolate(), sizeof(uint32_t));
-  } else {
-    // TODO(joyeecheung): save ab->GetBackingStore()->Data() in the Node.js
-    // array buffer allocator and include it into the C++ toggle while the
-    // Environment is still alive.
-    uint32_t* zero_fill_field = allocator->zero_fill_field();
-    std::unique_ptr<BackingStore> backing =
-        ArrayBuffer::NewBackingStore(zero_fill_field,
-                                     sizeof(*zero_fill_field),
-                                     [](void*, size_t, void*) {},
-                                     nullptr);
-    ab = ArrayBuffer::New(env->isolate(), std::move(backing));
-  }
-
-  if (ab->SetPrivate(env->context(),
-                     env->untransferable_object_private_symbol(),
-                     True(env->isolate()))
-          .IsNothing()) {
-    return;
-  }
-
-  args.GetReturnValue().Set(Uint32Array::New(ab, 0, 1));
-}
-
 static void Btoa(const FunctionCallbackInfo<Value>& args) {
   CHECK_EQ(args.Length(), 1);
   Environment* env = Environment::GetCurrent(args);
@@ -1449,6 +1409,57 @@ void CopyArrayBuffer(const FunctionCallbackInfo<Value>& args) {
   memcpy(dest, src, bytes_to_copy);
 }
 
+// Converts a number parameter to size_t suitable for ArrayBuffer sizes
+// Could be larger than uint32_t
+// See v8::internal::TryNumberToSize and v8::internal::NumberToSize
+inline size_t CheckNumberToSize(Local<Value> number) {
+  CHECK(number->IsNumber());
+  double value = number.As<Number>()->Value();
+  // See v8::internal::TryNumberToSize on this (and on < comparison)
+  double maxSize = static_cast<double>(std::numeric_limits<size_t>::max());
+  CHECK(value >= 0 && value < maxSize);
+  size_t size = static_cast<size_t>(value);
+#ifdef V8_ENABLE_SANDBOX
+  CHECK_LE(size, kMaxSafeBufferSizeForSandbox);
+#endif
+  return size;
+}
+
+void CreateUnsafeArrayBuffer(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  if (args.Length() != 1) {
+    env->ThrowRangeError("Invalid array buffer length");
+    return;
+  }
+
+  size_t size = CheckNumberToSize(args[0]);
+
+  Isolate* isolate = env->isolate();
+
+  Local<ArrayBuffer> buf;
+
+  // 0-length, or zero-fill flag is set, or building snapshot
+  if (size == 0 || per_process::cli_options->zero_fill_all_buffers ||
+      env->isolate_data()->is_building_snapshot()) {
+    buf = ArrayBuffer::New(isolate, size);
+  } else {
+    std::unique_ptr<BackingStore> store = ArrayBuffer::NewBackingStore(
+        isolate,
+        size,
+        BackingStoreInitializationMode::kUninitialized,
+        v8::BackingStoreOnFailureMode::kReturnNull);
+
+    if (!store) {
+      env->ThrowRangeError("Array buffer allocation failed");
+      return;
+    }
+
+    buf = ArrayBuffer::New(isolate, std::move(store));
+  }
+
+  args.GetReturnValue().Set(buf);
+}
+
 template <encoding encoding>
 uint32_t WriteOneByteString(const char* src,
                             uint32_t src_len,
@@ -1576,6 +1587,8 @@ void Initialize(Local<Object> target,
   SetMethodNoSideEffect(context, target, "indexOfString", IndexOfString);
 
   SetMethod(context, target, "copyArrayBuffer", CopyArrayBuffer);
+  SetMethodNoSideEffect(
+      context, target, "createUnsafeArrayBuffer", CreateUnsafeArrayBuffer);
 
   SetMethod(context, target, "swap16", Swap16);
   SetMethod(context, target, "swap32", Swap32);
@@ -1625,8 +1638,6 @@ void Initialize(Local<Object> target,
                 "utf8WriteStatic",
                 SlowWriteString<UTF8>,
                 &fast_write_string_utf8);
-
-  SetMethod(context, target, "getZeroFillToggle", GetZeroFillToggle);
 }
 
 }  // anonymous namespace
@@ -1675,9 +1686,9 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(StringWrite<HEX>);
   registry->Register(StringWrite<UCS2>);
   registry->Register(StringWrite<UTF8>);
-  registry->Register(GetZeroFillToggle);
 
   registry->Register(CopyArrayBuffer);
+  registry->Register(CreateUnsafeArrayBuffer);
 
   registry->Register(Atob);
   registry->Register(Btoa);
diff --git a/src/node_internals.h b/src/node_internals.h
@@ -124,8 +124,6 @@ v8::MaybeLocal<v8::Object> InitializePrivateSymbols(
 
 class NodeArrayBufferAllocator : public ArrayBufferAllocator {
  public:
-  inline uint32_t* zero_fill_field() { return &zero_fill_field_; }
-
   void* Allocate(size_t size) override;  // Defined in src/node.cc
   void* AllocateUninitialized(size_t size) override;
   void Free(void* data, size_t size) override;
@@ -142,7 +140,6 @@ class NodeArrayBufferAllocator : public ArrayBufferAllocator {
   }
 
  private:
-  uint32_t zero_fill_field_ = 1;  // Boolean but exposed as uint32 to JS land.
   std::atomic<size_t> total_mem_usage_ {0};
 
   // Delegate to V8's allocator for compatibility with the V8 memory cage.
