From 982406ee4e5848d73e9a6a81496fecb379cc43cc Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Tue, 9 Jul 2019 22:23:30 -0700 Subject: [PATCH 01/12] Revert "fix: allocate memory of Buffer with V8's allocator" This reverts commit e66aa775ee4484349dc7ae368653acd84977f041. --- src/node_buffer.cc | 28 +++++++--------- src/node_crypto.cc | 77 +++++++++++++++---------------------------- src/node_crypto.h | 3 +- src/node_messaging.cc | 29 +++------------- src/stream_base.cc | 13 ++------ src/udp_wrap.cc | 22 +++++-------- src/util.h | 23 ++++--------- 7 files changed, 61 insertions(+), 134 deletions(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 475770def92..dd285156b56 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -61,10 +61,9 @@ bool zero_fill_all_buffers = false; namespace { -inline void* BufferMalloc(v8::Isolate* isolate, size_t length) { - auto* allocator = isolate->GetArrayBufferAllocator(); - return zero_fill_all_buffers ? allocator->Allocate(length) : - allocator->AllocateUninitialized(length); +inline void* BufferMalloc(size_t length) { + return zero_fill_all_buffers ? node::UncheckedCalloc(length) : + node::UncheckedMalloc(length); } } // namespace @@ -246,7 +245,7 @@ MaybeLocal New(Isolate* isolate, char* data = nullptr; if (length > 0) { - data = static_cast(BufferMalloc(isolate, length)); + data = static_cast(BufferMalloc(length)); if (data == nullptr) return Local(); @@ -255,14 +254,10 @@ MaybeLocal New(Isolate* isolate, CHECK(actual <= length); if (actual == 0) { - isolate->GetArrayBufferAllocator()->Free(data, length); + free(data); data = nullptr; } else if (actual < length) { - auto* allocator = isolate->GetArrayBufferAllocator(); - auto* excessive_data = data; - data = static_cast(allocator->AllocateUninitialized(actual)); - memcpy(data, excessive_data, actual); - allocator->Free(excessive_data, length); + data = node::Realloc(data, actual); } } @@ -271,7 +266,7 @@ MaybeLocal New(Isolate* isolate, return scope.Escape(buf); // Object failed to be created. Clean up resources. - isolate->GetArrayBufferAllocator()->Free(data, length); + free(data); return Local(); } @@ -297,7 +292,7 @@ MaybeLocal New(Environment* env, size_t length) { void* data; if (length > 0) { - data = BufferMalloc(env->isolate(), length); + data = BufferMalloc(length); if (data == nullptr) return Local(); } else { @@ -313,7 +308,7 @@ MaybeLocal New(Environment* env, size_t length) { if (ui.IsEmpty()) { // Object failed to be created. Clean up resources. - env->isolate()->GetArrayBufferAllocator()->Free(data, length); + free(data); } return scope.Escape(ui.FromMaybe(Local())); @@ -339,11 +334,10 @@ MaybeLocal Copy(Environment* env, const char* data, size_t length) { return Local(); } - auto* allocator = env->isolate()->GetArrayBufferAllocator(); void* new_data; if (length > 0) { CHECK_NOT_NULL(data); - new_data = allocator->AllocateUninitialized(length); + new_data = node::UncheckedMalloc(length); if (new_data == nullptr) return Local(); memcpy(new_data, data, length); @@ -360,7 +354,7 @@ MaybeLocal Copy(Environment* env, const char* data, size_t length) { if (ui.IsEmpty()) { // Object failed to be created. Clean up resources. - allocator->Free(new_data, length); + free(new_data); } return scope.Escape(ui.FromMaybe(Local())); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 21ca17c13f8..d7472f117cd 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1867,8 +1867,7 @@ void SSLWrap::GetFinished(const FunctionCallbackInfo& args) { if (len == 0) return; - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - char* buf = static_cast(allocator->AllocateUninitialized(len)); + char* buf = Malloc(len); CHECK_EQ(len, SSL_get_finished(w->ssl_.get(), buf, len)); args.GetReturnValue().Set(Buffer::New(env, buf, len).ToLocalChecked()); } @@ -1891,8 +1890,7 @@ void SSLWrap::GetPeerFinished(const FunctionCallbackInfo& args) { if (len == 0) return; - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - char* buf = static_cast(allocator->AllocateUninitialized(len)); + char* buf = Malloc(len); CHECK_EQ(len, SSL_get_peer_finished(w->ssl_.get(), buf, len)); args.GetReturnValue().Set(Buffer::New(env, buf, len).ToLocalChecked()); } @@ -1912,8 +1910,7 @@ void SSLWrap::GetSession(const FunctionCallbackInfo& args) { int slen = i2d_SSL_SESSION(sess, nullptr); CHECK_GT(slen, 0); - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - char* sbuf = static_cast(allocator->AllocateUninitialized(slen)); + char* sbuf = Malloc(slen); unsigned char* p = reinterpret_cast(sbuf); i2d_SSL_SESSION(sess, &p); args.GetReturnValue().Set(Buffer::New(env, sbuf, slen).ToLocalChecked()); @@ -2334,12 +2331,11 @@ int SSLWrap::TLSExtStatusCallback(SSL* s, void* arg) { size_t len = Buffer::Length(obj); // OpenSSL takes control of the pointer after accepting it - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - uint8_t* data = static_cast(allocator->AllocateUninitialized(len)); + char* data = node::Malloc(len); memcpy(data, resp, len); if (!SSL_set_tlsext_status_ocsp_resp(s, data, len)) - allocator->Free(data, len); + free(data); w->ocsp_response_.Reset(); return SSL_TLSEXT_ERR_OK; @@ -3054,8 +3050,7 @@ CipherBase::UpdateResult CipherBase::Update(const char* data, return kErrorState; } - auto* allocator = env()->isolate()->GetArrayBufferAllocator(); - *out = static_cast(allocator->AllocateUninitialized(buff_len)); + *out = Malloc(buff_len); int r = EVP_CipherUpdate(ctx_.get(), *out, out_len, @@ -3098,8 +3093,7 @@ void CipherBase::Update(const FunctionCallbackInfo& args) { } if (r != kSuccess) { - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - allocator->Free(out, out_len); + free(out); if (r == kErrorState) { ThrowCryptoError(env, ERR_get_error(), "Trying to add data in unsupported state"); @@ -3138,9 +3132,8 @@ bool CipherBase::Final(unsigned char** out, int* out_len) { const int mode = EVP_CIPHER_CTX_mode(ctx_.get()); - auto* allocator = env()->isolate()->GetArrayBufferAllocator(); - *out = static_cast(allocator->AllocateUninitialized( - EVP_CIPHER_CTX_block_size(ctx_.get()))); + *out = Malloc( + static_cast(EVP_CIPHER_CTX_block_size(ctx_.get()))); if (kind_ == kDecipher && IsSupportedAuthenticatedMode(mode)) { MaybePassAuthTagToOpenSSL(); @@ -3189,8 +3182,7 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { bool r = cipher->Final(&out_value, &out_len); if (out_len <= 0 || !r) { - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - allocator->Free(out_value, out_len); + free(out_value); out_value = nullptr; out_len = 0; if (!r) { @@ -3839,8 +3831,7 @@ void Verify::VerifyFinal(const FunctionCallbackInfo& args) { template -bool PublicKeyCipher::Cipher(Environment* env, - const char* key_pem, +bool PublicKeyCipher::Cipher(const char* key_pem, int key_pem_len, const char* passphrase, int padding, @@ -3849,7 +3840,6 @@ bool PublicKeyCipher::Cipher(Environment* env, unsigned char** out, size_t* out_len) { EVPKeyPointer pkey; - auto* allocator = env->isolate()->GetArrayBufferAllocator(); // Check if this is a PKCS#8 or RSA public key before trying as X.509 and // private key. @@ -3882,7 +3872,7 @@ bool PublicKeyCipher::Cipher(Environment* env, if (EVP_PKEY_cipher(ctx.get(), nullptr, out_len, data, len) <= 0) return false; - *out = static_cast(allocator->AllocateUninitialized(*out_len)); + *out = Malloc(*out_len); if (EVP_PKEY_cipher(ctx.get(), *out, out_len, data, len) <= 0) return false; @@ -3916,7 +3906,6 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo& args) { ClearErrorOnReturn clear_error_on_return; bool r = Cipher( - env, kbuf, klen, args.Length() >= 4 && !args[3]->IsNull() ? *passphrase : nullptr, @@ -3927,8 +3916,7 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo& args) { &out_len); if (out_len == 0 || !r) { - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - allocator->Free(out_value, out_len); + free(out_value); out_value = nullptr; out_len = 0; if (!r) { @@ -4141,8 +4129,7 @@ void DiffieHellman::GenerateKeys(const FunctionCallbackInfo& args) { const BIGNUM* pub_key; DH_get0_key(diffieHellman->dh_.get(), &pub_key, nullptr); size_t size = BN_num_bytes(pub_key); - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - char* data = static_cast(allocator->AllocateUninitialized(size)); + char* data = Malloc(size); BN_bn2bin(pub_key, reinterpret_cast(data)); args.GetReturnValue().Set(Buffer::New(env, data, size).ToLocalChecked()); } @@ -4161,8 +4148,7 @@ void DiffieHellman::GetField(const FunctionCallbackInfo& args, if (num == nullptr) return env->ThrowError(err_if_null); size_t size = BN_num_bytes(num); - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - char* data = static_cast(allocator->AllocateUninitialized(size)); + char* data = Malloc(size); BN_bn2bin(num, reinterpret_cast(data)); args.GetReturnValue().Set(Buffer::New(env, data, size).ToLocalChecked()); } @@ -4226,8 +4212,7 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo& args) { Buffer::Length(args[0]), 0)); - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - MallocedBuffer data(DH_size(diffieHellman->dh_.get()), allocator); + MallocedBuffer data(DH_size(diffieHellman->dh_.get())); int size = DH_compute_key(reinterpret_cast(data.data), key.get(), @@ -4447,14 +4432,13 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo& args) { } // NOTE: field_size is in bits - auto* allocator = env->isolate()->GetArrayBufferAllocator(); int field_size = EC_GROUP_get_degree(ecdh->group_); size_t out_len = (field_size + 7) / 8; - char* out = static_cast(allocator->AllocateUninitialized(out_len)); + char* out = node::Malloc(out_len); int r = ECDH_compute_key(out, out_len, pub.get(), ecdh->key_.get(), nullptr); if (!r) { - allocator->Free(out, out_len); + free(out); return env->ThrowError("Failed to compute ECDH key"); } @@ -4485,13 +4469,11 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo& args) { if (size == 0) return env->ThrowError("Failed to get public key length"); - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - unsigned char* out = - static_cast(allocator->AllocateUninitialized(size)); + unsigned char* out = node::Malloc(size); int r = EC_POINT_point2oct(ecdh->group_, pub, form, out, size, nullptr); if (r != size) { - allocator->Free(out, size); + free(out); return env->ThrowError("Failed to get public key"); } @@ -4511,13 +4493,11 @@ void ECDH::GetPrivateKey(const FunctionCallbackInfo& args) { if (b == nullptr) return env->ThrowError("Failed to get ECDH private key"); - auto* allocator = env->isolate()->GetArrayBufferAllocator(); int size = BN_num_bytes(b); - unsigned char* out = - static_cast(allocator->AllocateUninitialized(size)); + unsigned char* out = node::Malloc(size); if (size != BN_bn2bin(b, out)) { - allocator->Free(out, size); + free(out); return env->ThrowError("Failed to convert ECDH private key to Buffer"); } @@ -4986,9 +4966,8 @@ void VerifySpkac(const FunctionCallbackInfo& args) { } -char* ExportPublicKey(Environment* env, const char* data, int len, size_t* size) { +char* ExportPublicKey(const char* data, int len, size_t* size) { char* buf = nullptr; - auto* allocator = env->isolate()->GetArrayBufferAllocator(); BIOPointer bio(BIO_new(BIO_s_mem())); if (!bio) @@ -5009,7 +4988,7 @@ char* ExportPublicKey(Environment* env, const char* data, int len, size_t* size) BIO_get_mem_ptr(bio.get(), &ptr); *size = ptr->length; - buf = static_cast(allocator->AllocateUninitialized(*size)); + buf = Malloc(*size); memcpy(buf, ptr->data, *size); return buf; @@ -5027,7 +5006,7 @@ void ExportPublicKey(const FunctionCallbackInfo& args) { CHECK_NOT_NULL(data); size_t pkey_size; - char* pkey = ExportPublicKey(env, data, length, &pkey_size); + char* pkey = ExportPublicKey(data, length, &pkey_size); if (pkey == nullptr) return args.GetReturnValue().SetEmptyString(); @@ -5109,13 +5088,11 @@ void ConvertKey(const FunctionCallbackInfo& args) { if (size == 0) return env->ThrowError("Failed to get public key length"); - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - unsigned char* out = - static_cast(allocator->AllocateUninitialized(size)); + unsigned char* out = node::Malloc(size); int r = EC_POINT_point2oct(group.get(), pub.get(), form, out, size, nullptr); if (r != size) { - allocator->Free(out, size); + free(out); return env->ThrowError("Failed to get public key"); } diff --git a/src/node_crypto.h b/src/node_crypto.h index 2da3bc7cac5..1a93ae7a47e 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -576,8 +576,7 @@ class PublicKeyCipher { template - static bool Cipher(Environment* env, - const char* key_pem, + static bool Cipher(const char* key_pem, int key_pem_len, const char* passphrase, int padding, diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 2271ed3f74b..20e0c7673b8 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -164,12 +164,8 @@ void ThrowDataCloneException(Environment* env, Local message) { // DeserializerDelegate understands how to unpack. class SerializerDelegate : public ValueSerializer::Delegate { public: - SerializerDelegate( - Environment* env, - Local context, - Message* m, - v8::ArrayBuffer::Allocator *allocator) - : env_(env), context_(context), msg_(m), allocator_(allocator) {} + SerializerDelegate(Environment* env, Local context, Message* m) + : env_(env), context_(context), msg_(m) {} void ThrowDataCloneError(Local message) override { ThrowDataCloneException(env_, message); @@ -205,19 +201,6 @@ class SerializerDelegate : public ValueSerializer::Delegate { return Just(i); } - void FreeBufferMemory(void* buffer) override { - // the second parameter is unused, so pass 0 as a filler - return allocator_->Free(buffer, 0); - } - - void* ReallocateBufferMemory(void* old_buffer, - size_t size, - size_t* actual_size) override { - auto result = allocator_->Realloc(old_buffer, size); - *actual_size = result ? size :0; - return result; - } - void Finish() { // Only close the MessagePort handles and actually transfer them // once we know that serialization succeeded. @@ -247,7 +230,6 @@ class SerializerDelegate : public ValueSerializer::Delegate { Message* msg_; std::vector> seen_shared_array_buffers_; std::vector ports_; - v8::ArrayBuffer::Allocator* allocator_; friend class worker::Message; }; @@ -265,7 +247,7 @@ Maybe Message::Serialize(Environment* env, // Verify that we're not silently overwriting an existing message. CHECK(main_message_buf_.is_empty()); - SerializerDelegate delegate(env, context, this, env->isolate()->GetArrayBufferAllocator()); + SerializerDelegate delegate(env, context, this); ValueSerializer serializer(env->isolate(), &delegate); delegate.serializer = &serializer; @@ -331,8 +313,7 @@ Maybe Message::Serialize(Environment* env, ab->Neuter(); array_buffer_contents_.push_back( MallocedBuffer { static_cast(contents.Data()), - contents.ByteLength(), - env->isolate()->GetArrayBufferAllocator() }); + contents.ByteLength() }); } delegate.Finish(); @@ -340,7 +321,7 @@ Maybe Message::Serialize(Environment* env, // The serializer gave us a buffer allocated using `malloc()`. std::pair data = serializer.Release(); main_message_buf_ = - MallocedBuffer(reinterpret_cast(data.first), data.second, env->isolate()->GetArrayBufferAllocator()); + MallocedBuffer(reinterpret_cast(data.first), data.second); return Just(true); } diff --git a/src/stream_base.cc b/src/stream_base.cc index dcba9e1ffa9..f429f3593fd 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -358,13 +358,7 @@ void StreamResource::ClearError() { uv_buf_t StreamListener::OnStreamAlloc(size_t suggested_size) { - CHECK_NE(stream_, nullptr); - StreamBase* stream = static_cast(stream_); - Environment* env = stream->stream_env(); - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - return uv_buf_init( - static_cast(allocator->AllocateUninitialized(suggested_size)), - suggested_size); + return uv_buf_init(Malloc(suggested_size), suggested_size); } @@ -372,19 +366,18 @@ void EmitToJSStreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { CHECK_NOT_NULL(stream_); StreamBase* stream = static_cast(stream_); Environment* env = stream->stream_env(); - auto* allocator = env->isolate()->GetArrayBufferAllocator(); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); if (nread <= 0) { - allocator->Free(buf.base, buf.len); + free(buf.base); if (nread < 0) stream->CallJSOnreadMethod(nread, Local()); return; } CHECK_LE(static_cast(nread), buf.len); - char* base = static_cast(allocator->Realloc(buf.base, nread)); + char* base = Realloc(buf.base, nread); Local obj = Buffer::New(env, base, nread).ToLocalChecked(); stream->CallJSOnreadMethod(nread, obj); diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 9ed9a5a71f4..724f98c0cc9 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -455,10 +455,7 @@ void UDPWrap::OnSend(uv_udp_send_t* req, int status) { void UDPWrap::OnAlloc(uv_handle_t* handle, size_t suggested_size, uv_buf_t* buf) { - auto* wrap = static_cast(handle->data); - auto* allocator = wrap->env()->isolate()->GetArrayBufferAllocator(); - buf->base = - static_cast(allocator->AllocateUninitialized(suggested_size)); + buf->base = node::Malloc(suggested_size); buf->len = suggested_size; } @@ -468,16 +465,15 @@ void UDPWrap::OnRecv(uv_udp_t* handle, const uv_buf_t* buf, const struct sockaddr* addr, unsigned int flags) { - UDPWrap* wrap = static_cast(handle->data); - Environment* env = wrap->env(); - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - if (nread == 0 && addr == nullptr) { if (buf->base != nullptr) - allocator->Free(buf->base, buf->len); + free(buf->base); return; } + UDPWrap* wrap = static_cast(handle->data); + Environment* env = wrap->env(); + HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -491,15 +487,13 @@ void UDPWrap::OnRecv(uv_udp_t* handle, if (nread < 0) { if (buf->base != nullptr) - allocator->Free(buf->base, buf->len); + free(buf->base); wrap->MakeCallback(env->onmessage_string(), arraysize(argv), argv); return; } - // Note that nread may be smaller thant buf->len, in that case the length - // passed to ArrayBufferAllocator::Free would not be the correct one, but - // it should be fine unless embedder is using some unusual memory allocator. - argv[2] = Buffer::New(env, buf->base, nread).ToLocalChecked(); + char* base = node::UncheckedRealloc(buf->base, nread); + argv[2] = Buffer::New(env, base, nread).ToLocalChecked(); argv[3] = AddressToJS(env, addr); wrap->MakeCallback(env->onmessage_string(), arraysize(argv), argv); } diff --git a/src/util.h b/src/util.h index 8453762a728..4036383dee9 100644 --- a/src/util.h +++ b/src/util.h @@ -1,4 +1,5 @@ // Copyright Joyent, Inc. and other Node contributors. +// // Permission is hereby granted, free of charge, to any person obtaining a // copy of this software and associated documentation files (the // "Software"), to deal in the Software without restriction, including @@ -439,10 +440,8 @@ template struct MallocedBuffer { T* data; size_t size; - v8::ArrayBuffer::Allocator* allocator; T* release() { - allocator = nullptr; T* ret = data; data = nullptr; return ret; @@ -450,28 +449,18 @@ struct MallocedBuffer { inline bool is_empty() const { return data == nullptr; } - MallocedBuffer() : data(nullptr), allocator(nullptr) {} - MallocedBuffer(size_t size, v8::ArrayBuffer::Allocator* allocator) - : size(size), allocator(allocator) { - data = static_cast(allocator->AllocateUninitialized(size)); - } - MallocedBuffer(char *data, size_t size, v8::ArrayBuffer::Allocator *allocator) - : data(data), size(size), allocator(allocator) {} - MallocedBuffer(MallocedBuffer&& other) - : data(other.data), size(other.size), allocator(other.allocator) { + MallocedBuffer() : data(nullptr) {} + explicit MallocedBuffer(size_t size) : data(Malloc(size)), size(size) {} + MallocedBuffer(char* data, size_t size) : data(data), size(size) {} + MallocedBuffer(MallocedBuffer&& other) : data(other.data), size(other.size) { other.data = nullptr; - other.allocator = nullptr; } MallocedBuffer& operator=(MallocedBuffer&& other) { this->~MallocedBuffer(); return *new(this) MallocedBuffer(std::move(other)); } ~MallocedBuffer() { - if (allocator) { - allocator->Free(data, size); - } else { - free(data); - } + free(data); } MallocedBuffer(const MallocedBuffer&) = delete; MallocedBuffer& operator=(const MallocedBuffer&) = delete; From c2ac64913a6fd4abcf8ce3e138df21791e60cec1 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 25 Jan 2019 17:27:00 -0600 Subject: [PATCH 02/12] crypto: fix malloc mixing in X509ToObject EC_KEY_key2buf returns an OPENSSL_malloc'd pointer so it shouldn't be passed into Buffer::New, which expect a libc malloc'd pointer. Instead, factor out the ECDH::GetPublicKey code which uses EC_POINT_point2oct. This preserves the existing behavior where encoding failures are silently ignored, but it is probably safe to CHECK fail them instead. PR-URL: https://github.com/nodejs/node/pull/25717 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen --- src/node_crypto.cc | 58 ++++++++++++++++++++-------------------------- 1 file changed, 25 insertions(+), 33 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d7472f117cd..23c4d0c7aef 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1556,6 +1556,24 @@ static void AddFingerprintDigest(const unsigned char* md, } } +static MaybeLocal ECPointToBuffer(Environment* env, + const EC_GROUP* group, + const EC_POINT* point, + point_conversion_form_t form) { + size_t len = EC_POINT_point2oct(group, point, form, nullptr, 0, nullptr); + if (len == 0) { + env->ThrowError("Failed to get public key length"); + return MaybeLocal(); + } + MallocedBuffer buf(len); + len = EC_POINT_point2oct(group, point, form, buf.data, buf.size, nullptr); + if (len == 0) { + env->ThrowError("Failed to get public key"); + return MaybeLocal(); + } + return Buffer::New(env, buf.release(), len); +} + static Local X509ToObject(Environment* env, X509* cert) { EscapableHandleScope scope(env->isolate()); Local context = env->context(); @@ -4460,26 +4478,14 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo& args) { if (pub == nullptr) return env->ThrowError("Failed to get ECDH public key"); - int size; CHECK(args[0]->IsUint32()); uint32_t val = args[0].As()->Value(); point_conversion_form_t form = static_cast(val); - size = EC_POINT_point2oct(ecdh->group_, pub, form, nullptr, 0, nullptr); - if (size == 0) - return env->ThrowError("Failed to get public key length"); - - unsigned char* out = node::Malloc(size); - - int r = EC_POINT_point2oct(ecdh->group_, pub, form, out, size, nullptr); - if (r != size) { - free(out); - return env->ThrowError("Failed to get public key"); - } - - Local buf = - Buffer::New(env, reinterpret_cast(out), size).ToLocalChecked(); - args.GetReturnValue().Set(buf); + MaybeLocal buf = + ECPointToBuffer(env, EC_KEY_get0_group(ecdh->key_.get()), pub, form); + if (buf.IsEmpty()) return; + args.GetReturnValue().Set(buf.ToLocalChecked()); } @@ -5082,23 +5088,9 @@ void ConvertKey(const FunctionCallbackInfo& args) { uint32_t val = args[2].As()->Value(); point_conversion_form_t form = static_cast(val); - int size = EC_POINT_point2oct( - group.get(), pub.get(), form, nullptr, 0, nullptr); - - if (size == 0) - return env->ThrowError("Failed to get public key length"); - - unsigned char* out = node::Malloc(size); - - int r = EC_POINT_point2oct(group.get(), pub.get(), form, out, size, nullptr); - if (r != size) { - free(out); - return env->ThrowError("Failed to get public key"); - } - - Local buf = - Buffer::New(env, reinterpret_cast(out), size).ToLocalChecked(); - args.GetReturnValue().Set(buf); + MaybeLocal buf = ECPointToBuffer(env, group.get(), pub.get(), form); + if (buf.IsEmpty()) return; + args.GetReturnValue().Set(buf.ToLocalChecked()); } From 4f63360eda60cb6f27fec2a0c780ff42a23049d9 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 29 Jan 2019 05:51:09 +0000 Subject: [PATCH 03/12] crypto: don't crash X509ToObject on error Use MaybeLocal::ToLocal and don't crash X509ToObject on error. PR-URL: https://github.com/nodejs/node/pull/25717 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen --- src/node_crypto.cc | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 23c4d0c7aef..e4f2c3103c8 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1556,24 +1556,27 @@ static void AddFingerprintDigest(const unsigned char* md, } } + static MaybeLocal ECPointToBuffer(Environment* env, const EC_GROUP* group, const EC_POINT* point, - point_conversion_form_t form) { + point_conversion_form_t form, + const char** error) { size_t len = EC_POINT_point2oct(group, point, form, nullptr, 0, nullptr); if (len == 0) { - env->ThrowError("Failed to get public key length"); + if (error != nullptr) *error = "Failed to get public key length"; return MaybeLocal(); } MallocedBuffer buf(len); len = EC_POINT_point2oct(group, point, form, buf.data, buf.size, nullptr); if (len == 0) { - env->ThrowError("Failed to get public key"); + if (error != nullptr) *error = "Failed to get public key"; return MaybeLocal(); } return Buffer::New(env, buf.release(), len); } + static Local X509ToObject(Environment* env, X509* cert) { EscapableHandleScope scope(env->isolate()); Local context = env->context(); @@ -4474,6 +4477,7 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo& args) { ECDH* ecdh; ASSIGN_OR_RETURN_UNWRAP(&ecdh, args.Holder()); + const EC_GROUP* group = EC_KEY_get0_group(ecdh->key_.get()); const EC_POINT* pub = EC_KEY_get0_public_key(ecdh->key_.get()); if (pub == nullptr) return env->ThrowError("Failed to get ECDH public key"); @@ -4482,10 +4486,11 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo& args) { uint32_t val = args[0].As()->Value(); point_conversion_form_t form = static_cast(val); - MaybeLocal buf = - ECPointToBuffer(env, EC_KEY_get0_group(ecdh->key_.get()), pub, form); - if (buf.IsEmpty()) return; - args.GetReturnValue().Set(buf.ToLocalChecked()); + const char* error; + Local buf; + if (!ECPointToBuffer(env, group, pub, form, &error).ToLocal(&buf)) + return env->ThrowError(error); + args.GetReturnValue().Set(buf); } @@ -5088,9 +5093,11 @@ void ConvertKey(const FunctionCallbackInfo& args) { uint32_t val = args[2].As()->Value(); point_conversion_form_t form = static_cast(val); - MaybeLocal buf = ECPointToBuffer(env, group.get(), pub.get(), form); - if (buf.IsEmpty()) return; - args.GetReturnValue().Set(buf.ToLocalChecked()); + const char* error; + Local buf; + if (!ECPointToBuffer(env, group.get(), pub.get(), form, &error).ToLocal(&buf)) + return env->ThrowError(error); + args.GetReturnValue().Set(buf); } From dea1404add303eaa84bcb616263f8dc7bf39d9e6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 18 Feb 2019 22:58:27 +0100 Subject: [PATCH 04/12] src: allocate Buffer memory using ArrayBuffer allocator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Always use the right allocator for memory that is turned into an `ArrayBuffer` at a later point. This enables embedders to use their own `ArrayBuffer::Allocator`s, and is inspired by Electron’s electron/node@f61bae3440e. It should render their downstream patch unnecessary. Refs: https://github.com/electron/node/commit/f61bae3440e1bfcc83bba6ff0785adfb89b4045e PR-URL: https://github.com/nodejs/node/pull/26207 Reviewed-By: James M Snell Reviewed-By: Joyee Cheung --- src/node_buffer.cc | 112 ++++++++----------- src/node_crypto.cc | 237 ++++++++++++++++++---------------------- src/node_crypto.h | 11 +- src/node_http2.cc | 30 +++-- src/node_http2.h | 1 + src/node_http_parser.cc | 5 +- src/node_internals.h | 12 +- src/node_messaging.cc | 21 ++++ src/node_serdes.cc | 5 +- src/stream_base-inl.h | 15 +-- src/stream_base.cc | 45 ++++---- src/stream_base.h | 13 +-- src/stream_pipe.cc | 15 ++- src/stream_pipe.h | 2 +- src/udp_wrap.cc | 22 ++-- 15 files changed, 255 insertions(+), 291 deletions(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index dd285156b56..2da7fcb0891 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -55,19 +55,6 @@ size_t length = end - start; namespace node { - -// if true, all Buffer and SlowBuffer instances will automatically zero-fill -bool zero_fill_all_buffers = false; - -namespace { - -inline void* BufferMalloc(size_t length) { - return zero_fill_all_buffers ? node::UncheckedCalloc(length) : - node::UncheckedMalloc(length); -} - -} // namespace - namespace Buffer { using v8::ArrayBuffer; @@ -245,7 +232,7 @@ MaybeLocal New(Isolate* isolate, char* data = nullptr; if (length > 0) { - data = static_cast(BufferMalloc(length)); + data = UncheckedMalloc(length); if (data == nullptr) return Local(); @@ -261,13 +248,7 @@ MaybeLocal New(Isolate* isolate, } } - Local buf; - if (New(isolate, data, actual).ToLocal(&buf)) - return scope.Escape(buf); - - // Object failed to be created. Clean up resources. - free(data); - return Local(); + return scope.EscapeMaybe(New(isolate, data, actual)); } @@ -290,28 +271,16 @@ MaybeLocal New(Environment* env, size_t length) { return Local(); } - void* data; + AllocatedBuffer ret(env); if (length > 0) { - data = BufferMalloc(length); - if (data == nullptr) + ret = env->AllocateManaged(length, false); + if (ret.data() == nullptr) { + THROW_ERR_MEMORY_ALLOCATION_FAILED(env); return Local(); - } else { - data = nullptr; - } - - Local ab = - ArrayBuffer::New(env->isolate(), - data, - length, - ArrayBufferCreationMode::kInternalized); - MaybeLocal ui = Buffer::New(env, ab, 0, length); - - if (ui.IsEmpty()) { - // Object failed to be created. Clean up resources. - free(data); + } } - return scope.Escape(ui.FromMaybe(Local())); + return scope.EscapeMaybe(ret.ToBuffer()); } @@ -334,30 +303,18 @@ MaybeLocal Copy(Environment* env, const char* data, size_t length) { return Local(); } - void* new_data; + AllocatedBuffer ret(env); if (length > 0) { CHECK_NOT_NULL(data); - new_data = node::UncheckedMalloc(length); - if (new_data == nullptr) + ret = env->AllocateManaged(length, false); + if (ret.data() == nullptr) { + THROW_ERR_MEMORY_ALLOCATION_FAILED(env); return Local(); - memcpy(new_data, data, length); - } else { - new_data = nullptr; - } - - Local ab = - ArrayBuffer::New(env->isolate(), - new_data, - length, - ArrayBufferCreationMode::kInternalized); - MaybeLocal ui = Buffer::New(env, ab, 0, length); - - if (ui.IsEmpty()) { - // Object failed to be created. Clean up resources. - free(new_data); + } + memcpy(ret.data(), data, length); } - return scope.Escape(ui.FromMaybe(Local())); + return scope.EscapeMaybe(ret.ToBuffer()); } @@ -403,24 +360,44 @@ MaybeLocal New(Environment* env, return scope.Escape(ui.ToLocalChecked()); } - +// Warning: This function needs `data` to be allocated with malloc() and not +// necessarily isolate's ArrayBuffer::Allocator. MaybeLocal New(Isolate* isolate, char* data, size_t length) { EscapableHandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(isolate); CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. Local obj; - if (Buffer::New(env, data, length).ToLocal(&obj)) + if (Buffer::New(env, data, length, true).ToLocal(&obj)) return handle_scope.Escape(obj); return Local(); } - -MaybeLocal New(Environment* env, char* data, size_t length) { +// Warning: If this call comes through the public node_buffer.h API, +// 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) { 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); + } + } + Local ab = ArrayBuffer::New(env->isolate(), data, @@ -1020,15 +997,13 @@ static void EncodeUtf8String(const FunctionCallbackInfo& args) { Local str = args[0].As(); size_t length = str->Utf8Length(isolate); - char* data = node::UncheckedMalloc(length); + AllocatedBuffer buf = env->AllocateManaged(length); str->WriteUtf8(isolate, - data, + buf.data(), -1, // We are certain that `data` is sufficiently large nullptr, String::NO_NULL_TERMINATION | String::REPLACE_INVALID_UTF8); - auto array_buf = ArrayBuffer::New( - isolate, data, length, ArrayBufferCreationMode::kInternalized); - auto array = Uint8Array::New(array_buf, 0, length); + auto array = Uint8Array::New(buf.ToArrayBuffer(), 0, length); args.GetReturnValue().Set(array); } @@ -1055,7 +1030,8 @@ void SetupBufferJS(const FunctionCallbackInfo& args) { env->SetMethod(proto, "ucs2Write", StringWrite); env->SetMethod(proto, "utf8Write", StringWrite); - if (auto zero_fill_field = env->isolate_data()->zero_fill_field()) { + if (ArrayBufferAllocator* allocator = env->isolate_data()->node_allocator()) { + uint32_t* zero_fill_field = allocator->zero_fill_field(); CHECK(args[1]->IsObject()); auto binding_object = args[1].As(); auto array_buffer = ArrayBuffer::New(env->isolate(), diff --git a/src/node_crypto.cc b/src/node_crypto.cc index e4f2c3103c8..c170f4ebca7 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1556,7 +1556,6 @@ static void AddFingerprintDigest(const unsigned char* md, } } - static MaybeLocal ECPointToBuffer(Environment* env, const EC_GROUP* group, const EC_POINT* point, @@ -1567,16 +1566,20 @@ static MaybeLocal ECPointToBuffer(Environment* env, if (error != nullptr) *error = "Failed to get public key length"; return MaybeLocal(); } - MallocedBuffer buf(len); - len = EC_POINT_point2oct(group, point, form, buf.data, buf.size, nullptr); + AllocatedBuffer buf = env->AllocateManaged(len); + len = EC_POINT_point2oct(group, + point, + form, + reinterpret_cast(buf.data()), + buf.size(), + nullptr); if (len == 0) { if (error != nullptr) *error = "Failed to get public key"; return MaybeLocal(); } - return Buffer::New(env, buf.release(), len); + return buf.ToBuffer(); } - static Local X509ToObject(Environment* env, X509* cert) { EscapableHandleScope scope(env->isolate()); Local context = env->context(); @@ -1888,9 +1891,9 @@ void SSLWrap::GetFinished(const FunctionCallbackInfo& args) { if (len == 0) return; - char* buf = Malloc(len); - CHECK_EQ(len, SSL_get_finished(w->ssl_.get(), buf, len)); - args.GetReturnValue().Set(Buffer::New(env, buf, len).ToLocalChecked()); + AllocatedBuffer buf = env->AllocateManaged(len); + CHECK_EQ(len, SSL_get_finished(w->ssl_.get(), buf.data(), len)); + args.GetReturnValue().Set(buf.ToBuffer().ToLocalChecked()); } @@ -1911,9 +1914,9 @@ void SSLWrap::GetPeerFinished(const FunctionCallbackInfo& args) { if (len == 0) return; - char* buf = Malloc(len); - CHECK_EQ(len, SSL_get_peer_finished(w->ssl_.get(), buf, len)); - args.GetReturnValue().Set(Buffer::New(env, buf, len).ToLocalChecked()); + AllocatedBuffer buf = env->AllocateManaged(len); + CHECK_EQ(len, SSL_get_peer_finished(w->ssl_.get(), buf.data(), len)); + args.GetReturnValue().Set(buf.ToBuffer().ToLocalChecked()); } @@ -1931,10 +1934,10 @@ void SSLWrap::GetSession(const FunctionCallbackInfo& args) { int slen = i2d_SSL_SESSION(sess, nullptr); CHECK_GT(slen, 0); - char* sbuf = Malloc(slen); - unsigned char* p = reinterpret_cast(sbuf); + AllocatedBuffer sbuf = env->AllocateManaged(slen); + unsigned char* p = reinterpret_cast(sbuf.data()); i2d_SSL_SESSION(sess, &p); - args.GetReturnValue().Set(Buffer::New(env, sbuf, slen).ToLocalChecked()); + args.GetReturnValue().Set(sbuf.ToBuffer().ToLocalChecked()); } @@ -2352,11 +2355,12 @@ int SSLWrap::TLSExtStatusCallback(SSL* s, void* arg) { size_t len = Buffer::Length(obj); // OpenSSL takes control of the pointer after accepting it - char* data = node::Malloc(len); + auto* allocator = env->isolate()->GetArrayBufferAllocator(); + uint8_t* data = static_cast(allocator->AllocateUninitialized(len)); memcpy(data, resp, len); if (!SSL_set_tlsext_status_ocsp_resp(s, data, len)) - free(data); + allocator->Free(data, len); w->ocsp_response_.Reset(); return SSL_TLSEXT_ERR_OK; @@ -3036,11 +3040,9 @@ void CipherBase::SetAAD(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(b); // Possibly report invalid state failure } - CipherBase::UpdateResult CipherBase::Update(const char* data, int len, - unsigned char** out, - int* out_len) { + AllocatedBuffer* out) { if (!ctx_) return kErrorState; MarkPopErrorOnReturn mark_pop_error_on_return; @@ -3058,27 +3060,27 @@ CipherBase::UpdateResult CipherBase::Update(const char* data, CHECK(MaybePassAuthTagToOpenSSL()); } - *out_len = 0; - int buff_len = len + EVP_CIPHER_CTX_block_size(ctx_.get()); + int buf_len = len + EVP_CIPHER_CTX_block_size(ctx_.get()); // For key wrapping algorithms, get output size by calling // EVP_CipherUpdate() with null output. if (kind_ == kCipher && mode == EVP_CIPH_WRAP_MODE && EVP_CipherUpdate(ctx_.get(), nullptr, - &buff_len, + &buf_len, reinterpret_cast(data), len) != 1) { return kErrorState; } - *out = Malloc(buff_len); + *out = env()->AllocateManaged(buf_len); int r = EVP_CipherUpdate(ctx_.get(), - *out, - out_len, + reinterpret_cast(out->data()), + &buf_len, reinterpret_cast(data), len); - CHECK_LE(*out_len, buff_len); + CHECK_LE(static_cast(buf_len), out->size()); + out->Resize(buf_len); // When in CCM mode, EVP_CipherUpdate will fail if the authentication tag is // invalid. In that case, remember the error and throw in final(). @@ -3096,9 +3098,8 @@ void CipherBase::Update(const FunctionCallbackInfo& args) { CipherBase* cipher; ASSIGN_OR_RETURN_UNWRAP(&cipher, args.Holder()); - unsigned char* out = nullptr; + AllocatedBuffer out; UpdateResult r; - int out_len = 0; // Only copy the data if we have to, because it's a string if (args[0]->IsString()) { @@ -3106,15 +3107,14 @@ void CipherBase::Update(const FunctionCallbackInfo& args) { if (!decoder.Decode(env, args[0].As(), args[1], UTF8) .FromMaybe(false)) return; - r = cipher->Update(decoder.out(), decoder.size(), &out, &out_len); + r = cipher->Update(decoder.out(), decoder.size(), &out); } else { char* buf = Buffer::Data(args[0]); size_t buflen = Buffer::Length(args[0]); - r = cipher->Update(buf, buflen, &out, &out_len); + r = cipher->Update(buf, buflen, &out); } if (r != kSuccess) { - free(out); if (r == kErrorState) { ThrowCryptoError(env, ERR_get_error(), "Trying to add data in unsupported state"); @@ -3122,11 +3122,9 @@ void CipherBase::Update(const FunctionCallbackInfo& args) { return; } - CHECK(out != nullptr || out_len == 0); - Local buf = - Buffer::New(env, reinterpret_cast(out), out_len).ToLocalChecked(); + CHECK(out.data() != nullptr || out.size() == 0); - args.GetReturnValue().Set(buf); + args.GetReturnValue().Set(out.ToBuffer().ToLocalChecked()); } @@ -3146,14 +3144,13 @@ void CipherBase::SetAutoPadding(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(b); // Possibly report invalid state failure } - -bool CipherBase::Final(unsigned char** out, int* out_len) { +bool CipherBase::Final(AllocatedBuffer* out) { if (!ctx_) return false; const int mode = EVP_CIPHER_CTX_mode(ctx_.get()); - *out = Malloc( + *out = env()->AllocateManaged( static_cast(EVP_CIPHER_CTX_block_size(ctx_.get()))); if (kind_ == kDecipher && IsSupportedAuthenticatedMode(mode)) { @@ -3165,8 +3162,17 @@ bool CipherBase::Final(unsigned char** out, int* out_len) { bool ok; if (kind_ == kDecipher && mode == EVP_CIPH_CCM_MODE) { ok = !pending_auth_failed_; + *out = AllocatedBuffer(env()); // Empty buffer. } else { - ok = EVP_CipherFinal_ex(ctx_.get(), *out, out_len) == 1; + int out_len = out->size(); + ok = EVP_CipherFinal_ex(ctx_.get(), + reinterpret_cast(out->data()), + &out_len) == 1; + + if (out_len >= 0) + out->Resize(out_len); + else + *out = AllocatedBuffer(); // *out will not be used. if (ok && kind_ == kCipher && IsAuthenticatedMode()) { // In GCM mode, the authentication tag length can be specified in advance, @@ -3195,33 +3201,21 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&cipher, args.Holder()); if (cipher->ctx_ == nullptr) return env->ThrowError("Unsupported state"); - unsigned char* out_value = nullptr; - int out_len = -1; + AllocatedBuffer out; // Check IsAuthenticatedMode() first, Final() destroys the EVP_CIPHER_CTX. const bool is_auth_mode = cipher->IsAuthenticatedMode(); - bool r = cipher->Final(&out_value, &out_len); - - if (out_len <= 0 || !r) { - free(out_value); - out_value = nullptr; - out_len = 0; - if (!r) { - const char* msg = is_auth_mode ? - "Unsupported state or unable to authenticate data" : - "Unsupported state"; - - return ThrowCryptoError(env, - ERR_get_error(), - msg); - } + bool r = cipher->Final(&out); + + if (!r) { + const char* msg = is_auth_mode + ? "Unsupported state or unable to authenticate data" + : "Unsupported state"; + + return ThrowCryptoError(env, ERR_get_error(), msg); } - Local buf = Buffer::New( - env, - reinterpret_cast(out_value), - out_len).ToLocalChecked(); - args.GetReturnValue().Set(buf); + args.GetReturnValue().Set(out.ToBuffer().ToLocalChecked()); } @@ -3848,18 +3842,17 @@ void Verify::VerifyFinal(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(verify_result); } - template -bool PublicKeyCipher::Cipher(const char* key_pem, +bool PublicKeyCipher::Cipher(Environment* env, + const char* key_pem, int key_pem_len, const char* passphrase, int padding, const unsigned char* data, int len, - unsigned char** out, - size_t* out_len) { + AllocatedBuffer* out) { EVPKeyPointer pkey; // Check if this is a PKCS#8 or RSA public key before trying as X.509 and @@ -3890,14 +3883,21 @@ bool PublicKeyCipher::Cipher(const char* key_pem, if (EVP_PKEY_CTX_set_rsa_padding(ctx.get(), padding) <= 0) return false; - if (EVP_PKEY_cipher(ctx.get(), nullptr, out_len, data, len) <= 0) + size_t out_len = 0; + if (EVP_PKEY_cipher(ctx.get(), nullptr, &out_len, data, len) <= 0) return false; - *out = Malloc(*out_len); + *out = env->AllocateManaged(out_len); - if (EVP_PKEY_cipher(ctx.get(), *out, out_len, data, len) <= 0) + if (EVP_PKEY_cipher(ctx.get(), + reinterpret_cast(out->data()), + &out_len, + data, + len) <= 0) { return false; + } + out->Resize(out_len); return true; } @@ -3921,35 +3921,24 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo& args) { String::Utf8Value passphrase(args.GetIsolate(), args[3]); - unsigned char* out_value = nullptr; - size_t out_len = 0; + AllocatedBuffer out; ClearErrorOnReturn clear_error_on_return; bool r = Cipher( + env, kbuf, klen, args.Length() >= 4 && !args[3]->IsNull() ? *passphrase : nullptr, padding, reinterpret_cast(buf), len, - &out_value, - &out_len); - - if (out_len == 0 || !r) { - free(out_value); - out_value = nullptr; - out_len = 0; - if (!r) { - return ThrowCryptoError(env, - ERR_get_error()); - } - } + &out); - Local vbuf = - Buffer::New(env, reinterpret_cast(out_value), out_len) - .ToLocalChecked(); - args.GetReturnValue().Set(vbuf); + if (!r) + return ThrowCryptoError(env, ERR_get_error()); + + args.GetReturnValue().Set(out.ToBuffer().ToLocalChecked()); } @@ -4150,9 +4139,9 @@ void DiffieHellman::GenerateKeys(const FunctionCallbackInfo& args) { const BIGNUM* pub_key; DH_get0_key(diffieHellman->dh_.get(), &pub_key, nullptr); size_t size = BN_num_bytes(pub_key); - char* data = Malloc(size); - BN_bn2bin(pub_key, reinterpret_cast(data)); - args.GetReturnValue().Set(Buffer::New(env, data, size).ToLocalChecked()); + AllocatedBuffer data = env->AllocateManaged(size); + BN_bn2bin(pub_key, reinterpret_cast(data.data())); + args.GetReturnValue().Set(data.ToBuffer().ToLocalChecked()); } @@ -4169,9 +4158,9 @@ void DiffieHellman::GetField(const FunctionCallbackInfo& args, if (num == nullptr) return env->ThrowError(err_if_null); size_t size = BN_num_bytes(num); - char* data = Malloc(size); - BN_bn2bin(num, reinterpret_cast(data)); - args.GetReturnValue().Set(Buffer::New(env, data, size).ToLocalChecked()); + AllocatedBuffer data = env->AllocateManaged(size); + BN_bn2bin(num, reinterpret_cast(data.data())); + args.GetReturnValue().Set(data.ToBuffer().ToLocalChecked()); } void DiffieHellman::GetPrime(const FunctionCallbackInfo& args) { @@ -4233,9 +4222,9 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo& args) { Buffer::Length(args[0]), 0)); - MallocedBuffer data(DH_size(diffieHellman->dh_.get())); + AllocatedBuffer ret = env->AllocateManaged(DH_size(diffieHellman->dh_.get())); - int size = DH_compute_key(reinterpret_cast(data.data), + int size = DH_compute_key(reinterpret_cast(ret.data()), key.get(), diffieHellman->dh_.get()); @@ -4270,14 +4259,13 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo& args) { // DH_compute_key returns number of bytes in a remainder of exponent, which // may have less bytes than a prime number. Therefore add 0-padding to the // allocated buffer. - if (static_cast(size) != data.size) { - CHECK_GT(data.size, static_cast(size)); - memmove(data.data + data.size - size, data.data, size); - memset(data.data, 0, data.size - size); + if (static_cast(size) != ret.size()) { + CHECK_GT(ret.size(), static_cast(size)); + memmove(ret.data() + ret.size() - size, ret.data(), size); + memset(ret.data(), 0, ret.size() - size); } - args.GetReturnValue().Set( - Buffer::New(env->isolate(), data.release(), data.size).ToLocalChecked()); + args.GetReturnValue().Set(ret.ToBuffer().ToLocalChecked()); } void DiffieHellman::SetKey(const v8::FunctionCallbackInfo& args, @@ -4455,15 +4443,14 @@ 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; - char* out = node::Malloc(out_len); + AllocatedBuffer out = env->AllocateManaged(out_len); - int r = ECDH_compute_key(out, out_len, pub.get(), ecdh->key_.get(), nullptr); - if (!r) { - free(out); + int r = ECDH_compute_key( + out.data(), out_len, pub.get(), ecdh->key_.get(), nullptr); + if (!r) return env->ThrowError("Failed to compute ECDH key"); - } - Local buf = Buffer::New(env, out, out_len).ToLocalChecked(); + Local buf = out.ToBuffer().ToLocalChecked(); args.GetReturnValue().Set(buf); } @@ -4505,15 +4492,13 @@ void ECDH::GetPrivateKey(const FunctionCallbackInfo& args) { return env->ThrowError("Failed to get ECDH private key"); int size = BN_num_bytes(b); - unsigned char* out = node::Malloc(size); + AllocatedBuffer out = env->AllocateManaged(size); - if (size != BN_bn2bin(b, out)) { - free(out); + if (size != BN_bn2bin(b, reinterpret_cast(out.data()))) { return env->ThrowError("Failed to convert ECDH private key to Buffer"); } - Local buf = - Buffer::New(env, reinterpret_cast(out), size).ToLocalChecked(); + Local buf = out.ToBuffer().ToLocalChecked(); args.GetReturnValue().Set(buf); } @@ -4976,31 +4961,28 @@ void VerifySpkac(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(verify_result); } - -char* ExportPublicKey(const char* data, int len, size_t* size) { - char* buf = nullptr; - +AllocatedBuffer ExportPublicKey(Environment* env, + const char* data, + int len, + size_t* size) { BIOPointer bio(BIO_new(BIO_s_mem())); - if (!bio) - return nullptr; + if (!bio) return AllocatedBuffer(); NetscapeSPKIPointer spki(NETSCAPE_SPKI_b64_decode(data, len)); - if (!spki) - return nullptr; + if (!spki) return AllocatedBuffer(); EVPKeyPointer pkey(NETSCAPE_SPKI_get_pubkey(spki.get())); - if (!pkey) - return nullptr; + if (!pkey) return AllocatedBuffer(); if (PEM_write_bio_PUBKEY(bio.get(), pkey.get()) <= 0) - return nullptr; + return AllocatedBuffer(); BUF_MEM* ptr; BIO_get_mem_ptr(bio.get(), &ptr); *size = ptr->length; - buf = Malloc(*size); - memcpy(buf, ptr->data, *size); + AllocatedBuffer buf = env->AllocateManaged(*size); + memcpy(buf.data(), ptr->data, *size); return buf; } @@ -5017,12 +4999,11 @@ void ExportPublicKey(const FunctionCallbackInfo& args) { CHECK_NOT_NULL(data); size_t pkey_size; - char* pkey = ExportPublicKey(data, length, &pkey_size); - if (pkey == nullptr) + AllocatedBuffer pkey = ExportPublicKey(env, data, length, &pkey_size); + if (pkey.data() == nullptr) return args.GetReturnValue().SetEmptyString(); - Local out = Buffer::New(env, pkey, pkey_size).ToLocalChecked(); - args.GetReturnValue().Set(out); + args.GetReturnValue().Set(pkey.ToBuffer().ToLocalChecked()); } diff --git a/src/node_crypto.h b/src/node_crypto.h index 1a93ae7a47e..b9bff4adde4 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -383,9 +383,8 @@ class CipherBase : public BaseObject { bool InitAuthenticated(const char* cipher_type, int iv_len, unsigned int auth_tag_len); bool CheckCCMMessageLength(int message_len); - UpdateResult Update(const char* data, int len, unsigned char** out, - int* out_len); - bool Final(unsigned char** out, int* out_len); + UpdateResult Update(const char* data, int len, AllocatedBuffer* out); + bool Final(AllocatedBuffer* out); bool SetAutoPadding(bool auto_padding); bool IsAuthenticatedMode() const; @@ -576,14 +575,14 @@ class PublicKeyCipher { template - static bool Cipher(const char* key_pem, + static bool Cipher(Environment* env, + const char* key_pem, int key_pem_len, const char* passphrase, int padding, const unsigned char* data, int len, - unsigned char** out, - size_t* out_len); + AllocatedBuffer* out); template AllocateManaged(suggested_size).release(); +} + // Callback used to receive inbound data from the i/o stream -void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { +void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { HandleScope handle_scope(env()->isolate()); Context::Scope context_scope(env()->context()); Http2Scope h2scope(this); CHECK_NOT_NULL(stream_); Debug(this, "receiving %d bytes", nread); - IncrementCurrentSessionMemory(buf.len); CHECK(stream_buf_ab_.IsEmpty()); + AllocatedBuffer buf(env(), buf_); if (nread <= 0) { - free(buf.base); if (nread < 0) { PassReadErrorToPreviousListener(nread); } } else { // Only pass data on if nread > 0 + // Shrink to the actual amount of used data. + buf.Resize(nread); + + IncrementCurrentSessionMemory(buf.size()); + // Makre sure that there was no read previously active. CHECK_NULL(stream_buf_.base); CHECK_EQ(stream_buf_.len, 0); // Remember the current buffer, so that OnDataChunkReceived knows the // offset of a DATA frame's data into the socket read buffer. - stream_buf_ = uv_buf_init(buf.base, nread); - - // Verify that currently: There is memory allocated into which - // the data has been read, and that memory buffer is at least as large - // as the amount of data we have read, but we have not yet made an - // ArrayBuffer out of it. - CHECK_LE(static_cast(nread), stream_buf_.len); + stream_buf_ = uv_buf_init(buf.data(), nread); Isolate* isolate = env()->isolate(); // Create an array buffer for the read data. DATA frames will be emitted // as slices of this array buffer to avoid having to copy memory. - stream_buf_ab_ = - ArrayBuffer::New(isolate, - buf.base, - nread, - v8::ArrayBufferCreationMode::kInternalized); + stream_buf_ab_ = buf.ToArrayBuffer(); statistics_.data_received += nread; ssize_t ret = Write(&stream_buf_, 1); @@ -1737,7 +1735,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { // Since we are finished handling this write, reset the stream buffer. // The memory has either been free()d or was handed over to V8. - DecrementCurrentSessionMemory(buf.len); + DecrementCurrentSessionMemory(buf.size()); stream_buf_ab_ = Local(); stream_buf_ = uv_buf_init(nullptr, 0); diff --git a/src/node_http2.h b/src/node_http2.h index d7f8d9acae9..8ea6e5fc36f 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -779,6 +779,7 @@ class Http2Session : public AsyncWrap, public StreamListener { } // Handle reads/writes from the underlying network transport. + uv_buf_t OnStreamAlloc(size_t suggested_size) override; void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override; void OnStreamAfterWrite(WriteWrap* w, int status) override; diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index a4b92cbb20b..852f925b9bc 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -531,10 +531,9 @@ class Parser : public AsyncWrap, public StreamListener { uv_buf_t OnStreamAlloc(size_t suggested_size) override { // For most types of streams, OnStreamRead will be immediately after // OnStreamAlloc, and will consume all data, so using a static buffer for - // reading is more efficient. For other streams, just use the default - // allocator, which uses Malloc(). + // reading is more efficient. For other streams, just use Malloc() directly. if (env()->http_parser_buffer_in_use()) - return StreamListener::OnStreamAlloc(suggested_size); + return uv_buf_init(Malloc(suggested_size), suggested_size); env()->set_http_parser_buffer_in_use(true); if (env()->http_parser_buffer() == nullptr) diff --git a/src/node_internals.h b/src/node_internals.h index fe446dbc713..3f66787fee8 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -416,10 +416,12 @@ v8::MaybeLocal New(Environment* env, size_t length, void (*callback)(char* data, void* hint), void* hint); -// Takes ownership of |data|. Must allocate |data| with malloc() or realloc() -// because ArrayBufferAllocator::Free() deallocates it again with free(). -// Mixing operator new and free() is undefined behavior so don't do that. -v8::MaybeLocal New(Environment* env, char* data, size_t length); +// Takes ownership of |data|. Must allocate |data| with the current Isolate's +// ArrayBuffer::Allocator(). +v8::MaybeLocal New(Environment* env, + char* data, + size_t length, + bool uses_malloc); inline v8::MaybeLocal New(Environment* env, @@ -450,7 +452,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); + ret = New(env, src, len_in_bytes, true); else if (!buf->IsInvalidated()) ret = Copy(env, src, len_in_bytes); diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 20e0c7673b8..e8b569be772 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -118,6 +118,21 @@ MaybeLocal Message::Deserialize(Environment* env, // Attach all transfered ArrayBuffers to their new Isolate. for (uint32_t i = 0; i < array_buffer_contents_.size(); ++i) { + if (!env->isolate_data()->uses_node_allocator()) { + // We don't use Node's allocator on the receiving side, so we have + // to create the ArrayBuffer from a copy of the memory. + AllocatedBuffer buf = + env->AllocateManaged(array_buffer_contents_[i].size); + memcpy(buf.data(), + array_buffer_contents_[i].data, + array_buffer_contents_[i].size); + deserializer.TransferArrayBuffer(i, buf.ToArrayBuffer()); + continue; + } + + env->isolate_data()->node_allocator()->RegisterPointer( + array_buffer_contents_[i].data, array_buffer_contents_[i].size); + Local ab = ArrayBuffer::New(env->isolate(), array_buffer_contents_[i].release(), @@ -266,6 +281,7 @@ Maybe Message::Serialize(Environment* env, // take ownership of its memory, copying the buffer will have to do. if (!ab->IsNeuterable() || ab->IsExternal()) continue; + } // We simply use the array index in the `array_buffers` list as the // ID that we write into the serialized buffer. uint32_t id = array_buffers.size(); @@ -311,6 +327,11 @@ Maybe Message::Serialize(Environment* env, // it inaccessible in this Isolate. ArrayBuffer::Contents contents = ab->Externalize(); ab->Neuter(); + + CHECK(env->isolate_data()->uses_node_allocator()); + env->isolate_data()->node_allocator()->UnregisterPointer( + contents.Data(), contents.ByteLength()); + array_buffer_contents_.push_back( MallocedBuffer { static_cast(contents.Data()), contents.ByteLength() }); diff --git a/src/node_serdes.cc b/src/node_serdes.cc index 5de0ddd8190..79df651664e 100644 --- a/src/node_serdes.cc +++ b/src/node_serdes.cc @@ -205,10 +205,13 @@ void SerializerContext::ReleaseBuffer(const FunctionCallbackInfo& args) { SerializerContext* ctx; ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Holder()); + // Note: Both ValueSerializer and this Buffer::New() variant use malloc() + // as the underlying allocator. std::pair ret = ctx->serializer_.Release(); auto buf = Buffer::New(ctx->env(), reinterpret_cast(ret.first), - ret.second); + ret.second, + true /* uses_malloc */); if (!buf.IsEmpty()) { args.GetReturnValue().Set(buf.ToLocalChecked()); diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 027b938d30d..21cf0d6d94b 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -412,18 +412,13 @@ inline void ShutdownWrap::OnDone(int status) { Dispose(); } -inline void WriteWrap::SetAllocatedStorage(char* data, size_t size) { - CHECK_NULL(storage_); - storage_ = data; - storage_size_ = size; -} - -inline char* WriteWrap::Storage() { - return storage_; +inline size_t WriteWrap::StorageSize() const { + return storage_.size(); } -inline size_t WriteWrap::StorageSize() const { - return storage_size_; +inline void WriteWrap::SetAllocatedStorage(AllocatedBuffer&& storage) { + CHECK_NULL(storage_.data()); + storage_ = std::move(storage); } inline void WriteWrap::OnDone(int status) { diff --git a/src/stream_base.cc b/src/stream_base.cc index f429f3593fd..c8400f69833 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -127,9 +127,9 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { } } - std::unique_ptr storage; + AllocatedBuffer storage; if (storage_size > 0) - storage = std::unique_ptr(Malloc(storage_size)); + storage = env->AllocateManaged(storage_size); offset = 0; if (!all_buffers) { @@ -145,8 +145,8 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { // Write string CHECK_LE(offset, storage_size); - char* str_storage = storage.get() + offset; - size_t str_size = storage_size - offset; + char* str_storage = storage.data() + offset; + size_t str_size = storage.size() - offset; Local string = chunk->ToString(env->context()).ToLocalChecked(); enum encoding encoding = ParseEncoding(env->isolate(), @@ -164,8 +164,8 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { StreamWriteResult res = Write(*bufs, count, nullptr, req_wrap_obj); SetWriteResultPropertiesOnWrapObject(env, req_wrap_obj, res); - if (res.wrap != nullptr && storage) { - res.wrap->SetAllocatedStorage(storage.release(), storage_size); + if (res.wrap != nullptr && storage_size > 0) { + res.wrap->SetAllocatedStorage(std::move(storage)); } return res.err; } @@ -265,18 +265,18 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { CHECK_EQ(count, 1); } - std::unique_ptr data; + AllocatedBuffer data; if (try_write) { // Copy partial data - data = std::unique_ptr(Malloc(buf.len)); - memcpy(data.get(), buf.base, buf.len); + data = env->AllocateManaged(buf.len); + memcpy(data.data(), buf.base, buf.len); data_size = buf.len; } else { // Write it - data = std::unique_ptr(Malloc(storage_size)); + data = env->AllocateManaged(storage_size); data_size = StringBytes::Write(env->isolate(), - data.get(), + data.data(), storage_size, string, enc); @@ -284,7 +284,7 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { CHECK_LE(data_size, storage_size); - buf = uv_buf_init(data.get(), data_size); + buf = uv_buf_init(data.data(), data_size); uv_stream_t* send_handle = nullptr; @@ -302,7 +302,7 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { SetWriteResultPropertiesOnWrapObject(env, req_wrap_obj, res); if (res.wrap != nullptr) { - res.wrap->SetAllocatedStorage(data.release(), data_size); + res.wrap->SetAllocatedStorage(std::move(data)); } return res.err; @@ -356,31 +356,30 @@ void StreamResource::ClearError() { // No-op } - -uv_buf_t StreamListener::OnStreamAlloc(size_t suggested_size) { - return uv_buf_init(Malloc(suggested_size), suggested_size); +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(); } - -void EmitToJSStreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { +void EmitToJSStreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { CHECK_NOT_NULL(stream_); StreamBase* stream = static_cast(stream_); Environment* env = stream->stream_env(); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); + AllocatedBuffer buf(env, buf_); if (nread <= 0) { - free(buf.base); if (nread < 0) stream->CallJSOnreadMethod(nread, Local()); return; } - CHECK_LE(static_cast(nread), buf.len); - char* base = Realloc(buf.base, nread); + CHECK_LE(static_cast(nread), buf.size()); + buf.Resize(nread); - Local obj = Buffer::New(env, base, nread).ToLocalChecked(); - stream->CallJSOnreadMethod(nread, obj); + stream->CallJSOnreadMethod(nread, buf.ToBuffer().ToLocalChecked()); } diff --git a/src/stream_base.h b/src/stream_base.h index 05c2a962362..597ae0c9062 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -74,24 +74,18 @@ class ShutdownWrap : public StreamReq { class WriteWrap : public StreamReq { public: - char* Storage(); size_t StorageSize() const; - void SetAllocatedStorage(char* data, size_t size); + void SetAllocatedStorage(AllocatedBuffer&& storage); WriteWrap(StreamBase* stream, v8::Local req_wrap_obj) : StreamReq(stream, req_wrap_obj) { } - ~WriteWrap() { - free(storage_); - } - // Call stream()->EmitAfterWrite() and dispose of this request wrap. void OnDone(int status) override; private: - char* storage_ = nullptr; - size_t storage_size_ = 0; + AllocatedBuffer storage_; }; @@ -115,7 +109,7 @@ class StreamListener { // It is not valid to return a zero-length buffer from this method. // It is not guaranteed that the corresponding `OnStreamRead()` call // happens in the same event loop turn as this call. - virtual uv_buf_t OnStreamAlloc(size_t suggested_size); + virtual uv_buf_t OnStreamAlloc(size_t suggested_size) = 0; // `OnStreamRead()` is called when data is available on the socket and has // been read into the buffer provided by `OnStreamAlloc()`. @@ -181,6 +175,7 @@ class ReportWritesToJSStreamListener : public StreamListener { // JS land via the handle’s .ondata method. class EmitToJSStreamListener : public ReportWritesToJSStreamListener { public: + uv_buf_t OnStreamAlloc(size_t suggested_size) override; void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override; }; diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc index e19f98e35d2..2444b6edb8b 100644 --- a/src/stream_pipe.cc +++ b/src/stream_pipe.cc @@ -109,17 +109,17 @@ 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 uv_buf_init(Malloc(size), size); + return pipe->env()->AllocateManaged(size).release(); } void StreamPipe::ReadableListener::OnStreamRead(ssize_t nread, - const uv_buf_t& buf) { + const uv_buf_t& buf_) { StreamPipe* pipe = ContainerOf(&StreamPipe::readable_listener_, this); + AllocatedBuffer buf(pipe->env(), buf_); AsyncScope async_scope(pipe); if (nread < 0) { // EOF or error; stop reading and pass the error to the previous listener // (which might end up in JS). - free(buf.base); pipe->is_eof_ = true; stream()->ReadStop(); CHECK_NOT_NULL(previous_listener_); @@ -133,19 +133,18 @@ void StreamPipe::ReadableListener::OnStreamRead(ssize_t nread, return; } - pipe->ProcessData(nread, buf); + pipe->ProcessData(nread, std::move(buf)); } -void StreamPipe::ProcessData(size_t nread, const uv_buf_t& buf) { - uv_buf_t buffer = uv_buf_init(buf.base, nread); +void StreamPipe::ProcessData(size_t nread, AllocatedBuffer&& buf) { + uv_buf_t buffer = uv_buf_init(buf.data(), nread); StreamWriteResult res = sink()->Write(&buffer, 1); if (!res.async) { - free(buf.base); writable_listener_.OnStreamAfterWrite(nullptr, res.err); } else { is_writing_ = true; is_reading_ = false; - res.wrap->SetAllocatedStorage(buf.base, buf.len); + res.wrap->SetAllocatedStorage(std::move(buf)); if (source() != nullptr) source()->ReadStop(); } diff --git a/src/stream_pipe.h b/src/stream_pipe.h index c76afac4168..ea9ea853fed 100644 --- a/src/stream_pipe.h +++ b/src/stream_pipe.h @@ -44,7 +44,7 @@ class StreamPipe : public AsyncWrap { // `OnStreamWantsWrite()` support. size_t wanted_data_ = 0; - void ProcessData(size_t nread, const uv_buf_t& buf); + void ProcessData(size_t nread, AllocatedBuffer&& buf); class ReadableListener : public StreamListener { public: diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 724f98c0cc9..b201a40e4c8 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -455,25 +455,23 @@ void UDPWrap::OnSend(uv_udp_send_t* req, int status) { void UDPWrap::OnAlloc(uv_handle_t* handle, size_t suggested_size, uv_buf_t* buf) { - buf->base = node::Malloc(suggested_size); - buf->len = suggested_size; + UDPWrap* wrap = static_cast(handle->data); + *buf = wrap->env()->AllocateManaged(suggested_size).release(); } - void UDPWrap::OnRecv(uv_udp_t* handle, ssize_t nread, - const uv_buf_t* buf, + const uv_buf_t* buf_, const struct sockaddr* addr, unsigned int flags) { + UDPWrap* wrap = static_cast(handle->data); + Environment* env = wrap->env(); + + AllocatedBuffer buf(env, *buf_); if (nread == 0 && addr == nullptr) { - if (buf->base != nullptr) - free(buf->base); return; } - UDPWrap* wrap = static_cast(handle->data); - Environment* env = wrap->env(); - HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -486,14 +484,12 @@ void UDPWrap::OnRecv(uv_udp_t* handle, }; if (nread < 0) { - if (buf->base != nullptr) - free(buf->base); wrap->MakeCallback(env->onmessage_string(), arraysize(argv), argv); return; } - char* base = node::UncheckedRealloc(buf->base, nread); - argv[2] = Buffer::New(env, base, nread).ToLocalChecked(); + buf.Resize(nread); + argv[2] = buf.ToBuffer().ToLocalChecked(); argv[3] = AddressToJS(env, addr); wrap->MakeCallback(env->onmessage_string(), arraysize(argv), argv); } From 55381261b8ee08f789bf5c3e5e03a2633473aea2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 18 Feb 2019 17:30:42 +0100 Subject: [PATCH 05/12] src: make IsolateData store ArrayBufferAllocator This enables us to identify whether we are using an allocator that we know more about than what the generic `ArrayBuffer::Allocator` API provides, in particular whether it is `malloc()`-compatible. PR-URL: https://github.com/nodejs/node/pull/26207 Reviewed-By: James M Snell Reviewed-By: Joyee Cheung --- src/env-inl.h | 12 ++++++++++-- src/env.cc | 13 ++++++++----- src/env.h | 14 ++++++++++---- src/node.cc | 2 +- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 4e0a95ed424..0223e732ec8 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -47,8 +47,16 @@ inline uv_loop_t* IsolateData::event_loop() const { return event_loop_; } -inline uint32_t* IsolateData::zero_fill_field() const { - return zero_fill_field_; +inline bool IsolateData::uses_node_allocator() const { + return uses_node_allocator_; +} + +inline v8::ArrayBuffer::Allocator* IsolateData::allocator() const { + return allocator_; +} + +inline ArrayBufferAllocator* IsolateData::node_allocator() const { + return node_allocator_; } inline MultiIsolatePlatform* IsolateData::platform() const { diff --git a/src/env.cc b/src/env.cc index d76e691a53f..aaf16c5ddc8 100644 --- a/src/env.cc +++ b/src/env.cc @@ -36,11 +36,14 @@ void* Environment::kNodeContextTagPtr = const_cast( IsolateData::IsolateData(Isolate* isolate, uv_loop_t* event_loop, MultiIsolatePlatform* platform, - uint32_t* zero_fill_field) : - isolate_(isolate), - event_loop_(event_loop), - zero_fill_field_(zero_fill_field), - platform_(platform) { + ArrayBufferAllocator* node_allocator) + : isolate_(isolate), + event_loop_(event_loop), + allocator_(isolate->GetArrayBufferAllocator()), + node_allocator_(node_allocator), + uses_node_allocator_(allocator_ == node_allocator_), + platform_(platform) { + CHECK_NOT_NULL(allocator_); if (platform_ != nullptr) platform_->RegisterIsolate(isolate_, event_loop); diff --git a/src/env.h b/src/env.h index 4b678cee289..23725ff70a6 100644 --- a/src/env.h +++ b/src/env.h @@ -362,15 +362,19 @@ class Environment; class IsolateData { public: - IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop, + IsolateData(v8::Isolate* isolate, + uv_loop_t* event_loop, MultiIsolatePlatform* platform = nullptr, - uint32_t* zero_fill_field = nullptr); + ArrayBufferAllocator* node_allocator = nullptr); ~IsolateData(); inline uv_loop_t* event_loop() const; - inline uint32_t* zero_fill_field() const; inline MultiIsolatePlatform* platform() const; inline std::shared_ptr options(); + inline bool uses_node_allocator() const; + inline v8::ArrayBuffer::Allocator* allocator() const; + inline ArrayBufferAllocator* node_allocator() const; + #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) @@ -401,7 +405,9 @@ class IsolateData { v8::Isolate* const isolate_; uv_loop_t* const event_loop_; - uint32_t* const zero_fill_field_; + v8::ArrayBuffer::Allocator* const allocator_; + ArrayBufferAllocator* const node_allocator_; + const bool uses_node_allocator_; MultiIsolatePlatform* platform_; std::shared_ptr options_; diff --git a/src/node.cc b/src/node.cc index aa51c0b46a1..47a13077db0 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2832,7 +2832,7 @@ IsolateData* CreateIsolateData( uv_loop_t* loop, MultiIsolatePlatform* platform, ArrayBufferAllocator* allocator) { - return new IsolateData(isolate, loop, platform, allocator->zero_fill_field()); + return new IsolateData(isolate, loop, platform, allocator); } From 6418b510245bac35fd7679a6f7b6e877078874d3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 19 Feb 2019 14:56:52 +0100 Subject: [PATCH 06/12] worker: copy transferList ArrayBuffers on unknown allocator If the `ArrayBuffer::Allocator` used to create `ArrayBuffer`s in the current `Isolate` is not a Node.js `ArrayBufferAllocator`, we cannot know that it is `malloc()`-based, an in particular it might not be compatible with the `ArrayBuffer::Allocator` on the receiving end of the connection. PR-URL: https://github.com/nodejs/node/pull/26207 Reviewed-By: James M Snell Reviewed-By: Joyee Cheung --- src/node_messaging.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node_messaging.cc b/src/node_messaging.cc index e8b569be772..7d6c6ad80cd 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -279,7 +279,8 @@ Maybe Message::Serialize(Environment* env, Local ab = entry.As(); // If we cannot render the ArrayBuffer unusable in this Isolate and // take ownership of its memory, copying the buffer will have to do. - if (!ab->IsNeuterable() || ab->IsExternal()) + if (!ab->IsNeuterable() || ab->IsExternal() || + !env->isolate_data()->uses_node_allocator()) { continue; } // We simply use the array index in the `array_buffers` list as the From 8867a265bc627d3296d907a06235548fa7ec72a7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 18 Feb 2019 17:30:10 +0100 Subject: [PATCH 07/12] src: add debugging array allocator Add a subclass of `ArrayBufferAllocator` that performs additional debug checking, which in particular verifies that: - All `ArrayBuffer` backing stores have been allocated with this allocator, or have been explicitly marked as coming from a compatible source. - All memory allocated by the allocator has been freed once it is destroyed. PR-URL: https://github.com/nodejs/node/pull/26207 Reviewed-By: James M Snell Reviewed-By: Joyee Cheung --- src/node.cc | 76 +++++++++++++++++++++++++++++++++++++++++++- src/node_internals.h | 29 +++++++++++++++-- src/node_options.cc | 4 +++ src/node_options.h | 1 + 4 files changed, 106 insertions(+), 4 deletions(-) diff --git a/src/node.cc b/src/node.cc index 47a13077db0..21a9c9beefa 100644 --- a/src/node.cc +++ b/src/node.cc @@ -629,6 +629,77 @@ void* ArrayBufferAllocator::Allocate(size_t size) { return UncheckedMalloc(size); } +DebuggingArrayBufferAllocator::~DebuggingArrayBufferAllocator() { + CHECK(allocations_.empty()); +} + +void* DebuggingArrayBufferAllocator::Allocate(size_t size) { + Mutex::ScopedLock lock(mutex_); + void* data = ArrayBufferAllocator::Allocate(size); + RegisterPointerInternal(data, size); + return data; +} + +void* DebuggingArrayBufferAllocator::AllocateUninitialized(size_t size) { + Mutex::ScopedLock lock(mutex_); + void* data = ArrayBufferAllocator::AllocateUninitialized(size); + RegisterPointerInternal(data, size); + return data; +} + +void DebuggingArrayBufferAllocator::Free(void* data, size_t size) { + Mutex::ScopedLock lock(mutex_); + UnregisterPointerInternal(data, size); + ArrayBufferAllocator::Free(data, size); +} + +void* DebuggingArrayBufferAllocator::Reallocate(void* data, + size_t old_size, + size_t size) { + Mutex::ScopedLock lock(mutex_); + void* ret = ArrayBufferAllocator::Reallocate(data, old_size, size); + if (ret == nullptr) { + if (size == 0) // i.e. equivalent to free(). + UnregisterPointerInternal(data, old_size); + return nullptr; + } + + if (data != nullptr) { + auto it = allocations_.find(data); + CHECK_NE(it, allocations_.end()); + allocations_.erase(it); + } + + RegisterPointerInternal(ret, size); + return ret; +} + +void DebuggingArrayBufferAllocator::RegisterPointer(void* data, size_t size) { + Mutex::ScopedLock lock(mutex_); + RegisterPointerInternal(data, size); +} + +void DebuggingArrayBufferAllocator::UnregisterPointer(void* data, size_t size) { + Mutex::ScopedLock lock(mutex_); + UnregisterPointerInternal(data, size); +} + +void DebuggingArrayBufferAllocator::UnregisterPointerInternal(void* data, + size_t size) { + if (data == nullptr) return; + auto it = allocations_.find(data); + CHECK_NE(it, allocations_.end()); + CHECK_EQ(it->second, size); + allocations_.erase(it); +} + +void DebuggingArrayBufferAllocator::RegisterPointerInternal(void* data, + size_t size) { + if (data == nullptr) return; + CHECK_EQ(allocations_.count(data), 0); + allocations_[data] = size; +} + namespace { bool ShouldAbortOnUncaughtException(Isolate* isolate) { @@ -2805,7 +2876,10 @@ int EmitExit(Environment* env) { ArrayBufferAllocator* CreateArrayBufferAllocator() { - return new ArrayBufferAllocator(); + if (per_process_opts->debug_arraybuffer_allocations) + return new DebuggingArrayBufferAllocator(); + else + return new ArrayBufferAllocator(); } diff --git a/src/node_internals.h b/src/node_internals.h index 3f66787fee8..9f4fdf2e162 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -398,15 +398,38 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator { public: inline uint32_t* zero_fill_field() { return &zero_fill_field_; } - virtual void* Allocate(size_t size); // Defined in src/node.cc - virtual void* AllocateUninitialized(size_t size) + void* Allocate(size_t size) override; // Defined in src/node.cc + void* AllocateUninitialized(size_t size) override { return node::UncheckedMalloc(size); } - virtual void Free(void* data, size_t) { free(data); } + void Free(void* data, size_t) override { free(data); } + virtual void* Reallocate(void* data, size_t old_size, size_t size) { + return static_cast( + UncheckedRealloc(static_cast(data), size)); + } + virtual void RegisterPointer(void* data, size_t size) {} + virtual void UnregisterPointer(void* data, size_t size) {} private: uint32_t zero_fill_field_ = 1; // Boolean but exposed as uint32 to JS land. }; +class DebuggingArrayBufferAllocator final : public ArrayBufferAllocator { + public: + ~DebuggingArrayBufferAllocator() override; + void* Allocate(size_t size) override; + void* AllocateUninitialized(size_t size) override; + void Free(void* data, size_t size) override; + void* Reallocate(void* data, size_t old_size, size_t size) override; + void RegisterPointer(void* data, size_t size) override; + void UnregisterPointer(void* data, size_t size) override; + + private: + void RegisterPointerInternal(void* data, size_t size); + void UnregisterPointerInternal(void* data, size_t size); + Mutex mutex_; + std::unordered_map allocations_; +}; + namespace Buffer { v8::MaybeLocal Copy(Environment* env, const char* data, size_t len); v8::MaybeLocal New(Environment* env, size_t size); diff --git a/src/node_options.cc b/src/node_options.cc index 5cce1179b79..c34e29fdda3 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -223,6 +223,10 @@ PerProcessOptionsParser::PerProcessOptionsParser() { "SlowBuffer instances", &PerProcessOptions::zero_fill_all_buffers, kAllowedInEnvironment); + AddOption("--debug-arraybuffer-allocations", + "", /* undocumented, only for debugging */ + &PerProcessOptions::debug_arraybuffer_allocations, + kAllowedInEnvironment); AddOption("--security-reverts", "", &PerProcessOptions::security_reverts); AddOption("--help", diff --git a/src/node_options.h b/src/node_options.h index ec0179d890f..c19099dfa2d 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -111,6 +111,7 @@ class PerProcessOptions { std::string trace_event_file_pattern = "node_trace.${rotation}.log"; int64_t v8_thread_pool_size = 4; bool zero_fill_all_buffers = false; + bool debug_arraybuffer_allocations = false; std::vector security_reverts; bool print_help = false; From b2ed6c35ab673c31dd1ca7a415e8534eb6891567 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 18 Feb 2019 22:58:19 +0100 Subject: [PATCH 08/12] src: add allocation utils to env Add a RAII utility for managing blocks of memory that have been allocated with the `ArrayBuffer::Allocator` for a given `Isolate`. PR-URL: https://github.com/nodejs/node/pull/26207 Reviewed-By: James M Snell Reviewed-By: Joyee Cheung --- src/env-inl.h | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++ src/env.cc | 18 ++++++++++ src/env.h | 41 +++++++++++++++++++++ 3 files changed, 157 insertions(+) diff --git a/src/env-inl.h b/src/env-inl.h index 0223e732ec8..bff754f5a0e 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -669,6 +669,104 @@ 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); +} + +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) { + char* new_data = env_->Reallocate(buffer_.base, buffer_.len, len); + CHECK_IMPLIES(len > 0, new_data != nullptr); + 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(); + 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 { +v8::MaybeLocal New(Environment* env, + char* data, + size_t length, + 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(); + return v8::ArrayBuffer::New(env_->isolate(), + buf.base, + buf.len, + v8::ArrayBufferCreationMode::kInternalized); +} + inline void Environment::ThrowError(const char* errmsg) { ThrowError(v8::Exception::Error, errmsg); } diff --git a/src/env.cc b/src/env.cc index aaf16c5ddc8..47cd43e8394 100644 --- a/src/env.cc +++ b/src/env.cc @@ -13,6 +13,7 @@ namespace node { +using v8::ArrayBuffer; using v8::Context; using v8::FunctionTemplate; using v8::HandleScope; @@ -691,6 +692,23 @@ void Environment::BuildEmbedderGraph(v8::Isolate* isolate, }); } +char* Environment::Reallocate(char* data, size_t old_size, size_t size) { + // 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; +} // Not really any better place than env.cc at this moment. void BaseObject::DeleteMe(void* data) { diff --git a/src/env.h b/src/env.h index 23725ff70a6..e0ef958dbcd 100644 --- a/src/env.h +++ b/src/env.h @@ -434,6 +434,38 @@ enum class DebugCategory { CATEGORY_COUNT }; +// 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 Environment { public: class AsyncHooks { @@ -648,6 +680,15 @@ class Environment { inline IsolateData* isolate_data() const; + // Utilites 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 bool printed_error() const; inline void set_printed_error(bool value); From 7b3564269c2fab1ced6bf8fdb01f35ea78897c8d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 28 Oct 2018 15:59:20 +0100 Subject: [PATCH 09/12] buffer: throw exception when creating from non-Node.js Context Throw an exception instead of crashing when attempting to create `Buffer` objects from a Context that is not associated with a Node.js `Environment`. Possible alternatives for the future might be just returning a plain `Uint8Array`, or working on providing `Buffer` for all `Context`s. PR-URL: https://github.com/nodejs/node/pull/23938 Reviewed-By: James M Snell Reviewed-By: Daniel Bevenius --- doc/api/errors.md | 13 +++++++++++++ src/node_buffer.cc | 22 ++++++++++++++++++---- src/node_errors.h | 8 +++++++- 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 6a286772dee..a90080e79bc 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -613,6 +613,19 @@ An attempt was made to register something that is not a function as an The type of an asynchronous resource was invalid. Note that users are also able to define their own types if using the public embedder API. + +### ERR_BUFFER_CONTEXT_NOT_AVAILABLE + +An attempt was made to create a Node.js `Buffer` instance from addon or embedder +code, while in a JS engine Context that is not associated with a Node.js +instance. The data passed to the `Buffer` method will have been released +by the time the method returns. + +When encountering this error, a possible alternative to creating a `Buffer` +instance is to create a normal `Uint8Array`, which only differs in the +prototype of the resulting object. `Uint8Array`s are generally accepted in all +Node.js core APIs where `Buffer`s are; they are available in all Contexts. + ### ERR_BUFFER_OUT_OF_BOUNDS diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 2da7fcb0891..6f739a0001c 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -256,7 +256,10 @@ MaybeLocal New(Isolate* isolate, size_t length) { EscapableHandleScope handle_scope(isolate); Local obj; Environment* env = Environment::GetCurrent(isolate); - CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. + if (env == nullptr) { + THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate); + return MaybeLocal(); + } if (Buffer::New(env, length).ToLocal(&obj)) return handle_scope.Escape(obj); return Local(); @@ -287,7 +290,10 @@ MaybeLocal New(Environment* env, size_t length) { MaybeLocal Copy(Isolate* isolate, const char* data, size_t length) { EscapableHandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(isolate); - CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. + if (env == nullptr) { + THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate); + return MaybeLocal(); + } Local obj; if (Buffer::Copy(env, data, length).ToLocal(&obj)) return handle_scope.Escape(obj); @@ -325,7 +331,11 @@ MaybeLocal New(Isolate* isolate, void* hint) { EscapableHandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(isolate); - CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. + if (env == nullptr) { + callback(data, hint); + THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate); + return MaybeLocal(); + } Local obj; if (Buffer::New(env, data, length, callback, hint).ToLocal(&obj)) return handle_scope.Escape(obj); @@ -365,7 +375,11 @@ MaybeLocal New(Environment* env, MaybeLocal New(Isolate* isolate, char* data, size_t length) { EscapableHandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(isolate); - CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. + if (env == nullptr) { + free(data); + THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate); + return MaybeLocal(); + } Local obj; if (Buffer::New(env, data, length, true).ToLocal(&obj)) return handle_scope.Escape(obj); diff --git a/src/node_errors.h b/src/node_errors.h index 976b864a670..eb9d61656e6 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -17,6 +17,7 @@ namespace node { // a `Local` containing the TypeError with proper code and message #define ERRORS_WITH_CODE(V) \ + V(ERR_BUFFER_CONTEXT_NOT_AVAILABLE, Error) \ V(ERR_BUFFER_OUT_OF_BOUNDS, RangeError) \ V(ERR_BUFFER_TOO_LARGE, Error) \ V(ERR_CANNOT_TRANSFER_OBJECT, TypeError) \ @@ -55,6 +56,8 @@ namespace node { // Errors with predefined static messages #define PREDEFINED_ERROR_MESSAGES(V) \ + V(ERR_BUFFER_CONTEXT_NOT_AVAILABLE, \ + "Buffer is not available for the current Context") \ V(ERR_CANNOT_TRANSFER_OBJECT, "Cannot transfer object of unsupported type")\ V(ERR_CLOSED_MESSAGE_PORT, "Cannot send data on closed MessagePort") \ V(ERR_CONSTRUCT_CALL_REQUIRED, "Cannot call constructor without `new`") \ @@ -73,8 +76,11 @@ namespace node { inline v8::Local code(v8::Isolate* isolate) { \ return code(isolate, message); \ } \ + inline void THROW_ ## code(v8::Isolate* isolate) { \ + isolate->ThrowException(code(isolate, message)); \ + } \ inline void THROW_ ## code(Environment* env) { \ - env->isolate()->ThrowException(code(env->isolate(), message)); \ + THROW_ ## code(env->isolate()); \ } PREDEFINED_ERROR_MESSAGES(V) #undef V From e13e8fe0bff67de3c600096ae0c1818b22a082a8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 13 Jan 2019 21:53:31 +0100 Subject: [PATCH 10/12] src: remove outdated `Neuter()` call in `node_buffer.cc` This call was introduced in 827ee498e332e3 to avoid a crash in a later `Neuter()` call that has later been removed in ebbbc5a790db69, rendering the original call unnecessary. Refs: https://github.com/nodejs/node/pull/3624 Refs: https://github.com/nodejs/node/pull/5204 PR-URL: https://github.com/nodejs/node/pull/25479 Reviewed-By: Anatoli Papirovski --- src/node_buffer.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 6f739a0001c..94f978f89af 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -355,11 +355,6 @@ MaybeLocal New(Environment* env, } Local ab = ArrayBuffer::New(env->isolate(), data, length); - // `Neuter()`ing is required here to prevent materialization of the backing - // store in v8. `nullptr` buffers are not writable, so this is semantically - // correct. - if (data == nullptr) - ab->Neuter(); MaybeLocal ui = Buffer::New(env, ab, 0, length); if (ui.IsEmpty()) { From 9027c0ff04908103bdf2650fb01464db45db2448 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 25 Jan 2019 04:34:38 +0000 Subject: [PATCH 11/12] tls: fix malloc mismatch in SSL_set_tlsext_status_ocsp_resp call SSL_set_tlsext_status_ocsp_resp expects the data to be allocated with OPENSSL_malloc, not libc malloc, so use OpenSSLMalloc. Additionally, though OpenSSL doesn't type-check due to it being a macro, the function is documented to take an unsigned char pointer: https://www.openssl.org/docs/man1.1.0/ssl/SSL_set_tlsext_status_ocsp_resp.html (By default, OPENSSL_malloc is the same as libc malloc, but it is possible to customize this.) PR-URL: https://github.com/nodejs/node/pull/25706 Reviewed-By: Sam Roberts Reviewed-By: Ali Ijaz Sheikh Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- src/node_crypto.cc | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c170f4ebca7..0194c8c5bbb 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -323,6 +323,14 @@ bool EntropySource(unsigned char* buffer, size_t length) { } +template +static T* MallocOpenSSL(size_t count) { + void* mem = OPENSSL_malloc(MultiplyWithOverflowCheck(count, sizeof(T))); + CHECK_IMPLIES(mem == nullptr, count == 0); + return static_cast(mem); +} + + void SecureContext::Initialize(Environment* env, Local target) { Local t = env->NewFunctionTemplate(New); t->InstanceTemplate()->SetInternalFieldCount(1); @@ -2355,12 +2363,11 @@ int SSLWrap::TLSExtStatusCallback(SSL* s, void* arg) { size_t len = Buffer::Length(obj); // OpenSSL takes control of the pointer after accepting it - auto* allocator = env->isolate()->GetArrayBufferAllocator(); - uint8_t* data = static_cast(allocator->AllocateUninitialized(len)); + unsigned char* data = MallocOpenSSL(len); memcpy(data, resp, len); if (!SSL_set_tlsext_status_ocsp_resp(s, data, len)) - allocator->Free(data, len); + OPENSSL_free(data); w->ocsp_response_.Reset(); return SSL_TLSEXT_ERR_OK; From 1ca642c77b91ba2c74f80bd8955210649dc6729e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 26 May 2019 17:48:47 +0200 Subject: [PATCH 12/12] http2: fix tracking received data for maxSessionMemory Track received data correctly. Specifically, for the buffer that is used for receiving data, we previously would try to increment the current memory usage by its length, and later decrement it by that, but in the meantime the buffer had been turned over to V8 and its length reset to zero. This gave the impression that more and more memory was consumed by the HTTP/2 session when it was in fact not. Fixes: https://github.com/nodejs/node/issues/27416 Refs: https://github.com/nodejs/node/pull/26207 PR-URL: https://github.com/nodejs/node/pull/27914 Reviewed-By: Colin Ihrig Reviewed-By: Rich Trott --- src/node_http2.cc | 4 +- .../test-http2-max-session-memory-leak.js | 46 +++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-http2-max-session-memory-leak.js diff --git a/src/node_http2.cc b/src/node_http2.cc index f93b361a908..dcfa1694f93 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1702,7 +1702,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { // Shrink to the actual amount of used data. buf.Resize(nread); - IncrementCurrentSessionMemory(buf.size()); + IncrementCurrentSessionMemory(nread); // Makre sure that there was no read previously active. CHECK_NULL(stream_buf_.base); @@ -1735,7 +1735,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { // Since we are finished handling this write, reset the stream buffer. // The memory has either been free()d or was handed over to V8. - DecrementCurrentSessionMemory(buf.size()); + DecrementCurrentSessionMemory(nread); stream_buf_ab_ = Local(); stream_buf_ = uv_buf_init(nullptr, 0); diff --git a/test/parallel/test-http2-max-session-memory-leak.js b/test/parallel/test-http2-max-session-memory-leak.js new file mode 100644 index 00000000000..b066ca80bc5 --- /dev/null +++ b/test/parallel/test-http2-max-session-memory-leak.js @@ -0,0 +1,46 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const http2 = require('http2'); + +// Regression test for https://github.com/nodejs/node/issues/27416. +// Check that received data is accounted for correctly in the maxSessionMemory +// mechanism. + +const bodyLength = 8192; +const maxSessionMemory = 1; // 1 MB +const requestCount = 1000; + +const server = http2.createServer({ maxSessionMemory }); +server.on('stream', (stream) => { + stream.respond(); + stream.end(); +}); + +server.listen(common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`, { + maxSessionMemory + }); + + function request() { + return new Promise((resolve, reject) => { + const stream = client.request({ + ':method': 'POST', + 'content-length': bodyLength + }); + stream.on('error', reject); + stream.on('response', resolve); + stream.end('a'.repeat(bodyLength)); + }); + } + + (async () => { + for (let i = 0; i < requestCount; i++) { + await request(); + } + + client.close(); + server.close(); + })().then(common.mustCall()); +}));