Disable caching in ArrayBuffer

tniessen · Gabriel Schulhof · commit b72f1d69786c · 2019-12-14T08:13:38.000-08:00
Caching the data pointer and the byteLength in the ArrayBuffer class causes it to behave incorrectly when the buffer is detached. PR-URL: #611 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
diff --git a/napi-inl.h b/napi-inl.h
@@ -1346,7 +1346,7 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env, size_t byteLength) {
   napi_status status = napi_create_arraybuffer(env, byteLength, &data, &value);
   NAPI_THROW_IF_FAILED(env, status, ArrayBuffer());
 
-  return ArrayBuffer(env, value, data, byteLength);
+  return ArrayBuffer(env, value);
 }
 
 inline ArrayBuffer ArrayBuffer::New(napi_env env,
@@ -1357,7 +1357,7 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
     env, externalData, byteLength, nullptr, nullptr, &value);
   NAPI_THROW_IF_FAILED(env, status, ArrayBuffer());
 
-  return ArrayBuffer(env, value, externalData, byteLength);
+  return ArrayBuffer(env, value);
 }
 
 template <typename Finalizer>
@@ -1380,7 +1380,7 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
     NAPI_THROW_IF_FAILED(env, status, ArrayBuffer());
   }
 
-  return ArrayBuffer(env, value, externalData, byteLength);
+  return ArrayBuffer(env, value);
 }
 
 template <typename Finalizer, typename Hint>
@@ -1404,38 +1404,28 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
     NAPI_THROW_IF_FAILED(env, status, ArrayBuffer());
   }
 
-  return ArrayBuffer(env, value, externalData, byteLength);
+  return ArrayBuffer(env, value);
 }
 
-inline ArrayBuffer::ArrayBuffer() : Object(), _data(nullptr), _length(0) {
+inline ArrayBuffer::ArrayBuffer() : Object() {
 }
 
 inline ArrayBuffer::ArrayBuffer(napi_env env, napi_value value)
-  : Object(env, value), _data(nullptr), _length(0) {
-}
-
-inline ArrayBuffer::ArrayBuffer(napi_env env, napi_value value, void* data, size_t length)
-  : Object(env, value), _data(data), _length(length) {
+  : Object(env, value) {
 }
 
 inline void* ArrayBuffer::Data() {
-  EnsureInfo();
-  return _data;
+  void* data;
+  napi_status status = napi_get_arraybuffer_info(_env, _value, &data, nullptr);
+  NAPI_THROW_IF_FAILED(_env, status, nullptr);
+  return data;
 }
 
 inline size_t ArrayBuffer::ByteLength() {
-  EnsureInfo();
-  return _length;
-}
-
-inline void ArrayBuffer::EnsureInfo() const {
-  // The ArrayBuffer instance may have been constructed from a napi_value whose
-  // length/data are not yet known. Fetch and cache these values just once,
-  // since they can never change during the lifetime of the ArrayBuffer.
-  if (_data == nullptr) {
-    napi_status status = napi_get_arraybuffer_info(_env, _value, &_data, &_length);
-    NAPI_THROW_IF_FAILED_VOID(_env, status);
-  }
+  size_t length;
+  napi_status status = napi_get_arraybuffer_info(_env, _value, nullptr, &length);
+  NAPI_THROW_IF_FAILED(_env, status, 0);
+  return length;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/napi.h b/napi.h
@@ -124,6 +124,7 @@ namespace Napi {
   class String;
   class Object;
   class Array;
+  class ArrayBuffer;
   class Function;
   template <typename T> class Buffer;
   class Error;
@@ -806,13 +807,6 @@ namespace Napi {
 
     void* Data();        ///< Gets a pointer to the data buffer.
     size_t ByteLength(); ///< Gets the length of the array buffer in bytes.
-
-  private:
-    mutable void* _data;
-    mutable size_t _length;
-
-    ArrayBuffer(napi_env env, napi_value value, void* data, size_t length);
-    void EnsureInfo() const;
   };
 
   /// A JavaScript typed-array value with unknown array type.
diff --git a/test/arraybuffer.cc b/test/arraybuffer.cc
@@ -151,6 +151,37 @@ Value CheckEmptyBuffer(const CallbackInfo& info) {
   return Boolean::New(info.Env(), buffer.IsEmpty());
 }
 
+void CheckDetachUpdatesData(const CallbackInfo& info) {
+  if (!info[0].IsArrayBuffer()) {
+    Error::New(info.Env(), "A buffer was expected.").ThrowAsJavaScriptException();
+    return;
+  }
+
+  if (!info[1].IsFunction()) {
+    Error::New(info.Env(), "A function was expected.").ThrowAsJavaScriptException();
+    return;
+  }
+
+  ArrayBuffer buffer = info[0].As<ArrayBuffer>();
+  Function detach = info[1].As<Function>();
+
+  // This potentially causes the buffer to cache its data pointer and length.
+  buffer.Data();
+  buffer.ByteLength();
+
+  detach.Call({});
+
+  if (buffer.Data() != nullptr) {
+    Error::New(info.Env(), "Incorrect data pointer.").ThrowAsJavaScriptException();
+    return;
+  }
+
+  if (buffer.ByteLength() != 0) {
+    Error::New(info.Env(), "Incorrect buffer length.").ThrowAsJavaScriptException();
+    return;
+  }
+}
+
 } // end anonymous namespace
 
 Object InitArrayBuffer(Env env) {
@@ -166,6 +197,7 @@ Object InitArrayBuffer(Env env) {
   exports["getFinalizeCount"] = Function::New(env, GetFinalizeCount);
   exports["createBufferWithConstructor"] = Function::New(env, CreateBufferWithConstructor);
   exports["checkEmptyBuffer"] = Function::New(env, CheckEmptyBuffer);
+  exports["checkDetachUpdatesData"] = Function::New(env, CheckDetachUpdatesData);
 
   return exports;
 }
diff --git a/test/arraybuffer.js b/test/arraybuffer.js
@@ -61,5 +61,11 @@ function test(binding) {
       binding.arraybuffer.checkBuffer(test);
       assert.ok(test instanceof ArrayBuffer);
     },
+
+    'ArrayBuffer updates data pointer and length when detached',
+    () => {
+      const mem = new WebAssembly.Memory({ initial: 1 });
+      binding.arraybuffer.checkDetachUpdatesData(mem.buffer, () => mem.grow(1));
+    },
   ]);
 }
