From b72f1d69786cf5092db0dd24352645a9652d4615 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] 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. PR-URL: https://github.com/nodejs/node-addon-api/pull/611 Reviewed-By: Nicola Del Gobbo Reviewed-By: Michael Dawson --- napi-inl.h | 38 ++++++++++++++------------------------ napi.h | 8 +------- test/arraybuffer.cc | 32 ++++++++++++++++++++++++++++++++ test/arraybuffer.js | 6 ++++++ 4 files changed, 53 insertions(+), 31 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index 7bef465fc..8647cd8dc 100644 --- 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 @@ -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 @@ -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 index c44b3ba92..62198b1b3 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; @@ -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 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)); + }, ]); }