diff --git a/src/env-inl.h b/src/env-inl.h index b5a044a1b00995..d6207257b11537 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -867,117 +867,91 @@ inline IsolateData* Environment::isolate_data() const { return isolate_data_; } -inline char* Environment::AllocateUnchecked(size_t size) { - return static_cast( - isolate_data()->allocator()->AllocateUninitialized(size)); +AllocatedBuffer Environment::AllocateManaged(size_t size) { + NoArrayBufferZeroFillScope no_zero_fill_scope(isolate_data()); + std::unique_ptr bs = + v8::ArrayBuffer::NewBackingStore(isolate(), size); + return AllocatedBuffer(this, std::move(bs)); } -inline char* Environment::Allocate(size_t size) { - char* ret = AllocateUnchecked(size); - CHECK_NE(ret, nullptr); - return ret; -} - -inline void Environment::Free(char* data, size_t size) { - if (data != nullptr) - isolate_data()->allocator()->Free(data, size); -} - -inline AllocatedBuffer Environment::AllocateManaged(size_t size, bool checked) { - char* data = checked ? Allocate(size) : AllocateUnchecked(size); - if (data == nullptr) size = 0; - return AllocatedBuffer(this, uv_buf_init(data, size)); +std::unordered_map>* + Environment::released_allocated_buffers() { + return &released_allocated_buffers_; } -inline AllocatedBuffer::AllocatedBuffer(Environment* env, uv_buf_t buf) - : env_(env), buffer_(buf) {} +AllocatedBuffer::AllocatedBuffer( + Environment* env, std::unique_ptr bs) + : env_(env), backing_store_(std::move(bs)) {} -inline void AllocatedBuffer::Resize(size_t len) { - // The `len` check is to make sure we don't end up with `nullptr` as our base. - char* new_data = env_->Reallocate(buffer_.base, buffer_.len, - len > 0 ? len : 1); - CHECK_NOT_NULL(new_data); - buffer_ = uv_buf_init(new_data, len); -} +AllocatedBuffer::AllocatedBuffer( + Environment* env, uv_buf_t buffer) + : env_(env) { + if (buffer.base == nullptr) return; -inline uv_buf_t AllocatedBuffer::release() { - uv_buf_t ret = buffer_; - buffer_ = uv_buf_init(nullptr, 0); - return ret; + auto map = env->released_allocated_buffers(); + auto it = map->find(buffer.base); + CHECK_NE(it, map->end()); + backing_store_ = std::move(it->second); + map->erase(it); } -inline char* AllocatedBuffer::data() { - return buffer_.base; +void AllocatedBuffer::Resize(size_t len) { + if (len == 0) { + backing_store_ = v8::ArrayBuffer::NewBackingStore(env_->isolate(), 0); + return; + } + NoArrayBufferZeroFillScope no_zero_fill_scope(env_->isolate_data()); + backing_store_ = v8::BackingStore::Reallocate( + env_->isolate(), std::move(backing_store_), len); } -inline const char* AllocatedBuffer::data() const { - return buffer_.base; -} +inline uv_buf_t AllocatedBuffer::release() { + if (data() == nullptr) return uv_buf_init(nullptr, 0); -inline size_t AllocatedBuffer::size() const { - return buffer_.len; + CHECK_NOT_NULL(env_); + uv_buf_t ret = uv_buf_init(data(), size()); + env_->released_allocated_buffers()->emplace( + ret.base, std::move(backing_store_)); + return ret; } -inline AllocatedBuffer::AllocatedBuffer(Environment* env) - : env_(env), buffer_(uv_buf_init(nullptr, 0)) {} - -inline AllocatedBuffer::AllocatedBuffer(AllocatedBuffer&& other) - : AllocatedBuffer() { - *this = std::move(other); +char* AllocatedBuffer::data() { + if (!backing_store_) return nullptr; + return static_cast(backing_store_->Data()); } -inline AllocatedBuffer& AllocatedBuffer::operator=(AllocatedBuffer&& other) { - clear(); - env_ = other.env_; - buffer_ = other.release(); - return *this; +const char* AllocatedBuffer::data() const { + if (!backing_store_) return nullptr; + return static_cast(backing_store_->Data()); } -inline AllocatedBuffer::~AllocatedBuffer() { - clear(); +size_t AllocatedBuffer::size() const { + if (!backing_store_) return 0; + return backing_store_->ByteLength(); } -inline void AllocatedBuffer::clear() { - uv_buf_t buf = release(); - if (buf.base != nullptr) { - CHECK_NOT_NULL(env_); - env_->Free(buf.base, buf.len); - } +void AllocatedBuffer::clear() { + backing_store_.reset(); } // It's a bit awkward to define this Buffer::New() overload here, but it // avoids a circular dependency with node_internals.h. namespace Buffer { -v8::MaybeLocal New(Environment* env, - char* data, - size_t length, - bool uses_malloc); +v8::MaybeLocal New(Environment* env, + v8::Local ab, + size_t byte_offset, + size_t length); } inline v8::MaybeLocal AllocatedBuffer::ToBuffer() { - CHECK_NOT_NULL(env_); - v8::MaybeLocal obj = Buffer::New(env_, data(), size(), false); - if (!obj.IsEmpty()) release(); - return obj; + v8::Local ab = ToArrayBuffer(); + return Buffer::New(env_, ab, 0, ab->ByteLength()) + .FromMaybe(v8::Local()); } inline v8::Local AllocatedBuffer::ToArrayBuffer() { CHECK_NOT_NULL(env_); - uv_buf_t buf = release(); - auto callback = [](void* data, size_t length, void* deleter_data){ - CHECK_NOT_NULL(deleter_data); - - static_cast(deleter_data) - ->Free(data, length); - }; - std::unique_ptr backing = - v8::ArrayBuffer::NewBackingStore(buf.base, - buf.len, - callback, - env_->isolate() - ->GetArrayBufferAllocator()); - return v8::ArrayBuffer::New(env_->isolate(), - std::move(backing)); + return v8::ArrayBuffer::New(env_->isolate(), std::move(backing_store_)); } inline void Environment::ThrowError(const char* errmsg) { diff --git a/src/env.cc b/src/env.cc index b83bd62835128c..d451c6a78f5c65 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1133,29 +1133,20 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const { // node, we shift its sizeof() size out of the Environment node. } -char* Environment::Reallocate(char* data, size_t old_size, size_t size) { - if (old_size == size) return data; - // If we know that the allocator is our ArrayBufferAllocator, we can let - // if reallocate directly. - if (isolate_data()->uses_node_allocator()) { - return static_cast( - isolate_data()->node_allocator()->Reallocate(data, old_size, size)); - } - // Generic allocators do not provide a reallocation method; we need to - // allocate a new chunk of memory and copy the data over. - char* new_data = AllocateUnchecked(size); - if (new_data == nullptr) return nullptr; - memcpy(new_data, data, std::min(size, old_size)); - if (size > old_size) - memset(new_data + old_size, 0, size - old_size); - Free(data, old_size); - return new_data; -} - void Environment::RunWeakRefCleanup() { isolate()->ClearKeptObjects(); } +NoArrayBufferZeroFillScope::NoArrayBufferZeroFillScope( + IsolateData* isolate_data) + : node_allocator_(isolate_data->node_allocator()) { + if (node_allocator_ != nullptr) node_allocator_->zero_fill_field()[0] = 0; +} + +NoArrayBufferZeroFillScope::~NoArrayBufferZeroFillScope() { + if (node_allocator_ != nullptr) node_allocator_->zero_fill_field()[0] = 1; +} + // Not really any better place than env.cc at this moment. void BaseObject::DeleteMe(void* data) { BaseObject* self = static_cast(data); diff --git a/src/env.h b/src/env.h index f4456228849d8b..deff0e23bd95b6 100644 --- a/src/env.h +++ b/src/env.h @@ -557,13 +557,29 @@ struct ContextInfo { class EnabledDebugList; +// Disables zero-filling for ArrayBuffer allocations in this scope. This is +// similar to how we implement Buffer.allocUnsafe() in JS land. +class NoArrayBufferZeroFillScope{ + public: + explicit NoArrayBufferZeroFillScope(IsolateData* isolate_data); + ~NoArrayBufferZeroFillScope(); + + private: + NodeArrayBufferAllocator* node_allocator_; +}; + // A unique-pointer-ish object that is compatible with the JS engine's // ArrayBuffer::Allocator. -struct AllocatedBuffer { +// TODO(addaleax): We may want to start phasing this out as it's only a thin +// wrapper around v8::BackingStore at this point. +class AllocatedBuffer { public: - explicit inline AllocatedBuffer(Environment* env = nullptr); - inline AllocatedBuffer(Environment* env, uv_buf_t buf); - inline ~AllocatedBuffer(); + AllocatedBuffer() = default; + inline AllocatedBuffer( + Environment* env, std::unique_ptr bs); + // For this constructor variant, `buffer` *must* come from an earlier call + // to .release(). + inline AllocatedBuffer(Environment* env, uv_buf_t buffer); inline void Resize(size_t len); inline uv_buf_t release(); @@ -575,16 +591,14 @@ struct AllocatedBuffer { inline v8::MaybeLocal ToBuffer(); inline v8::Local ToArrayBuffer(); - inline AllocatedBuffer(AllocatedBuffer&& other); - inline AllocatedBuffer& operator=(AllocatedBuffer&& other); + AllocatedBuffer(AllocatedBuffer&& other) = default; + AllocatedBuffer& operator=(AllocatedBuffer&& other) = default; AllocatedBuffer(const AllocatedBuffer& other) = delete; AllocatedBuffer& operator=(const AllocatedBuffer& other) = delete; private: - Environment* env_; - // We do not pass this to libuv directly, but uv_buf_t is a convenient way - // to represent a chunk of memory, and plays nicely with other parts of core. - uv_buf_t buffer_; + Environment* env_ = nullptr; + std::unique_ptr backing_store_; friend class Environment; }; @@ -959,11 +973,7 @@ class Environment : public MemoryRetainer { // Utilities that allocate memory using the Isolate's ArrayBuffer::Allocator. // In particular, using AllocateManaged() will provide a RAII-style object // with easy conversion to `Buffer` and `ArrayBuffer` objects. - inline AllocatedBuffer AllocateManaged(size_t size, bool checked = true); - inline char* Allocate(size_t size); - inline char* AllocateUnchecked(size_t size); - char* Reallocate(char* data, size_t old_size, size_t size); - inline void Free(char* data, size_t size); + inline AllocatedBuffer AllocateManaged(size_t size); inline bool printed_error() const; inline void set_printed_error(bool value); @@ -1251,6 +1261,9 @@ class Environment : public MemoryRetainer { void RunAndClearNativeImmediates(bool only_refed = false); void RunAndClearInterrupts(); + inline std::unordered_map>* + released_allocated_buffers(); + private: template inline void CreateImmediate(Fn&& cb, bool ref); @@ -1413,6 +1426,11 @@ class Environment : public MemoryRetainer { // We should probably find a way to just use plain `v8::String`s created from // the source passed to LoadEnvironment() directly instead. std::unique_ptr main_utf16_; + + // Used by AllocatedBuffer::release() to keep track of the BackingStore for + // a given pointer. + std::unordered_map> + released_allocated_buffers_; }; } // namespace node diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 1ff60ad721753e..3ddb7ffd847837 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -327,16 +327,7 @@ MaybeLocal New(Environment* env, size_t length) { return Local(); } - AllocatedBuffer ret(env); - if (length > 0) { - ret = env->AllocateManaged(length, false); - if (ret.data() == nullptr) { - THROW_ERR_MEMORY_ALLOCATION_FAILED(env); - return Local(); - } - } - - return scope.EscapeMaybe(ret.ToBuffer()); + return scope.EscapeMaybe(env->AllocateManaged(length).ToBuffer()); } @@ -363,14 +354,8 @@ MaybeLocal Copy(Environment* env, const char* data, size_t length) { return Local(); } - AllocatedBuffer ret(env); + AllocatedBuffer ret = env->AllocateManaged(length); if (length > 0) { - CHECK_NOT_NULL(data); - ret = env->AllocateManaged(length, false); - if (ret.data() == nullptr) { - THROW_ERR_MEMORY_ALLOCATION_FAILED(env); - return Local(); - } memcpy(ret.data(), data, length); } @@ -445,53 +430,23 @@ MaybeLocal New(Isolate* isolate, char* data, size_t length) { return MaybeLocal(); } Local obj; - if (Buffer::New(env, data, length, true).ToLocal(&obj)) + if (Buffer::New(env, data, length).ToLocal(&obj)) return handle_scope.Escape(obj); return Local(); } -// Warning: If this call comes through the public node_buffer.h API, -// the contract for this function is that `data` is allocated with malloc() +// The contract for this function is that `data` is allocated with malloc() // and not necessarily isolate's ArrayBuffer::Allocator. MaybeLocal New(Environment* env, char* data, - size_t length, - bool uses_malloc) { + size_t length) { if (length > 0) { CHECK_NOT_NULL(data); CHECK(length <= kMaxLength); } - if (uses_malloc) { - if (!env->isolate_data()->uses_node_allocator()) { - // We don't know for sure that the allocator is malloc()-based, so we need - // to fall back to the FreeCallback variant. - auto free_callback = [](char* data, void* hint) { free(data); }; - return New(env, data, length, free_callback, nullptr); - } else { - // This is malloc()-based, so we can acquire it into our own - // ArrayBufferAllocator. - CHECK_NOT_NULL(env->isolate_data()->node_allocator()); - env->isolate_data()->node_allocator()->RegisterPointer(data, length); - } - } - - auto callback = [](void* data, size_t length, void* deleter_data){ - CHECK_NOT_NULL(deleter_data); - - static_cast(deleter_data) - ->Free(data, length); - }; - std::unique_ptr backing = - v8::ArrayBuffer::NewBackingStore(data, - length, - callback, - env->isolate() - ->GetArrayBufferAllocator()); - Local ab = ArrayBuffer::New(env->isolate(), - std::move(backing)); - - return Buffer::New(env, ab, 0, length).FromMaybe(Local()); + auto free_callback = [](char* data, void* hint) { free(data); }; + return New(env, data, length, free_callback, nullptr); } namespace { diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d53a6d2f2325fe..8dab7784847f42 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4023,7 +4023,7 @@ bool CipherBase::Final(AllocatedBuffer* out) { bool ok; if (kind_ == kDecipher && mode == EVP_CIPH_CCM_MODE) { ok = !pending_auth_failed_; - *out = AllocatedBuffer(env()); // Empty buffer. + *out = env()->AllocateManaged(0); // Empty buffer. } else { int out_len = out->size(); ok = EVP_CipherFinal_ex(ctx_.get(), diff --git a/src/node_internals.h b/src/node_internals.h index 0ae17f71ecbcb2..5eac47906c3443 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -156,12 +156,10 @@ v8::MaybeLocal New(Environment* env, size_t length, void (*callback)(char* data, void* hint), void* hint); -// Takes ownership of |data|. Must allocate |data| with the current Isolate's -// ArrayBuffer::Allocator(). +// Takes ownership of |data|. |data| must be allocated with malloc(). v8::MaybeLocal New(Environment* env, char* data, - size_t length, - bool uses_malloc); + size_t length); // Creates a Buffer instance over an existing ArrayBuffer. v8::MaybeLocal New(Environment* env, v8::Local ab, @@ -183,7 +181,7 @@ static v8::MaybeLocal New(Environment* env, const size_t len_in_bytes = buf->length() * sizeof(buf->out()[0]); if (buf->IsAllocated()) - ret = New(env, src, len_in_bytes, true); + ret = New(env, src, len_in_bytes); else if (!buf->IsInvalidated()) ret = Copy(env, src, len_in_bytes); diff --git a/src/node_serdes.cc b/src/node_serdes.cc index 86fb822dd5bfa9..c7877215911f8e 100644 --- a/src/node_serdes.cc +++ b/src/node_serdes.cc @@ -206,8 +206,7 @@ void SerializerContext::ReleaseBuffer(const FunctionCallbackInfo& args) { std::pair ret = ctx->serializer_.Release(); auto buf = Buffer::New(ctx->env(), reinterpret_cast(ret.first), - ret.second, - true /* uses_malloc */); + ret.second); if (!buf.IsEmpty()) { args.GetReturnValue().Set(buf.ToLocalChecked());