src: mark ArrayBuffers with free callbacks as untransferable

More precisely, make them untransferable if they were created through
*our* APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: https://github.com/nodejs/node/pull/30339#issuecomment-552225353

PR-URL: https://github.com/nodejs/node/pull/30475
Backport-PR-URL: https://github.com/nodejs/node/pull/31355
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>

9 files changed, 86 insertions, 4 deletions

diff --git a/src/env.h b/src/env.h
index 1ae7074282..8b690c0906 100644
--- a/src/env.h
+++ b/src/env.h
@@ -151,6 +151,7 @@ constexpr size_t kFsStatsBufferLength =
 // "node:" prefix to avoid name clashes with third-party code.
 #define PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V)                              \
   V(alpn_buffer_private_symbol, "node:alpnBuffer")                            \
+  V(arraybuffer_untransferable_private_symbol, "node:untransferableBuffer")   \
   V(arrow_message_private_symbol, "node:arrowMessage")                        \
   V(contextify_context_private_symbol, "node:contextify:context")             \
   V(contextify_global_private_symbol, "node:contextify:global")               \
diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
index f29adeb8f8..b90db8f3e8 100644
--- a/src/js_native_api_v8.cc
+++ b/src/js_native_api_v8.cc
@@ -2614,6 +2614,8 @@ napi_status napi_create_external_arraybuffer(napi_env env,
   v8::Isolate* isolate = env->isolate;
   v8::Local<v8::ArrayBuffer> buffer =
       v8::ArrayBuffer::New(isolate, external_data, byte_length);
+  v8::Maybe<bool> marked = env->mark_arraybuffer_as_untransferable(buffer);
+  CHECK_MAYBE_NOTHING(env, marked, napi_generic_failure);
 
   if (finalize_cb != nullptr) {
     // Create a self-deleting weak reference that invokes the finalizer
diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h
index 8eed25bc9e..0d5424ccca 100644
--- a/src/js_native_api_v8.h
+++ b/src/js_native_api_v8.h
@@ -39,6 +39,10 @@ struct napi_env__ {
   inline void Unref() { if ( --refs == 0) delete this; }
 
   virtual bool can_call_into_js() const { return true; }
+  virtual v8::Maybe<bool> mark_arraybuffer_as_untransferable(
+      v8::Local<v8::ArrayBuffer> ab) const {
+    return v8::Just(true);
+  }
 
   template <typename T, typename U>
   void CallIntoModule(T&& call, U&& handle_exception) {
diff --git a/src/node_api.cc b/src/node_api.cc
index 20b591747d..0f7981cd56 100644
--- a/src/node_api.cc
+++ b/src/node_api.cc
@@ -24,6 +24,14 @@ struct node_napi_env__ : public napi_env__ {
   bool can_call_into_js() const override {
     return node_env()->can_call_into_js();
   }
+
+  v8::Maybe<bool> mark_arraybuffer_as_untransferable(
+      v8::Local<v8::ArrayBuffer> ab) const {
+    return ab->SetPrivate(
+        context(),
+        node_env()->arraybuffer_untransferable_private_symbol(),
+        v8::True(isolate));
+  }
 };
 
 typedef node_napi_env__* node_napi_env;
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
index 0cbc441c3c..e80604e2c0 100644
--- a/src/node_buffer.cc
+++ b/src/node_buffer.cc
@@ -364,10 +364,8 @@ MaybeLocal<Object> New(Isolate* isolate,
     THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate);
     return MaybeLocal<Object>();
   }
-  Local<Object> obj;
-  if (Buffer::New(env, data, length, callback, hint).ToLocal(&obj))
-    return handle_scope.Escape(obj);
-  return Local<Object>();
+  return handle_scope.EscapeMaybe(
+      Buffer::New(env, data, length, callback, hint));
 }
 
 
@@ -385,6 +383,12 @@ MaybeLocal<Object> New(Environment* env,
   }
 
   Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(), data, length);
+  if (ab->SetPrivate(env->context(),
+                     env->arraybuffer_untransferable_private_symbol(),
+                     True(env->isolate())).IsNothing()) {
+    callback(data, hint);
+    return Local<Object>();
+  }
   MaybeLocal<Uint8Array> ui = Buffer::New(env, ab, 0, length);
 
   CallbackInfo::New(env->isolate(), ab, callback, data, hint);
diff --git a/src/node_messaging.cc b/src/node_messaging.cc
index 19065fdb7d..b0e7d15a8c 100644
--- a/src/node_messaging.cc
+++ b/src/node_messaging.cc
@@ -329,6 +329,16 @@ Maybe<bool> Message::Serialize(Environment* env,
           !env->isolate_data()->uses_node_allocator()) {
         continue;
       }
+      // See https://github.com/nodejs/node/pull/30339#issuecomment-552225353
+      // for details.
+      bool untransferrable;
+      if (!ab->HasPrivate(
+              context,
+              env->arraybuffer_untransferable_private_symbol())
+              .To(&untransferrable)) {
+        return Nothing<bool>();
+      }
+      if (untransferrable) continue;
       if (std::find(array_buffers.begin(), array_buffers.end(), ab) !=
           array_buffers.end()) {
         ThrowDataCloneException(
diff --git a/test/addons/worker-buffer-callback/binding.cc b/test/addons/worker-buffer-callback/binding.cc
new file mode 100644
index 0000000000..a40876ebb5
--- /dev/null
+++ b/test/addons/worker-buffer-callback/binding.cc
@@ -0,0 +1,29 @@
+#include <node.h>
+#include <node_buffer.h>
+#include <v8.h>
+
+using v8::Context;
+using v8::Isolate;
+using v8::Local;
+using v8::Object;
+using v8::Value;
+
+char data[] = "hello";
+
+void Initialize(Local<Object> exports,
+                Local<Value> module,
+                Local<Context> context) {
+  Isolate* isolate = context->GetIsolate();
+  exports->Set(context,
+               v8::String::NewFromUtf8(
+                   isolate, "buffer", v8::NewStringType::kNormal)
+                   .ToLocalChecked(),
+               node::Buffer::New(
+                   isolate,
+                   data,
+                   sizeof(data),
+                   [](char* data, void* hint) {},
+                   nullptr).ToLocalChecked()).Check();
+}
+
+NODE_MODULE_CONTEXT_AWARE(NODE_GYP_MODULE_NAME, Initialize)
diff --git a/test/addons/worker-buffer-callback/binding.gyp b/test/addons/worker-buffer-callback/binding.gyp
new file mode 100644
index 0000000000..55fbe7050f
--- /dev/null
+++ b/test/addons/worker-buffer-callback/binding.gyp
@@ -0,0 +1,9 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'sources': [ 'binding.cc' ],
+      'includes': ['../common.gypi'],
+    }
+  ]
+}
diff --git a/test/addons/worker-buffer-callback/test.js b/test/addons/worker-buffer-callback/test.js
new file mode 100644
index 0000000000..b04984f157
--- /dev/null
+++ b/test/addons/worker-buffer-callback/test.js
@@ -0,0 +1,15 @@
+'use strict';
+const common = require('../../common');
+const assert = require('assert');
+const { MessageChannel } = require('worker_threads');
+const { buffer } = require(`./build/${common.buildType}/binding`);
+
+// Test that buffers allocated with a free callback through our APIs are not
+// transfered.
+
+const { port1 } = new MessageChannel();
+const origByteLength = buffer.byteLength;
+port1.postMessage(buffer, [buffer.buffer]);
+
+assert.strictEqual(buffer.byteLength, origByteLength);
+assert.notStrictEqual(buffer.byteLength, 0);