buffer: refactor zero-fill-field passing

Instead of storing the zero-fill-field as a fixed `ArrayBuffer`
on the binding, only load it via a function call once bootstrapping
is complete. This makes sure that the value is set correctly
after loading from a snapshot (i.e. that the accessor for the
field is stored in the snapshot, not the field itself).

Author: addaleax

lib/buffer.js

@@ -56,7 +56,7 @@ const {
   swap64: _swap64,
   kMaxLength,
   kStringMaxLength,
-  zeroFill: bindingZeroFill
+  getZeroFillField
 } = internalBinding('buffer');
 const {
   arraybuffer_untransferable_private_symbol,
@@ -131,18 +131,22 @@ const constants = ObjectDefineProperties({}, {
   }
 });
 
+const encodingsMap = ObjectCreate(null);
+for (let i = 0; i < encodings.length; ++i)
+  encodingsMap[encodings[i]] = i;
+
 Buffer.poolSize = 8 * 1024;
 let poolSize, poolOffset, allocPool;
 
 // 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.
-const zeroFill = bindingZeroFill || [0];
-
-const encodingsMap = ObjectCreate(null);
-for (let i = 0; i < encodings.length; ++i)
-  encodingsMap[encodings[i]] = i;
+// getZeroFillField() can return undefined when running inside an isolate where
+// we do not own the ArrayBuffer allocator. Zero fill is always on in that case.
+let zeroFill = new Uint32Array([0]);
+function refreshZeroFillField() {
+  const fromBinding = getZeroFillField();
+  if (fromBinding) zeroFill = fromBinding;
+}
 
 function createUnsafeBuffer(size) {
   zeroFill[0] = 0;
@@ -1207,7 +1211,9 @@ module.exports = {
   transcode,
   // Legacy
   kMaxLength,
-  kStringMaxLength
+  kStringMaxLength,
+  // Called and deleted by internal/bootstrap/pre_execution.js.
+  refreshZeroFillField
 };
 
 ObjectDefineProperties(module.exports, {

lib/internal/bootstrap/node.js

@@ -312,7 +312,7 @@ function setupBuffer() {
   // Only after this point can C++ use Buffer::New()
   bufferBinding.setBufferPrototype(Buffer.prototype);
   delete bufferBinding.setBufferPrototype;
-  delete bufferBinding.zeroFill;
+  delete bufferBinding.getZeroFillField;
 
   ObjectDefineProperty(global, 'Buffer', {
     value: Buffer,

lib/internal/bootstrap/pre_execution.js

@@ -7,7 +7,8 @@ const {
 } = primordials;
 
 const { getOptionValue } = require('internal/options');
-const { Buffer } = require('buffer');
+const buffer = require('buffer');
+const { Buffer } = buffer;
 const { ERR_MANIFEST_ASSERT_INTEGRITY } = require('internal/errors').codes;
 const assert = require('internal/assert');
 
@@ -107,6 +108,9 @@ function patchProcessObject(expandArgv1) {
   addReadOnlyProcessAlias('traceDeprecation', '--trace-deprecation');
   addReadOnlyProcessAlias('_breakFirstLine', '--inspect-brk', false);
   addReadOnlyProcessAlias('_breakNodeFirstLine', '--inspect-brk-node', false);
+
+  buffer.refreshZeroFillField();
+  delete buffer.refreshZeroFillField;
 }
 
 function addReadOnlyProcessAlias(name, option, enumerable = true) {

src/api/environment.cc

@@ -14,6 +14,7 @@
 namespace node {
 using errors::TryCatchScope;
 using v8::Array;
+using v8::ArrayBuffer;
 using v8::Context;
 using v8::EscapableHandleScope;
 using v8::FinalizationGroup;
@@ -91,6 +92,17 @@ static void HostCleanupFinalizationGroupCallback(
   env->RegisterFinalizationGroupForCleanup(group);
 }
 
+std::shared_ptr<v8::BackingStore> NodeArrayBufferAllocator::zero_fill_field() {
+  if (!zero_fill_field_bs_) {
+    zero_fill_field_bs_ =
+        ArrayBuffer::NewBackingStore(&zero_fill_field_,
+                                     sizeof(zero_fill_field_),
+                                     [](void*, size_t, void*){},
+                                     nullptr);
+  }
+  return zero_fill_field_bs_;
+}
+
 void* NodeArrayBufferAllocator::Allocate(size_t size) {
   void* ret;
   if (zero_fill_field_ || per_process::cli_options->zero_fill_all_buffers)

src/node_buffer.cc

@@ -1152,13 +1152,33 @@ void SetBufferPrototype(const FunctionCallbackInfo<Value>& args) {
 }
 
 
+void GetZeroFillField(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+
+  // This can be a nullptr when running inside an isolate where we
+  // do not own the ArrayBuffer allocator.
+  NodeArrayBufferAllocator* allocator = env->isolate_data()->node_allocator();
+  if (allocator == nullptr) return;
+
+  std::shared_ptr<v8::BackingStore> backing = allocator->zero_fill_field();
+  Local<ArrayBuffer> array_buffer =
+      ArrayBuffer::New(env->isolate(), std::move(backing));
+  array_buffer->SetPrivate(
+      env->context(),
+      env->arraybuffer_untransferable_private_symbol(),
+      True(env->isolate())).Check();
+  args.GetReturnValue().Set(Uint32Array::New(array_buffer, 0, 1));
+}
+
+
 void Initialize(Local<Object> target,
                 Local<Value> unused,
                 Local<Context> context,
                 void* priv) {
   Environment* env = Environment::GetCurrent(context);
 
   env->SetMethod(target, "setBufferPrototype", SetBufferPrototype);
+  env->SetMethod(target, "getZeroFillField", GetZeroFillField);
   env->SetMethodNoSideEffect(target, "createFromString", CreateFromString);
 
   env->SetMethodNoSideEffect(target, "byteLengthUtf8", ByteLengthUtf8);
@@ -1198,34 +1218,12 @@ void Initialize(Local<Object> target,
   env->SetMethod(target, "hexWrite", StringWrite<HEX>);
   env->SetMethod(target, "ucs2Write", StringWrite<UCS2>);
   env->SetMethod(target, "utf8Write", StringWrite<UTF8>);
-
-  // It can be a nullptr when running inside an isolate where we
-  // do not own the ArrayBuffer allocator.
-  if (NodeArrayBufferAllocator* allocator =
-          env->isolate_data()->node_allocator()) {
-    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);
-    Local<ArrayBuffer> array_buffer =
-        ArrayBuffer::New(env->isolate(), std::move(backing));
-    array_buffer->SetPrivate(
-        env->context(),
-        env->arraybuffer_untransferable_private_symbol(),
-        True(env->isolate())).Check();
-    CHECK(target
-              ->Set(env->context(),
-                    FIXED_ONE_BYTE_STRING(env->isolate(), "zeroFill"),
-                    Uint32Array::New(array_buffer, 0, 1))
-              .FromJust());
-  }
 }
 
 static ExternalReferences external_references {
   __FILE__,
   SetBufferPrototype,
+  GetZeroFillField,
   CreateFromString,
   ByteLengthUtf8,
   Copy,

src/node_internals.h

@@ -107,7 +107,7 @@ void PromiseRejectCallback(v8::PromiseRejectMessage message);
 
 class NodeArrayBufferAllocator : public ArrayBufferAllocator {
  public:
-  inline uint32_t* zero_fill_field() { return &zero_fill_field_; }
+  std::shared_ptr<v8::BackingStore> zero_fill_field();
 
   void* Allocate(size_t size) override;  // Defined in src/node.cc
   void* AllocateUninitialized(size_t size) override;
@@ -127,6 +127,7 @@ class NodeArrayBufferAllocator : public ArrayBufferAllocator {
 
  private:
   uint32_t zero_fill_field_ = 1;  // Boolean but exposed as uint32 to JS land.
+  std::shared_ptr<v8::BackingStore> zero_fill_field_bs_;
   std::atomic<size_t> total_mem_usage_ {0};
 };