From 64577463dfbc7ebe646c6c5a06e4b309b2757205 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 7 Aug 2015 23:03:00 +0200 Subject: [PATCH 1/4] src: plug memory leaks In a few places dynamic memory was passed to the Buffer::New() overload that makes a copy of the input, not the one that takes ownership. This commit is a band-aid to fix the memory leaks. Longer term, we should look into using C++11 move semantics more effectively. Fixes: https://github.com/nodejs/node/issues/2308 PR-URL: https://github.com/nodejs/node/pull/2352 Reviewed-By: Fedor Indutny Reviewed-By: Trevor Norris --- src/node_buffer.h | 5 +++++ src/node_crypto.cc | 6 +++--- src/stream_wrap.cc | 2 +- src/udp_wrap.cc | 2 +- 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/node_buffer.h b/src/node_buffer.h index 5b935b8c063abc..f7c88f60e03dea 100644 --- a/src/node_buffer.h +++ b/src/node_buffer.h @@ -67,12 +67,17 @@ static inline bool IsWithinBounds(size_t off, size_t len, size_t max) { // in src/node_internals.h because of a circular dependency. #if defined(NODE_WANT_INTERNALS) v8::MaybeLocal New(Environment* env, size_t size); +// Makes a copy of |data|. v8::MaybeLocal New(Environment* env, const char* data, size_t len); +// Takes ownership of |data|. v8::MaybeLocal New(Environment* env, char* data, size_t length, FreeCallback callback, 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 Use(Environment* env, char* data, size_t length); #endif // defined(NODE_WANT_INTERNALS) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index b5e83f9fa16496..d1bb677305d13d 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4479,7 +4479,7 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo& args) { return env->ThrowError("Failed to compute ECDH key"); } - Local buf = Buffer::New(env, out, out_len).ToLocalChecked(); + Local buf = Buffer::Use(env, out, out_len).ToLocalChecked(); args.GetReturnValue().Set(buf); } @@ -4544,7 +4544,7 @@ void ECDH::GetPrivateKey(const FunctionCallbackInfo& args) { } Local buf = - Buffer::New(env, reinterpret_cast(out), size).ToLocalChecked(); + Buffer::Use(env, reinterpret_cast(out), size).ToLocalChecked(); args.GetReturnValue().Set(buf); } @@ -4947,7 +4947,7 @@ void RandomBytesCheck(RandomBytesRequest* req, Local argv[2]) { size_t size; req->return_memory(&data, &size); argv[0] = Null(req->env()->isolate()); - argv[1] = Buffer::New(req->env(), data, size).ToLocalChecked(); + argv[1] = Buffer::Use(req->env(), data, size).ToLocalChecked(); } } diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index ae941f5edbd90b..3097eac4859761 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -223,7 +223,7 @@ void StreamWrap::OnReadImpl(ssize_t nread, CHECK_EQ(pending, UV_UNKNOWN_HANDLE); } - Local obj = Buffer::New(env, base, nread).ToLocalChecked(); + Local obj = Buffer::Use(env, base, nread).ToLocalChecked(); wrap->EmitData(nread, obj, pending_obj); } diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index dd3958ec0e37da..791ef76848cf43 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -408,7 +408,7 @@ void UDPWrap::OnRecv(uv_udp_t* handle, } char* base = static_cast(realloc(buf->base, nread)); - argv[2] = Buffer::New(env, base, nread).ToLocalChecked(); + argv[2] = Buffer::Use(env, base, nread).ToLocalChecked(); argv[3] = AddressToJS(env, addr); wrap->MakeCallback(env->onmessage_string(), ARRAY_SIZE(argv), argv); } From b86211a8b62c8f8ba7eed280ce7dcdf7d113cbe9 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 13 Aug 2015 13:29:28 +0200 Subject: [PATCH 2/4] src: move internal functions out of node_buffer.h The circular dependency problem that put them there in the first place is no longer an issue. Move them out of the public node_buffer.h header and into the private node_internals.h header. Fixes: https://github.com/nodejs/node/issues/2308 PR-URL: https://github.com/nodejs/node/pull/2352 Reviewed-By: Fedor Indutny Reviewed-By: Trevor Norris --- src/node_buffer.h | 22 ---------------------- src/node_internals.h | 16 ++++++++++++++++ 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/src/node_buffer.h b/src/node_buffer.h index f7c88f60e03dea..49fb5741640060 100644 --- a/src/node_buffer.h +++ b/src/node_buffer.h @@ -4,10 +4,6 @@ #include "node.h" #include "v8.h" -#if defined(NODE_WANT_INTERNALS) -#include "env.h" -#endif // defined(NODE_WANT_INTERNALS) - namespace node { namespace Buffer { @@ -63,24 +59,6 @@ static inline bool IsWithinBounds(size_t off, size_t len, size_t max) { return true; } -// Internal. Not for public consumption. We can't define these -// in src/node_internals.h because of a circular dependency. -#if defined(NODE_WANT_INTERNALS) -v8::MaybeLocal New(Environment* env, size_t size); -// Makes a copy of |data|. -v8::MaybeLocal New(Environment* env, const char* data, size_t len); -// Takes ownership of |data|. -v8::MaybeLocal New(Environment* env, - char* data, - size_t length, - FreeCallback callback, - 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 Use(Environment* env, char* data, size_t length); -#endif // defined(NODE_WANT_INTERNALS) - } // namespace Buffer } // namespace node diff --git a/src/node_internals.h b/src/node_internals.h index c99b2feeb0bdcd..aa4474becc1317 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -272,6 +272,22 @@ class NodeInstanceData { DISALLOW_COPY_AND_ASSIGN(NodeInstanceData); }; +namespace Buffer { +v8::MaybeLocal New(Environment* env, size_t size); +// Makes a copy of |data|. +v8::MaybeLocal New(Environment* env, const char* data, size_t len); +// Takes ownership of |data|. +v8::MaybeLocal New(Environment* env, + char* data, + 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 Use(Environment* env, char* data, size_t length); +} // namespace Buffer + } // namespace node #endif // SRC_NODE_INTERNALS_H_ From 9a3929dcbfd835721b514a781cdca1b7fb911cf0 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 13 Aug 2015 13:47:18 +0200 Subject: [PATCH 3/4] src: introduce internal Buffer::Copy() function Rename the three argument overload of Buffer::New() to Buffer::Copy() and update the code base accordingly. The reason for renaming is to make it impossible to miss a call site. This coincidentally plugs a small memory leak in crypto.getAuthTag(). Fixes: https://github.com/nodejs/node/issues/2308 PR-URL: https://github.com/nodejs/node/pull/2352 Reviewed-By: Fedor Indutny Reviewed-By: Trevor Norris --- src/js_stream.cc | 2 +- src/node_buffer.cc | 6 +++--- src/node_crypto.cc | 34 ++++++++++++++++++---------------- src/node_internals.h | 3 +-- src/tls_wrap.cc | 2 +- 5 files changed, 24 insertions(+), 23 deletions(-) diff --git a/src/js_stream.cc b/src/js_stream.cc index aa8de3a9ad9b8f..16fabc9df3449f 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -90,7 +90,7 @@ int JSStream::DoWrite(WriteWrap* w, Local bufs_arr = Array::New(env()->isolate(), count); Local buf; for (size_t i = 0; i < count; i++) { - buf = Buffer::New(env(), bufs[i].base, bufs[i].len).ToLocalChecked(); + buf = Buffer::Copy(env(), bufs[i].base, bufs[i].len).ToLocalChecked(); bufs_arr->Set(i, buf); } diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 5ab599ebab93ec..5f68822fc833bc 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -281,13 +281,13 @@ MaybeLocal Copy(Isolate* isolate, const char* data, size_t length) { Environment* env = Environment::GetCurrent(isolate); EscapableHandleScope handle_scope(env->isolate()); Local obj; - if (Buffer::New(env, data, length).ToLocal(&obj)) + if (Buffer::Copy(env, data, length).ToLocal(&obj)) return handle_scope.Escape(obj); return Local(); } -MaybeLocal New(Environment* env, const char* data, size_t length) { +MaybeLocal Copy(Environment* env, const char* data, size_t length) { EscapableHandleScope scope(env->isolate()); // V8 currently only allows a maximum Typed Array index of max Smi. @@ -365,7 +365,7 @@ MaybeLocal New(Isolate* isolate, char* data, size_t length) { Environment* env = Environment::GetCurrent(isolate); EscapableHandleScope handle_scope(env->isolate()); Local obj; - if (Buffer::New(env, data, length).ToLocal(&obj)) + if (Buffer::Use(env, data, length).ToLocal(&obj)) return handle_scope.Escape(obj); return Local(); } diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d1bb677305d13d..118c6b91b19b75 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1021,12 +1021,12 @@ int SecureContext::TicketKeyCallback(SSL* ssl, Context::Scope context_scope(env->context()); Local argv[] = { - Buffer::New(env, - reinterpret_cast(name), - kTicketPartSize).ToLocalChecked(), - Buffer::New(env, - reinterpret_cast(iv), - kTicketPartSize).ToLocalChecked(), + Buffer::Copy(env, + reinterpret_cast(name), + kTicketPartSize).ToLocalChecked(), + Buffer::Copy(env, + reinterpret_cast(iv), + kTicketPartSize).ToLocalChecked(), Boolean::New(env->isolate(), enc != 0) }; Local ret = node::MakeCallback(env, @@ -1219,7 +1219,7 @@ int SSLWrap::NewSessionCallback(SSL* s, SSL_SESSION* sess) { memset(serialized, 0, size); i2d_SSL_SESSION(sess, &serialized); - Local session = Buffer::New( + Local session = Buffer::Copy( env, reinterpret_cast(sess->session_id), sess->session_id_length).ToLocalChecked(); @@ -1240,7 +1240,7 @@ void SSLWrap::OnClientHello(void* arg, Context::Scope context_scope(env->context()); Local hello_obj = Object::New(env->isolate()); - Local buff = Buffer::New( + Local buff = Buffer::Copy( env, reinterpret_cast(hello.session_id()), hello.session_size()).ToLocalChecked(); @@ -1703,7 +1703,7 @@ void SSLWrap::GetTLSTicket(const FunctionCallbackInfo& args) { if (sess == nullptr || sess->tlsext_tick == nullptr) return; - Local buff = Buffer::New( + Local buff = Buffer::Copy( env, reinterpret_cast(sess->tlsext_tick), sess->tlsext_ticklen).ToLocalChecked(); @@ -1985,7 +1985,7 @@ int SSLWrap::TLSExtStatusCallback(SSL* s, void* arg) { if (resp == nullptr) { arg = Null(env->isolate()); } else { - arg = Buffer::New( + arg = Buffer::Copy( env, reinterpret_cast(const_cast(resp)), len).ToLocalChecked(); @@ -2991,7 +2991,8 @@ bool CipherBase::GetAuthTag(char** out, unsigned int* out_len) const { if (initialised_ || kind_ != kCipher || !auth_tag_) return false; *out_len = auth_tag_len_; - *out = new char[auth_tag_len_]; + *out = static_cast(malloc(auth_tag_len_)); + CHECK_NE(*out, nullptr); memcpy(*out, auth_tag_, auth_tag_len_); return true; } @@ -3005,7 +3006,7 @@ void CipherBase::GetAuthTag(const FunctionCallbackInfo& args) { unsigned int out_len = 0; if (cipher->GetAuthTag(&out, &out_len)) { - Local buf = Buffer::New(env, out, out_len).ToLocalChecked(); + Local buf = Buffer::Use(env, out, out_len).ToLocalChecked(); args.GetReturnValue().Set(buf); } else { env->ThrowError("Attempting to get auth tag in unsupported state"); @@ -3122,8 +3123,9 @@ void CipherBase::Update(const FunctionCallbackInfo& args) { "Trying to add data in unsupported state"); } + CHECK(out != nullptr || out_len == 0); Local buf = - Buffer::New(env, reinterpret_cast(out), out_len).ToLocalChecked(); + Buffer::Copy(env, reinterpret_cast(out), out_len).ToLocalChecked(); if (out) delete[] out; @@ -3198,7 +3200,7 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { } } - Local buf = Buffer::New( + Local buf = Buffer::Copy( env, reinterpret_cast(out_value), out_len).ToLocalChecked(); @@ -3980,7 +3982,7 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo& args) { } } - Local vbuf = Buffer::New( + Local vbuf = Buffer::Copy( env, reinterpret_cast(out_value), out_len).ToLocalChecked(); @@ -4517,7 +4519,7 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo& args) { } Local buf = - Buffer::New(env, reinterpret_cast(out), size).ToLocalChecked(); + Buffer::Use(env, reinterpret_cast(out), size).ToLocalChecked(); args.GetReturnValue().Set(buf); } diff --git a/src/node_internals.h b/src/node_internals.h index aa4474becc1317..f9068b650fb680 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -273,9 +273,8 @@ class NodeInstanceData { }; namespace Buffer { +v8::MaybeLocal Copy(Environment* env, const char* data, size_t len); v8::MaybeLocal New(Environment* env, size_t size); -// Makes a copy of |data|. -v8::MaybeLocal New(Environment* env, const char* data, size_t len); // Takes ownership of |data|. v8::MaybeLocal New(Environment* env, char* data, diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index fc19a5ce0bbe38..fbb35fdb5e6173 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -660,7 +660,7 @@ void TLSWrap::OnReadSelf(ssize_t nread, TLSWrap* wrap = static_cast(ctx); Local buf_obj; if (buf != nullptr) - buf_obj = Buffer::New(wrap->env(), buf->base, buf->len).ToLocalChecked(); + buf_obj = Buffer::Use(wrap->env(), buf->base, buf->len).ToLocalChecked(); wrap->EmitData(nread, buf_obj, Local()); } From 88419479ccb6384473339f09e5c55f234f4ee194 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 13 Aug 2015 13:53:39 +0200 Subject: [PATCH 4/4] src: rename Buffer::Use() to Buffer::New() Fixes: https://github.com/nodejs/node/issues/2308 PR-URL: https://github.com/nodejs/node/pull/2352 Reviewed-By: Fedor Indutny Reviewed-By: Trevor Norris --- src/node_buffer.cc | 4 ++-- src/node_crypto.cc | 10 +++++----- src/node_internals.h | 2 +- src/stream_wrap.cc | 2 +- src/tls_wrap.cc | 2 +- src/udp_wrap.cc | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 5f68822fc833bc..76a733311b0eb0 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -365,13 +365,13 @@ MaybeLocal New(Isolate* isolate, char* data, size_t length) { Environment* env = Environment::GetCurrent(isolate); EscapableHandleScope handle_scope(env->isolate()); Local obj; - if (Buffer::Use(env, data, length).ToLocal(&obj)) + if (Buffer::New(env, data, length).ToLocal(&obj)) return handle_scope.Escape(obj); return Local(); } -MaybeLocal Use(Environment* env, char* data, size_t length) { +MaybeLocal New(Environment* env, char* data, size_t length) { EscapableHandleScope scope(env->isolate()); if (length > 0) { diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 118c6b91b19b75..aa0b60ea898a52 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3006,7 +3006,7 @@ void CipherBase::GetAuthTag(const FunctionCallbackInfo& args) { unsigned int out_len = 0; if (cipher->GetAuthTag(&out, &out_len)) { - Local buf = Buffer::Use(env, out, out_len).ToLocalChecked(); + Local buf = Buffer::New(env, out, out_len).ToLocalChecked(); args.GetReturnValue().Set(buf); } else { env->ThrowError("Attempting to get auth tag in unsupported state"); @@ -4481,7 +4481,7 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo& args) { return env->ThrowError("Failed to compute ECDH key"); } - Local buf = Buffer::Use(env, out, out_len).ToLocalChecked(); + Local buf = Buffer::New(env, out, out_len).ToLocalChecked(); args.GetReturnValue().Set(buf); } @@ -4519,7 +4519,7 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo& args) { } Local buf = - Buffer::Use(env, reinterpret_cast(out), size).ToLocalChecked(); + Buffer::New(env, reinterpret_cast(out), size).ToLocalChecked(); args.GetReturnValue().Set(buf); } @@ -4546,7 +4546,7 @@ void ECDH::GetPrivateKey(const FunctionCallbackInfo& args) { } Local buf = - Buffer::Use(env, reinterpret_cast(out), size).ToLocalChecked(); + Buffer::New(env, reinterpret_cast(out), size).ToLocalChecked(); args.GetReturnValue().Set(buf); } @@ -4949,7 +4949,7 @@ void RandomBytesCheck(RandomBytesRequest* req, Local argv[2]) { size_t size; req->return_memory(&data, &size); argv[0] = Null(req->env()->isolate()); - argv[1] = Buffer::Use(req->env(), data, size).ToLocalChecked(); + argv[1] = Buffer::New(req->env(), data, size).ToLocalChecked(); } } diff --git a/src/node_internals.h b/src/node_internals.h index f9068b650fb680..c68b7155b0cdaf 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -284,7 +284,7 @@ v8::MaybeLocal New(Environment* env, // 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 Use(Environment* env, char* data, size_t length); +v8::MaybeLocal New(Environment* env, char* data, size_t length); } // namespace Buffer } // namespace node diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 3097eac4859761..ae941f5edbd90b 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -223,7 +223,7 @@ void StreamWrap::OnReadImpl(ssize_t nread, CHECK_EQ(pending, UV_UNKNOWN_HANDLE); } - Local obj = Buffer::Use(env, base, nread).ToLocalChecked(); + Local obj = Buffer::New(env, base, nread).ToLocalChecked(); wrap->EmitData(nread, obj, pending_obj); } diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index fbb35fdb5e6173..fc19a5ce0bbe38 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -660,7 +660,7 @@ void TLSWrap::OnReadSelf(ssize_t nread, TLSWrap* wrap = static_cast(ctx); Local buf_obj; if (buf != nullptr) - buf_obj = Buffer::Use(wrap->env(), buf->base, buf->len).ToLocalChecked(); + buf_obj = Buffer::New(wrap->env(), buf->base, buf->len).ToLocalChecked(); wrap->EmitData(nread, buf_obj, Local()); } diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 791ef76848cf43..dd3958ec0e37da 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -408,7 +408,7 @@ void UDPWrap::OnRecv(uv_udp_t* handle, } char* base = static_cast(realloc(buf->base, nread)); - argv[2] = Buffer::Use(env, base, nread).ToLocalChecked(); + argv[2] = Buffer::New(env, base, nread).ToLocalChecked(); argv[3] = AddressToJS(env, addr); wrap->MakeCallback(env->onmessage_string(), ARRAY_SIZE(argv), argv); }