From 25686d8031870a43b29e213f8aa2fbd039470324 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Thu, 28 Nov 2019 06:19:07 +0100 Subject: [PATCH 1/2] Disable caching in ArrayBuffer Caching the data pointer and the byteLength in the ArrayBuffer class causes it to behave incorrectly when the buffer is detached. --- napi-inl.h | 38 ++++++++++++++------------------------ napi.h | 7 ------- test/arraybuffer.cc | 32 ++++++++++++++++++++++++++++++++ test/arraybuffer.js | 6 ++++++ 4 files changed, 52 insertions(+), 31 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index f72e1daea..59315c842 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -1330,7 +1330,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, @@ -1341,7 +1341,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 @@ -1364,7 +1364,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 @@ -1388,38 +1388,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 index 2fc9b10f3..1dad9249f 100644 --- a/napi.h +++ b/napi.h @@ -802,13 +802,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 index e46f8cba3..cc4f1f51c 100644 --- 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(); + Function detach = info[1].As(); + + // 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 index 43604617f..30136980b 100644 --- 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)); + }, ]); } From f375ca99a6a2f86a345243ed684301e48c3e8d0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Fri, 29 Nov 2019 15:20:18 +0100 Subject: [PATCH 2/2] Add forward declaration for ArrayBuffer in napi.h --- napi.h | 1 + 1 file changed, 1 insertion(+) diff --git a/napi.h b/napi.h index 1dad9249f..a4b3a3ff0 100644 --- a/napi.h +++ b/napi.h @@ -124,6 +124,7 @@ namespace Napi { class String; class Object; class Array; + class ArrayBuffer; class Function; template class Buffer; class Error;