From fd63e1ce2b5883cb7a6425770b98435cb99df825 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 13 Aug 2015 13:47:18 +0200 Subject: [PATCH] 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 4e649e10688d41..fda1fad996d61e 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(); @@ -1701,7 +1701,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(); @@ -1983,7 +1983,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(); @@ -2989,7 +2989,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; } @@ -3003,7 +3004,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"); @@ -3120,8 +3121,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; @@ -3196,7 +3198,7 @@ void CipherBase::Final(const FunctionCallbackInfo& args) { } } - Local buf = Buffer::New( + Local buf = Buffer::Copy( env, reinterpret_cast(out_value), out_len).ToLocalChecked(); @@ -3978,7 +3980,7 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo& args) { } } - Local vbuf = Buffer::New( + Local vbuf = Buffer::Copy( env, reinterpret_cast(out_value), out_len).ToLocalChecked(); @@ -4515,7 +4517,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()); }