napi: reject SharedArrayBuffer in napi_detach_arraybuffer

src/jsc/bindings/napi.cpp

@@ -1280,11 +1280,10 @@ extern "C" napi_status napi_is_detached_arraybuffer(napi_env env,
     NAPI_CHECK_ARG(env, arraybuffer);
     NAPI_CHECK_ARG(env, result);
 
+    // Node computes IsArrayBuffer() && WasDetached() and always returns
+    // napi_ok; a non-ArrayBuffer (including SharedArrayBuffer) yields false.
     JSC::JSArrayBuffer* jsArrayBuffer = dynamicDowncast<JSC::JSArrayBuffer>(toJS(arraybuffer));
-    NAPI_RETURN_EARLY_IF_FALSE(env, jsArrayBuffer, napi_arraybuffer_expected);
-
-    auto* arrayBuffer = jsArrayBuffer->impl();
-    *result = arrayBuffer->isDetached();
+    *result = jsArrayBuffer && !jsArrayBuffer->isShared() && jsArrayBuffer->impl()->isDetached();
     NAPI_RETURN_SUCCESS(env);
 }
 
@@ -1297,10 +1296,16 @@ extern "C" napi_status napi_detach_arraybuffer(napi_env env,
     JSC::VM& vm = JSC::getVM(globalObject);
 
     JSC::JSArrayBuffer* jsArrayBuffer = dynamicDowncast<JSC::JSArrayBuffer>(toJS(arraybuffer));
-    NAPI_RETURN_EARLY_IF_FALSE(env, jsArrayBuffer, napi_arraybuffer_expected);
+    // V8's IsArrayBuffer() is false for SharedArrayBuffer; JSC uses the same
+    // cell type for both, so reject shared buffers here to match Node instead
+    // of returning napi_ok for a buffer that was never neutralized.
+    NAPI_RETURN_EARLY_IF_FALSE(env, jsArrayBuffer && !jsArrayBuffer->isShared(), napi_arraybuffer_expected);
 
     auto* arrayBuffer = jsArrayBuffer->impl();
-    if (!arrayBuffer->isDetached() && arrayBuffer->isDetachable()) {
+    // Node then requires IsDetachable(). Detaching an already-detached buffer
+    // is a no-op in both engines, so treat that as success.
+    NAPI_RETURN_EARLY_IF_FALSE(env, arrayBuffer->isDetached() || arrayBuffer->isDetachable(), napi_detachable_arraybuffer_expected);
+    if (!arrayBuffer->isDetached()) {
         arrayBuffer->detach(vm);
     }
     NAPI_RETURN_SUCCESS(env);

test/napi/napi-app/js_test_helpers.cpp

@@ -303,22 +303,6 @@ static napi_value get_property_names(const Napi::CallbackInfo &info) {
   return result;
 }
 
-// get_all_property_names(object, key_mode, key_filter, key_conversion) -> array
-static napi_value get_all_property_names(const Napi::CallbackInfo &info) {
-  napi_env env = info.Env();
-  uint32_t key_mode, key_filter, key_conversion;
-  NODE_API_CALL(env, napi_get_value_uint32(env, info[1], &key_mode));
-  NODE_API_CALL(env, napi_get_value_uint32(env, info[2], &key_filter));
-  NODE_API_CALL(env, napi_get_value_uint32(env, info[3], &key_conversion));
-  napi_value result;
-  NODE_API_CALL(env,
-                napi_get_all_property_names(
-                    env, info[0], (napi_key_collection_mode)key_mode,
-                    (napi_key_filter)key_filter,
-                    (napi_key_conversion)key_conversion, &result));
-  return result;
-}
-
 // add_tag(object, lower, upper)
 static napi_value add_tag(const Napi::CallbackInfo &info) {
   Napi::Env env = info.Env();
@@ -512,7 +496,6 @@ void register_js_test_helpers(Napi::Env env, Napi::Object exports) {
   REGISTER_FUNCTION(env, exports, make_empty_array);
   REGISTER_FUNCTION(env, exports, make_empty_object);
   REGISTER_FUNCTION(env, exports, get_property_names);
-  REGISTER_FUNCTION(env, exports, get_all_property_names);
   REGISTER_FUNCTION(env, exports, add_tag);
   REGISTER_FUNCTION(env, exports, try_add_tag);
   REGISTER_FUNCTION(env, exports, check_tag);

test/napi/napi-app/module.js

@@ -266,7 +266,7 @@ nativeTests.test_property_names_cache_poisoning = () => {
   console.log("Reflect.ownKeys after get_all_property_names(own_only):", Reflect.ownKeys(mkD()).join(","));
 
   // The napi result itself should still include inherited keys.
-  const apnResult = nativeTests.get_all_property_names(mkA(), 0, 0, 0);
+  const apnResult = nativeTests.get_all_property_names(mkA(), 0, 0, 0).keys;
   console.log("napi include_prototypes result has own a,b:", apnResult.includes("a") && apnResult.includes("b"));
   console.log("napi include_prototypes result has inherited toString:", apnResult.includes("toString"));
   const gpnResult = nativeTests.get_property_names(mkB());

test/napi/napi-app/standalone_tests.cpp

@@ -757,6 +757,30 @@ static napi_value test_is_arraybuffer(const Napi::CallbackInfo &info) {
   return ok(env);
 }
 
+static napi_value test_detach_arraybuffer(const Napi::CallbackInfo &info) {
+  napi_env env = info.Env();
+  for (size_t i = 1; i < info.Length(); i++) {
+    napi_value value = info[i];
+
+    napi_status detach_status = napi_detach_arraybuffer(env, value);
+
+    bool is_detached = false;
+    napi_status is_detached_status =
+        napi_is_detached_arraybuffer(env, value, &is_detached);
+
+    size_t length = 0;
+    napi_status info_status =
+        napi_get_arraybuffer_info(env, value, nullptr, &length);
+
+    printf("napi_detach_arraybuffer=%d napi_is_detached_arraybuffer=%d "
+           "is_detached=%s napi_get_arraybuffer_info=%d length=%zu\n",
+           static_cast<int>(detach_status),
+           static_cast<int>(is_detached_status), is_detached ? "true" : "false",
+           static_cast<int>(info_status), length);
+  }
+  return ok(env);
+}
+
 static napi_value test_napi_get_default_values(const Napi::CallbackInfo &info) {
   napi_env env = info.Env();
 
@@ -2707,6 +2731,7 @@ void register_standalone_tests(Napi::Env env, Napi::Object exports) {
   REGISTER_FUNCTION(env, exports, test_is_buffer);
   REGISTER_FUNCTION(env, exports, test_is_typedarray);
   REGISTER_FUNCTION(env, exports, test_is_arraybuffer);
+  REGISTER_FUNCTION(env, exports, test_detach_arraybuffer);
   REGISTER_FUNCTION(env, exports, test_napi_get_default_values);
   REGISTER_FUNCTION(env, exports, test_napi_numeric_string_keys);
   REGISTER_FUNCTION(env, exports, test_deferred_exceptions);

test/napi/napi.test.ts

@@ -1323,6 +1323,27 @@ describe.skipIf(!canBuildNodeAddons())("cleanup hooks", () => {
     });
   });
 
+  describe("napi_detach_arraybuffer", () => {
+    it("rejects SharedArrayBuffer instead of returning napi_ok for a no-op detach", async () => {
+      // napi_ok on a SharedArrayBuffer is a memory-lifetime lie: the addon
+      // believes the backing store is neutralized while JS (and other threads)
+      // still read and write it. Node rejects a SharedArrayBuffer with
+      // napi_arraybuffer_expected (19) because V8's IsArrayBuffer() is false
+      // for a SharedArrayBuffer. The same ArrayBuffer is passed twice so the
+      // third row covers a second detach on an already-detached buffer.
+      const output = await checkSameOutput(
+        "test_detach_arraybuffer",
+        "(() => { const ab = new ArrayBuffer(8); return [new SharedArrayBuffer(8), ab, ab, new Uint8Array(8)]; })()",
+      );
+      expect(output.split(/\r?\n/)).toEqual([
+        "napi_detach_arraybuffer=19 napi_is_detached_arraybuffer=0 is_detached=false napi_get_arraybuffer_info=0 length=8",
+        "napi_detach_arraybuffer=0 napi_is_detached_arraybuffer=0 is_detached=true napi_get_arraybuffer_info=0 length=0",
+        "napi_detach_arraybuffer=0 napi_is_detached_arraybuffer=0 is_detached=true napi_get_arraybuffer_info=0 length=0",
+        "napi_detach_arraybuffer=19 napi_is_detached_arraybuffer=0 is_detached=false napi_get_arraybuffer_info=1 length=0",
+      ]);
+    });
+  });
+
   describe("error handling", () => {
     it("removing non-existent env cleanup hook should not crash", async () => {
       // Test that removing non-existent hooks doesn't crash the process