From 1c61aba3e6d6a81082f0f7b00cc1bfec0b68e7c4 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 7 May 2020 13:31:56 -0700 Subject: [PATCH 1/3] src: extract AllocatedBuffer from env.h Cleanup up env.h by removing things that are not specific to `Environment`. --- node.gyp | 2 + src/allocated_buffer-inl.h | 110 +++++++++++++++++++++++++++++++++++++ src/allocated_buffer.h | 63 +++++++++++++++++++++ src/env-inl.h | 88 ----------------------------- src/env.cc | 2 +- src/env.h | 37 +------------ src/inspector_io.cc | 1 + src/node.cc | 1 + src/node_buffer.cc | 13 ++++- src/node_crypto.cc | 37 +++++++------ src/node_crypto.h | 1 + src/node_crypto_bio.cc | 1 + src/node_crypto_common.cc | 7 ++- src/node_http2.cc | 14 +++-- src/node_http2.h | 1 + src/stream_base-inl.h | 1 + src/stream_base.cc | 9 +-- src/stream_base.h | 1 + src/stream_pipe.cc | 3 +- src/stream_pipe.h | 1 + src/tls_wrap.cc | 5 +- src/tls_wrap.h | 1 + src/udp_wrap.cc | 3 +- 23 files changed, 241 insertions(+), 161 deletions(-) create mode 100644 src/allocated_buffer-inl.h create mode 100644 src/allocated_buffer.h diff --git a/node.gyp b/node.gyp index b0927572312521..94b6846b2de54d 100644 --- a/node.gyp +++ b/node.gyp @@ -643,6 +643,8 @@ 'src/aliased_buffer.h', 'src/aliased_struct.h', 'src/aliased_struct-inl.h', + 'src/allocated_buffer.h', + 'src/allocated_buffer-inl.h' 'src/async_wrap.h', 'src/async_wrap-inl.h', 'src/base_object.h', diff --git a/src/allocated_buffer-inl.h b/src/allocated_buffer-inl.h new file mode 100644 index 00000000000000..22956b2f2c1a5c --- /dev/null +++ b/src/allocated_buffer-inl.h @@ -0,0 +1,110 @@ +#ifndef SRC_ALLOCATED_BUFFER_INL_H_ +#define SRC_ALLOCATED_BUFFER_INL_H_ + +#include "allocated_buffer.h" +#include "base_object-inl.h" +#include "node_buffer.h" +#include "env-inl.h" +#include "uv.h" +#include "v8.h" +#include "util-inl.h" +#include "node_internals.h" + +namespace node { + +AllocatedBuffer AllocatedBuffer::AllocateManaged( + Environment* env, + size_t size, + int flags) { + char* data = flags & ALLOCATE_MANAGED_UNCHECKED ? + env->AllocateUnchecked(size) : + env->Allocate(size); + if (data == nullptr) size = 0; + return AllocatedBuffer(env, uv_buf_init(data, size)); +} + +inline AllocatedBuffer::AllocatedBuffer(Environment* env, uv_buf_t buf) + : env_(env), buffer_(buf) {} + +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); +} + +inline uv_buf_t AllocatedBuffer::release() { + uv_buf_t ret = buffer_; + buffer_ = uv_buf_init(nullptr, 0); + return ret; +} + +inline char* AllocatedBuffer::data() { + return buffer_.base; +} + +inline const char* AllocatedBuffer::data() const { + return buffer_.base; +} + +inline size_t AllocatedBuffer::size() const { + return buffer_.len; +} + +inline AllocatedBuffer::AllocatedBuffer(Environment* env) + : env_(env), buffer_(uv_buf_init(nullptr, 0)) {} + +inline AllocatedBuffer::AllocatedBuffer(AllocatedBuffer&& other) + : AllocatedBuffer() { + *this = std::move(other); +} + +inline AllocatedBuffer& AllocatedBuffer::operator=(AllocatedBuffer&& other) { + clear(); + env_ = other.env_; + buffer_ = other.release(); + return *this; +} + +inline AllocatedBuffer::~AllocatedBuffer() { + clear(); +} + +inline void AllocatedBuffer::clear() { + uv_buf_t buf = release(); + if (buf.base != nullptr) { + CHECK_NOT_NULL(env_); + env_->Free(buf.base, buf.len); + } +} + +inline v8::MaybeLocal AllocatedBuffer::ToBuffer() { + CHECK_NOT_NULL(env_); + v8::MaybeLocal obj = Buffer::New(env_, data(), size(), false); + if (!obj.IsEmpty()) release(); + return obj; +} + +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)); +} + +} // namespace node + +#endif // SRC_ALLOCATED_BUFFER_INL_H_ diff --git a/src/allocated_buffer.h b/src/allocated_buffer.h new file mode 100644 index 00000000000000..b54f74b2d278ff --- /dev/null +++ b/src/allocated_buffer.h @@ -0,0 +1,63 @@ +#ifndef SRC_ALLOCATED_BUFFER_H_ +#define SRC_ALLOCATED_BUFFER_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "base_object.h" +#include "uv.h" +#include "v8.h" + +namespace node { + +class Environment; + +// A unique-pointer-ish object that is compatible with the JS engine's +// ArrayBuffer::Allocator. +struct AllocatedBuffer { + public: + enum AllocateManagedFlags { + ALLOCATE_MANAGED_FLAG_NONE, + ALLOCATE_MANAGED_UNCHECKED + }; + + // 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 static AllocatedBuffer AllocateManaged( + Environment* env, + size_t size, + int flags = ALLOCATE_MANAGED_FLAG_NONE); + + explicit inline AllocatedBuffer(Environment* env = nullptr); + inline AllocatedBuffer(Environment* env, uv_buf_t buf); + inline ~AllocatedBuffer(); + inline void Resize(size_t len); + + inline uv_buf_t release(); + inline char* data(); + inline const char* data() const; + inline size_t size() const; + inline void clear(); + + inline v8::MaybeLocal ToBuffer(); + inline v8::Local ToArrayBuffer(); + + inline AllocatedBuffer(AllocatedBuffer&& other); + inline AllocatedBuffer& operator=(AllocatedBuffer&& other); + 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_; + + friend class Environment; +}; + +} // namespace node + +#endif // NODE_WANT_INTERNALS + +#endif // SRC_ALLOCATED_BUFFER_H_ diff --git a/src/env-inl.h b/src/env-inl.h index 97ccc24f809e5e..3afda388bc9e2a 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -883,68 +883,6 @@ inline void Environment::Free(char* data, size_t size) { 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)); -} - -inline AllocatedBuffer::AllocatedBuffer(Environment* env, uv_buf_t buf) - : env_(env), buffer_(buf) {} - -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); -} - -inline uv_buf_t AllocatedBuffer::release() { - uv_buf_t ret = buffer_; - buffer_ = uv_buf_init(nullptr, 0); - return ret; -} - -inline char* AllocatedBuffer::data() { - return buffer_.base; -} - -inline const char* AllocatedBuffer::data() const { - return buffer_.base; -} - -inline size_t AllocatedBuffer::size() const { - return buffer_.len; -} - -inline AllocatedBuffer::AllocatedBuffer(Environment* env) - : env_(env), buffer_(uv_buf_init(nullptr, 0)) {} - -inline AllocatedBuffer::AllocatedBuffer(AllocatedBuffer&& other) - : AllocatedBuffer() { - *this = std::move(other); -} - -inline AllocatedBuffer& AllocatedBuffer::operator=(AllocatedBuffer&& other) { - clear(); - env_ = other.env_; - buffer_ = other.release(); - return *this; -} - -inline AllocatedBuffer::~AllocatedBuffer() { - clear(); -} - -inline void AllocatedBuffer::clear() { - uv_buf_t buf = release(); - if (buf.base != nullptr) { - CHECK_NOT_NULL(env_); - env_->Free(buf.base, buf.len); - } -} - // It's a bit awkward to define this Buffer::New() overload here, but it // avoids a circular dependency with node_internals.h. namespace Buffer { @@ -954,32 +892,6 @@ v8::MaybeLocal New(Environment* env, bool uses_malloc); } -inline v8::MaybeLocal AllocatedBuffer::ToBuffer() { - CHECK_NOT_NULL(env_); - v8::MaybeLocal obj = Buffer::New(env_, data(), size(), false); - if (!obj.IsEmpty()) release(); - return obj; -} - -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)); -} - inline void Environment::ThrowError(const char* errmsg) { ThrowError(v8::Exception::Error, errmsg); } diff --git a/src/env.cc b/src/env.cc index 4e7c975902b091..a28679728e5565 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1,5 +1,5 @@ #include "env.h" - +#include "allocated_buffer-inl.h" #include "async_wrap.h" #include "base_object-inl.h" #include "debug_utils-inl.h" diff --git a/src/env.h b/src/env.h index 1ef085ee867f38..eb6e63dc3fc46f 100644 --- a/src/env.h +++ b/src/env.h @@ -472,6 +472,7 @@ constexpr size_t kFsStatsBufferLength = V(url_constructor_function, v8::Function) class Environment; +struct AllocatedBuffer; class IsolateData : public MemoryRetainer { public: @@ -558,38 +559,6 @@ struct ContextInfo { class EnabledDebugList; -// A unique-pointer-ish object that is compatible with the JS engine's -// ArrayBuffer::Allocator. -struct AllocatedBuffer { - public: - explicit inline AllocatedBuffer(Environment* env = nullptr); - inline AllocatedBuffer(Environment* env, uv_buf_t buf); - inline ~AllocatedBuffer(); - inline void Resize(size_t len); - - inline uv_buf_t release(); - inline char* data(); - inline const char* data() const; - inline size_t size() const; - inline void clear(); - - inline v8::MaybeLocal ToBuffer(); - inline v8::Local ToArrayBuffer(); - - inline AllocatedBuffer(AllocatedBuffer&& other); - inline AllocatedBuffer& operator=(AllocatedBuffer&& other); - 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_; - - friend class Environment; -}; - class KVStore { public: KVStore() = default; @@ -957,10 +926,6 @@ class Environment : public MemoryRetainer { inline IsolateData* isolate_data() const; - // 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); diff --git a/src/inspector_io.cc b/src/inspector_io.cc index 75290317d2fcae..d3bd1911214f83 100644 --- a/src/inspector_io.cc +++ b/src/inspector_io.cc @@ -3,6 +3,7 @@ #include "inspector_socket_server.h" #include "inspector/main_thread_interface.h" #include "inspector/node_string.h" +#include "allocated_buffer-inl.h" // Inlined functions needed by node_crypto.h. #include "base_object-inl.h" #include "debug_utils-inl.h" #include "node.h" diff --git a/src/node.cc b/src/node.cc index bc8017bdd06029..3895b2b4076e0c 100644 --- a/src/node.cc +++ b/src/node.cc @@ -41,6 +41,7 @@ #include "node_version.h" #if HAVE_OPENSSL +#include "allocated_buffer-inl.h" // Inlined functions needed by node_crypto.h #include "node_crypto.h" #endif diff --git a/src/node_buffer.cc b/src/node_buffer.cc index fd20415936e169..398d58608493dc 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -20,6 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. #include "node_buffer.h" +#include "allocated_buffer-inl.h" #include "node.h" #include "node_errors.h" #include "node_internals.h" @@ -350,7 +351,10 @@ MaybeLocal New(Environment* env, size_t length) { AllocatedBuffer ret(env); if (length > 0) { - ret = env->AllocateManaged(length, false); + ret = AllocatedBuffer::AllocateManaged( + env, + length, + AllocatedBuffer::ALLOCATE_MANAGED_UNCHECKED); if (ret.data() == nullptr) { THROW_ERR_MEMORY_ALLOCATION_FAILED(env); return Local(); @@ -387,7 +391,10 @@ MaybeLocal Copy(Environment* env, const char* data, size_t length) { AllocatedBuffer ret(env); if (length > 0) { CHECK_NOT_NULL(data); - ret = env->AllocateManaged(length, false); + ret = AllocatedBuffer::AllocateManaged( + env, + length, + AllocatedBuffer::ALLOCATE_MANAGED_UNCHECKED); if (ret.data() == nullptr) { THROW_ERR_MEMORY_ALLOCATION_FAILED(env); return Local(); @@ -1107,7 +1114,7 @@ static void EncodeUtf8String(const FunctionCallbackInfo& args) { Local str = args[0].As(); size_t length = str->Utf8Length(isolate); - AllocatedBuffer buf = env->AllocateManaged(length); + AllocatedBuffer buf = AllocatedBuffer::AllocateManaged(env, length); str->WriteUtf8(isolate, buf.data(), -1, // We are certain that `data` is sufficiently large diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 22d8f66339860e..fe9ac2d0cbf076 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -28,6 +28,7 @@ #include "node_errors.h" #include "node_mutex.h" #include "node_process.h" +#include "allocated_buffer-inl.h" #include "tls_wrap.h" // TLSWrap #include "async_wrap-inl.h" @@ -1918,7 +1919,7 @@ void SSLWrap::GetFinished(const FunctionCallbackInfo& args) { if (len == 0) return; - AllocatedBuffer buf = env->AllocateManaged(len); + AllocatedBuffer buf = AllocatedBuffer::AllocateManaged(env, len); CHECK_EQ(len, SSL_get_finished(w->ssl_.get(), buf.data(), len)); args.GetReturnValue().Set(buf.ToBuffer().ToLocalChecked()); } @@ -1941,7 +1942,7 @@ void SSLWrap::GetPeerFinished(const FunctionCallbackInfo& args) { if (len == 0) return; - AllocatedBuffer buf = env->AllocateManaged(len); + AllocatedBuffer buf = AllocatedBuffer::AllocateManaged(env, len); CHECK_EQ(len, SSL_get_peer_finished(w->ssl_.get(), buf.data(), len)); args.GetReturnValue().Set(buf.ToBuffer().ToLocalChecked()); } @@ -1962,7 +1963,7 @@ void SSLWrap::GetSession(const FunctionCallbackInfo& args) { if (slen <= 0) return; // Invalid or malformed session. - AllocatedBuffer sbuf = env->AllocateManaged(slen); + AllocatedBuffer sbuf = AllocatedBuffer::AllocateManaged(env, slen); unsigned char* p = reinterpret_cast(sbuf.data()); CHECK_LT(0, i2d_SSL_SESSION(sess, &p)); args.GetReturnValue().Set(sbuf.ToBuffer().ToLocalChecked()); @@ -2272,7 +2273,7 @@ void SSLWrap::ExportKeyingMaterial( uint32_t olen = args[0].As()->Value(); node::Utf8Value label(env->isolate(), args[1]); - AllocatedBuffer out = env->AllocateManaged(olen); + AllocatedBuffer out = AllocatedBuffer::AllocateManaged(env, olen); ByteSource context; bool use_context = !args[2]->IsUndefined(); @@ -3950,7 +3951,7 @@ CipherBase::UpdateResult CipherBase::Update(const char* data, return kErrorState; } - *out = env()->AllocateManaged(buf_len); + *out = AllocatedBuffer::AllocateManaged(env(), buf_len); int r = EVP_CipherUpdate(ctx_.get(), reinterpret_cast(out->data()), &buf_len, @@ -4014,7 +4015,8 @@ bool CipherBase::Final(AllocatedBuffer* out) { const int mode = EVP_CIPHER_CTX_mode(ctx_.get()); - *out = env()->AllocateManaged( + *out = AllocatedBuffer::AllocateManaged( + env(), static_cast(EVP_CIPHER_CTX_block_size(ctx_.get()))); if (kind_ == kDecipher && IsSupportedAuthenticatedMode(ctx_.get())) { @@ -4535,7 +4537,7 @@ static AllocatedBuffer ConvertSignatureToP1363(Environment* env, if (!asn1_sig) return AllocatedBuffer(); - AllocatedBuffer buf = env->AllocateManaged(2 * n); + AllocatedBuffer buf = AllocatedBuffer::AllocateManaged(env, 2 * n); unsigned char* data = reinterpret_cast(buf.data()); const BIGNUM* r = ECDSA_SIG_get0_r(asn1_sig.get()); @@ -4594,7 +4596,7 @@ static AllocatedBuffer Node_SignFinal(Environment* env, int signed_sig_len = EVP_PKEY_size(pkey.get()); CHECK_GE(signed_sig_len, 0); size_t sig_len = static_cast(signed_sig_len); - AllocatedBuffer sig = env->AllocateManaged(sig_len); + AllocatedBuffer sig = AllocatedBuffer::AllocateManaged(env, sig_len); EVPKeyCtxPointer pkctx(EVP_PKEY_CTX_new(pkey.get(), nullptr)); if (pkctx && @@ -4755,7 +4757,7 @@ void SignOneShot(const FunctionCallbackInfo& args) { if (!EVP_DigestSign(mdctx.get(), nullptr, &sig_len, input, data.length())) return CheckThrow(env, SignBase::Error::kSignPrivateKey); - AllocatedBuffer signature = env->AllocateManaged(sig_len); + AllocatedBuffer signature = AllocatedBuffer::AllocateManaged(env, sig_len); if (!EVP_DigestSign(mdctx.get(), reinterpret_cast(signature.data()), &sig_len, @@ -5012,7 +5014,7 @@ bool PublicKeyCipher::Cipher(Environment* env, if (EVP_PKEY_cipher(ctx.get(), nullptr, &out_len, data, len) <= 0) return false; - *out = env->AllocateManaged(out_len); + *out = AllocatedBuffer::AllocateManaged(env, out_len); if (EVP_PKEY_cipher(ctx.get(), reinterpret_cast(out->data()), @@ -5264,7 +5266,7 @@ void DiffieHellman::GenerateKeys(const FunctionCallbackInfo& args) { DH_get0_key(diffieHellman->dh_.get(), &pub_key, nullptr); const int size = BN_num_bytes(pub_key); CHECK_GE(size, 0); - AllocatedBuffer data = env->AllocateManaged(size); + AllocatedBuffer data = AllocatedBuffer::AllocateManaged(env, size); CHECK_EQ(size, BN_bn2binpad( pub_key, reinterpret_cast(data.data()), size)); @@ -5285,7 +5287,7 @@ void DiffieHellman::GetField(const FunctionCallbackInfo& args, const int size = BN_num_bytes(num); CHECK_GE(size, 0); - AllocatedBuffer data = env->AllocateManaged(size); + AllocatedBuffer data = AllocatedBuffer::AllocateManaged(env, size); CHECK_EQ( size, BN_bn2binpad(num, reinterpret_cast(data.data()), size)); @@ -5355,7 +5357,8 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo& args) { ArrayBufferViewContents key_buf(args[0].As()); BignumPointer key(BN_bin2bn(key_buf.data(), key_buf.length(), nullptr)); - AllocatedBuffer ret = env->AllocateManaged(DH_size(diffieHellman->dh_.get())); + AllocatedBuffer ret = + AllocatedBuffer::AllocateManaged(env, DH_size(diffieHellman->dh_.get())); int size = DH_compute_key(reinterpret_cast(ret.data()), key.get(), @@ -5561,7 +5564,7 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo& args) { // NOTE: field_size is in bits int field_size = EC_GROUP_get_degree(ecdh->group_); size_t out_len = (field_size + 7) / 8; - AllocatedBuffer out = env->AllocateManaged(out_len); + AllocatedBuffer out = AllocatedBuffer::AllocateManaged(env, out_len); int r = ECDH_compute_key( out.data(), out_len, pub.get(), ecdh->key_.get(), nullptr); @@ -5610,7 +5613,7 @@ void ECDH::GetPrivateKey(const FunctionCallbackInfo& args) { return env->ThrowError("Failed to get ECDH private key"); const int size = BN_num_bytes(b); - AllocatedBuffer out = env->AllocateManaged(size); + AllocatedBuffer out = AllocatedBuffer::AllocateManaged(env, size); CHECK_EQ(size, BN_bn2binpad(b, reinterpret_cast(out.data()), size)); @@ -6595,7 +6598,7 @@ AllocatedBuffer ExportPublicKey(Environment* env, BIO_get_mem_ptr(bio.get(), &ptr); *size = ptr->length; - AllocatedBuffer buf = env->AllocateManaged(*size); + AllocatedBuffer buf = AllocatedBuffer::AllocateManaged(env, *size); memcpy(buf.data(), ptr->data, *size); return buf; @@ -6704,7 +6707,7 @@ AllocatedBuffer StatelessDiffieHellman(Environment* env, ManagedEVPPKey our_key, EVP_PKEY_derive(ctx.get(), nullptr, &out_size) <= 0) return AllocatedBuffer(); - AllocatedBuffer result = env->AllocateManaged(out_size); + AllocatedBuffer result = AllocatedBuffer::AllocateManaged(env, out_size); CHECK_NOT_NULL(result.data()); unsigned char* data = reinterpret_cast(result.data()); diff --git a/src/node_crypto.h b/src/node_crypto.h index 772a34a7da7699..1eea1239f1956d 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -27,6 +27,7 @@ // ClientHelloParser #include "node_crypto_clienthello.h" +#include "allocated_buffer.h" #include "env.h" #include "base_object.h" #include "util.h" diff --git a/src/node_crypto_bio.cc b/src/node_crypto_bio.cc index 55f5e8a5a37650..8c58e31f86b3fa 100644 --- a/src/node_crypto_bio.cc +++ b/src/node_crypto_bio.cc @@ -21,6 +21,7 @@ #include "base_object-inl.h" #include "memory_tracker-inl.h" +#include "allocated_buffer-inl.h" // Inlined functions needed by node_crypto.h. #include "node_crypto_bio.h" #include "openssl/bio.h" #include "util-inl.h" diff --git a/src/node_crypto_common.cc b/src/node_crypto_common.cc index 3b35ee1ff7ba8a..d88074dfe83c53 100644 --- a/src/node_crypto_common.cc +++ b/src/node_crypto_common.cc @@ -1,3 +1,4 @@ +#include "allocated_buffer-inl.h" #include "base_object-inl.h" #include "env-inl.h" #include "node_buffer.h" @@ -492,7 +493,7 @@ MaybeLocal GetLastIssuedCert( MaybeLocal GetRawDERCertificate(Environment* env, X509* cert) { int size = i2d_X509(cert, nullptr); - AllocatedBuffer buffer = env->AllocateManaged(size); + AllocatedBuffer buffer = AllocatedBuffer::AllocateManaged(env, size); unsigned char* serialized = reinterpret_cast(buffer.data()); i2d_X509(cert, &serialized); @@ -669,7 +670,7 @@ MaybeLocal GetPubKey(Environment* env, const RSAPointer& rsa) { int size = i2d_RSA_PUBKEY(rsa.get(), nullptr); CHECK_GE(size, 0); - AllocatedBuffer buffer = env->AllocateManaged(size); + AllocatedBuffer buffer = AllocatedBuffer::AllocateManaged(env, size); unsigned char* serialized = reinterpret_cast(buffer.data()); i2d_RSA_PUBKEY(rsa.get(), &serialized); @@ -891,7 +892,7 @@ MaybeLocal ECPointToBuffer(Environment* env, if (error != nullptr) *error = "Failed to get public key length"; return MaybeLocal(); } - AllocatedBuffer buf = env->AllocateManaged(len); + AllocatedBuffer buf = AllocatedBuffer::AllocateManaged(env, len); len = EC_POINT_point2oct(group, point, form, diff --git a/src/node_http2.cc b/src/node_http2.cc index 067e73b043b3d6..72901083a427fc 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1,4 +1,5 @@ #include "aliased_buffer.h" +#include "allocated_buffer-inl.h" #include "aliased_struct-inl.h" #include "debug_utils-inl.h" #include "memory_tracker-inl.h" @@ -259,7 +260,7 @@ Local Http2Settings::Pack( const nghttp2_settings_entry* entries) { EscapableHandleScope scope(env->isolate()); const size_t size = count * 6; - AllocatedBuffer buffer = env->AllocateManaged(size); + AllocatedBuffer buffer = AllocatedBuffer::AllocateManaged(env, size); ssize_t ret = nghttp2_pack_settings_payload( reinterpret_cast(buffer.data()), @@ -359,8 +360,10 @@ Origins::Origins( return; } - buf_ = env->AllocateManaged((alignof(nghttp2_origin_entry) - 1) + - count_ * sizeof(nghttp2_origin_entry) + + buf_ = AllocatedBuffer::AllocateManaged( + env, + (alignof(nghttp2_origin_entry) - 1) + + count_ * sizeof(nghttp2_origin_entry) + origin_string_len); // Make sure the start address is aligned appropriately for an nghttp2_nv*. @@ -1725,7 +1728,7 @@ Http2Stream* Http2Session::SubmitRequest( } uv_buf_t Http2Session::OnStreamAlloc(size_t suggested_size) { - return env()->AllocateManaged(suggested_size).release(); + return AllocatedBuffer::AllocateManaged(env(), suggested_size).release(); } // Callback used to receive inbound data from the i/o stream @@ -1757,7 +1760,8 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { // happen, we concatenate the data we received with the already-stored // pending input data, slicing off the already processed part. size_t pending_len = stream_buf_.len - stream_buf_offset_; - AllocatedBuffer new_buf = env()->AllocateManaged(pending_len + nread); + AllocatedBuffer new_buf = + AllocatedBuffer::AllocateManaged(env(), pending_len + nread); memcpy(new_buf.data(), stream_buf_.base + stream_buf_offset_, pending_len); memcpy(new_buf.data() + pending_len, buf.data(), nread); diff --git a/src/node_http2.h b/src/node_http2.h index 6b11535f84e121..4ef9e9fbab0c43 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -7,6 +7,7 @@ #include #include "nghttp2/nghttp2.h" +#include "allocated_buffer.h" #include "aliased_struct.h" #include "node_http2_state.h" #include "node_http_common.h" diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 149f542d841607..bd7224e9c0245e 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -3,6 +3,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "allocated_buffer-inl.h" #include "async_wrap-inl.h" #include "base_object-inl.h" #include "node.h" diff --git a/src/stream_base.cc b/src/stream_base.cc index d8a191dc2e5f6d..b35df39afe9d8c 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -1,6 +1,7 @@ #include "stream_base.h" // NOLINT(build/include_inline) #include "stream_base-inl.h" #include "stream_wrap.h" +#include "allocated_buffer-inl.h" #include "node.h" #include "node_buffer.h" @@ -132,7 +133,7 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { AllocatedBuffer storage; if (storage_size > 0) - storage = env->AllocateManaged(storage_size); + storage = AllocatedBuffer::AllocateManaged(env, storage_size); offset = 0; if (!all_buffers) { @@ -276,12 +277,12 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { if (try_write) { // Copy partial data - data = env->AllocateManaged(buf.len); + data = AllocatedBuffer::AllocateManaged(env, buf.len); memcpy(data.data(), buf.base, buf.len); data_size = buf.len; } else { // Write it - data = env->AllocateManaged(storage_size); + data = AllocatedBuffer::AllocateManaged(env, storage_size); data_size = StringBytes::Write(env->isolate(), data.data(), storage_size, @@ -486,7 +487,7 @@ void StreamResource::ClearError() { uv_buf_t EmitToJSStreamListener::OnStreamAlloc(size_t suggested_size) { CHECK_NOT_NULL(stream_); Environment* env = static_cast(stream_)->stream_env(); - return env->AllocateManaged(suggested_size).release(); + return AllocatedBuffer::AllocateManaged(env, suggested_size).release(); } void EmitToJSStreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { diff --git a/src/stream_base.h b/src/stream_base.h index 176271e05a8c39..7c6bcba81edd03 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -4,6 +4,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "env.h" +#include "allocated_buffer.h" #include "async_wrap.h" #include "node.h" #include "util.h" diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc index 1bed4514eb8dd8..1422f9e0ea548e 100644 --- a/src/stream_pipe.cc +++ b/src/stream_pipe.cc @@ -1,4 +1,5 @@ #include "stream_pipe.h" +#include "allocated_buffer-inl.h" #include "stream_base-inl.h" #include "node_buffer.h" #include "util-inl.h" @@ -118,7 +119,7 @@ uv_buf_t StreamPipe::ReadableListener::OnStreamAlloc(size_t suggested_size) { StreamPipe* pipe = ContainerOf(&StreamPipe::readable_listener_, this); size_t size = std::min(suggested_size, pipe->wanted_data_); CHECK_GT(size, 0); - return pipe->env()->AllocateManaged(size).release(); + return AllocatedBuffer::AllocateManaged(pipe->env(), size).release(); } void StreamPipe::ReadableListener::OnStreamRead(ssize_t nread, diff --git a/src/stream_pipe.h b/src/stream_pipe.h index e22abab0115c8a..36179c95f74a93 100644 --- a/src/stream_pipe.h +++ b/src/stream_pipe.h @@ -4,6 +4,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "stream_base.h" +#include "allocated_buffer.h" namespace node { diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 8fa1bd2eea0116..792d3ea79ceea9 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -20,6 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. #include "tls_wrap.h" +#include "allocated_buffer-inl.h" #include "async_wrap-inl.h" #include "debug_utils-inl.h" #include "memory_tracker-inl.h" @@ -766,7 +767,7 @@ int TLSWrap::DoWrite(WriteWrap* w, // and copying it when it could just be used. if (nonempty_count != 1) { - data = env()->AllocateManaged(length); + data = AllocatedBuffer::AllocateManaged(env(), length); size_t offset = 0; for (i = 0; i < count; i++) { memcpy(data.data() + offset, bufs[i].base, bufs[i].len); @@ -782,7 +783,7 @@ int TLSWrap::DoWrite(WriteWrap* w, written = SSL_write(ssl_.get(), buf->base, buf->len); if (written == -1) { - data = env()->AllocateManaged(length); + data = AllocatedBuffer::AllocateManaged(env(), length); memcpy(data.data(), buf->base, buf->len); } } diff --git a/src/tls_wrap.h b/src/tls_wrap.h index 7bb33b4a3cb69a..7b8e50de9d4689 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -26,6 +26,7 @@ #include "node_crypto.h" // SSLWrap +#include "allocated_buffer.h" #include "async_wrap.h" #include "stream_wrap.h" #include "v8.h" diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 9013bc9fe335c3..eb02db0a2a45a7 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -20,6 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. #include "udp_wrap.h" +#include "allocated_buffer-inl.h" #include "env-inl.h" #include "node_buffer.h" #include "node_sockaddr-inl.h" @@ -688,7 +689,7 @@ void UDPWrap::OnAlloc(uv_handle_t* handle, } uv_buf_t UDPWrap::OnAlloc(size_t suggested_size) { - return env()->AllocateManaged(suggested_size).release(); + return AllocatedBuffer::AllocateManaged(env(), suggested_size).release(); } void UDPWrap::OnRecv(uv_udp_t* handle, From 9f8dc2fa386d366634e02be44168f980c748fe30 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 14 May 2020 12:03:59 -0700 Subject: [PATCH 2/3] src: turn AllocatedBuffer into thin wrapper around v8::BackingStore Alternative to https://github.com/nodejs/node/pull/33381 that reimplements that change on top of moving AllocatedBuffer out of env.h --- src/allocated_buffer-inl.h | 128 ++++++++++++++++++------------------- src/allocated_buffer.h | 48 ++++++++------ src/env-inl.h | 26 +------- src/env.cc | 19 ------ src/env.h | 13 ++-- src/node_buffer.cc | 66 +++---------------- src/node_crypto.cc | 2 +- src/node_internals.h | 5 +- src/node_serdes.cc | 3 +- 9 files changed, 116 insertions(+), 194 deletions(-) diff --git a/src/allocated_buffer-inl.h b/src/allocated_buffer-inl.h index 22956b2f2c1a5c..e10aa08e7f087f 100644 --- a/src/allocated_buffer-inl.h +++ b/src/allocated_buffer-inl.h @@ -12,97 +12,97 @@ namespace node { +// 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, + v8::Local ab, + size_t byte_offset, + size_t length); +} + +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; +} + AllocatedBuffer AllocatedBuffer::AllocateManaged( Environment* env, - size_t size, - int flags) { - char* data = flags & ALLOCATE_MANAGED_UNCHECKED ? - env->AllocateUnchecked(size) : - env->Allocate(size); - if (data == nullptr) size = 0; - return AllocatedBuffer(env, uv_buf_init(data, size)); + size_t size) { + NoArrayBufferZeroFillScope no_zero_fill_scope(env->isolate_data()); + std::unique_ptr bs = + v8::ArrayBuffer::NewBackingStore(env->isolate(), size); + return AllocatedBuffer(env, std::move(bs)); } -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)) {} + +AllocatedBuffer::AllocatedBuffer( + Environment* env, uv_buf_t buffer) + : env_(env) { + if (buffer.base == nullptr) return; + 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 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); +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 uv_buf_t AllocatedBuffer::release() { - uv_buf_t ret = buffer_; - buffer_ = uv_buf_init(nullptr, 0); + if (data() == nullptr) return uv_buf_init(nullptr, 0); + + 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 char* AllocatedBuffer::data() { - return buffer_.base; + if (!backing_store_) return nullptr; + return static_cast(backing_store_->Data()); } inline const char* AllocatedBuffer::data() const { - return buffer_.base; + if (!backing_store_) return nullptr; + return static_cast(backing_store_->Data()); } -inline size_t AllocatedBuffer::size() const { - return buffer_.len; -} - -inline AllocatedBuffer::AllocatedBuffer(Environment* env) - : env_(env), buffer_(uv_buf_init(nullptr, 0)) {} - -inline AllocatedBuffer::AllocatedBuffer(AllocatedBuffer&& other) - : AllocatedBuffer() { - *this = std::move(other); -} - -inline AllocatedBuffer& AllocatedBuffer::operator=(AllocatedBuffer&& other) { - clear(); - env_ = other.env_; - buffer_ = other.release(); - return *this; -} -inline AllocatedBuffer::~AllocatedBuffer() { - clear(); +inline 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); - } + backing_store_.reset(); } 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_)); } } // namespace node diff --git a/src/allocated_buffer.h b/src/allocated_buffer.h index b54f74b2d278ff..c984a342a8b2b8 100644 --- a/src/allocated_buffer.h +++ b/src/allocated_buffer.h @@ -6,31 +6,43 @@ #include "base_object.h" #include "uv.h" #include "v8.h" +#include "env.h" namespace node { class Environment; +// Disables zero-filling for ArrayBuffer allocations in this scope. This is +// similar to how we implement Buffer.allocUnsafe() in JS land. +class NoArrayBufferZeroFillScope{ + public: + inline explicit NoArrayBufferZeroFillScope(IsolateData* isolate_data); + inline ~NoArrayBufferZeroFillScope(); + + private: + NodeArrayBufferAllocator* node_allocator_; + + friend class Environment; +}; + // A unique-pointer-ish object that is compatible with the JS engine's // ArrayBuffer::Allocator. +// TODO(addaleax): We may want to start phasing this out as it's only a +// thin wrapper around v8::BackingStore at this point struct AllocatedBuffer { public: - enum AllocateManagedFlags { - ALLOCATE_MANAGED_FLAG_NONE, - ALLOCATE_MANAGED_UNCHECKED - }; - // 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 static AllocatedBuffer AllocateManaged( - Environment* env, - size_t size, - int flags = ALLOCATE_MANAGED_FLAG_NONE); - - explicit inline AllocatedBuffer(Environment* env = nullptr); - inline AllocatedBuffer(Environment* env, uv_buf_t buf); - inline ~AllocatedBuffer(); + inline static AllocatedBuffer AllocateManaged(Environment* env, size_t size); + + 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(); @@ -42,16 +54,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; }; diff --git a/src/env-inl.h b/src/env-inl.h index 3afda388bc9e2a..fe92fcb3dd91cd 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -867,29 +867,9 @@ inline IsolateData* Environment::isolate_data() const { return isolate_data_; } -inline char* Environment::AllocateUnchecked(size_t size) { - return static_cast( - isolate_data()->allocator()->AllocateUninitialized(size)); -} - -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); -} - -// 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); +std::unordered_map>* + Environment::released_allocated_buffers() { + return &released_allocated_buffers_; } inline void Environment::ThrowError(const char* errmsg) { diff --git a/src/env.cc b/src/env.cc index a28679728e5565..7591ed1c435d82 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1104,25 +1104,6 @@ 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(); } diff --git a/src/env.h b/src/env.h index eb6e63dc3fc46f..dafb59c0460455 100644 --- a/src/env.h +++ b/src/env.h @@ -926,11 +926,6 @@ class Environment : public MemoryRetainer { inline IsolateData* isolate_data() const; - 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 bool printed_error() const; inline void set_printed_error(bool value); @@ -1218,6 +1213,9 @@ class Environment : public MemoryRetainer { void RunAndClearNativeImmediates(bool only_refed = false); void RunAndClearInterrupts(); + inline std::unordered_map>* + released_allocated_buffers(); + private: inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); @@ -1378,6 +1376,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 398d58608493dc..75d38289546820 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -349,19 +349,8 @@ MaybeLocal New(Environment* env, size_t length) { return Local(); } - AllocatedBuffer ret(env); - if (length > 0) { - ret = AllocatedBuffer::AllocateManaged( - env, - length, - AllocatedBuffer::ALLOCATE_MANAGED_UNCHECKED); - if (ret.data() == nullptr) { - THROW_ERR_MEMORY_ALLOCATION_FAILED(env); - return Local(); - } - } - - return scope.EscapeMaybe(ret.ToBuffer()); + return scope.EscapeMaybe( + AllocatedBuffer::AllocateManaged(env, length).ToBuffer()); } @@ -388,17 +377,8 @@ MaybeLocal Copy(Environment* env, const char* data, size_t length) { return Local(); } - AllocatedBuffer ret(env); + AllocatedBuffer ret = AllocatedBuffer::AllocateManaged(env, length); if (length > 0) { - CHECK_NOT_NULL(data); - ret = AllocatedBuffer::AllocateManaged( - env, - length, - AllocatedBuffer::ALLOCATE_MANAGED_UNCHECKED); - if (ret.data() == nullptr) { - THROW_ERR_MEMORY_ALLOCATION_FAILED(env); - return Local(); - } memcpy(ret.data(), data, length); } @@ -462,53 +442,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 fe9ac2d0cbf076..360732e37fd514 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4028,7 +4028,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 = AllocatedBuffer::AllocateManaged(env(), 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..65a4edcbea6b81 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -160,8 +160,7 @@ v8::MaybeLocal New(Environment* env, // ArrayBuffer::Allocator(). 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 +182,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()); From 3c49e03ce2053558d5ef923f60cb95c584dfbc2e Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 14 May 2020 12:47:59 -0700 Subject: [PATCH 3/3] src: remove superfluous inline keywords --- src/allocated_buffer-inl.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/allocated_buffer-inl.h b/src/allocated_buffer-inl.h index e10aa08e7f087f..2dee6f09a3e9d4 100644 --- a/src/allocated_buffer-inl.h +++ b/src/allocated_buffer-inl.h @@ -65,7 +65,7 @@ void AllocatedBuffer::Resize(size_t len) { env_->isolate(), std::move(backing_store_), len); } -inline uv_buf_t AllocatedBuffer::release() { +uv_buf_t AllocatedBuffer::release() { if (data() == nullptr) return uv_buf_init(nullptr, 0); CHECK_NOT_NULL(env_); @@ -75,33 +75,33 @@ inline uv_buf_t AllocatedBuffer::release() { return ret; } -inline char* AllocatedBuffer::data() { +char* AllocatedBuffer::data() { if (!backing_store_) return nullptr; return static_cast(backing_store_->Data()); } -inline const char* AllocatedBuffer::data() const { +const char* AllocatedBuffer::data() const { if (!backing_store_) return nullptr; return static_cast(backing_store_->Data()); } -inline size_t AllocatedBuffer::size() const { +size_t AllocatedBuffer::size() const { if (!backing_store_) return 0; return backing_store_->ByteLength(); } -inline void AllocatedBuffer::clear() { +void AllocatedBuffer::clear() { backing_store_.reset(); } -inline v8::MaybeLocal AllocatedBuffer::ToBuffer() { +v8::MaybeLocal AllocatedBuffer::ToBuffer() { v8::Local ab = ToArrayBuffer(); return Buffer::New(env_, ab, 0, ab->ByteLength()) .FromMaybe(v8::Local()); } -inline v8::Local AllocatedBuffer::ToArrayBuffer() { +v8::Local AllocatedBuffer::ToArrayBuffer() { return v8::ArrayBuffer::New(env_->isolate(), std::move(backing_store_)); }