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 <fedor@indutny.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

diff --git a/src/js_stream.cc b/src/js_stream.cc
index aa8de3a..16fabc9 100644 (file)
--- a/src/js_stream.cc
+++ b/src/js_stream.cc
@@ -90,7 +90,7 @@ int JSStream::DoWrite(WriteWrap* w,
   Local<Array> bufs_arr = Array::New(env()->isolate(), count);
   Local<Object> 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 5ab599e..5f68822 100644 (file)
--- a/src/node_buffer.cc
+++ b/src/node_buffer.cc
@@ -281,13 +281,13 @@ MaybeLocal<Object> Copy(Isolate* isolate, const char* data, size_t length) {
   Environment* env = Environment::GetCurrent(isolate);
   EscapableHandleScope handle_scope(env->isolate());
   Local<Object> obj;
-  if (Buffer::New(env, data, length).ToLocal(&obj))
+  if (Buffer::Copy(env, data, length).ToLocal(&obj))
     return handle_scope.Escape(obj);
   return Local<Object>();
 }
 
 
-MaybeLocal<Object> New(Environment* env, const char* data, size_t length) {
+MaybeLocal<Object> 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<Object> New(Isolate* isolate, char* data, size_t length) {
   Environment* env = Environment::GetCurrent(isolate);
   EscapableHandleScope handle_scope(env->isolate());
   Local<Object> obj;
-  if (Buffer::New(env, data, length).ToLocal(&obj))
+  if (Buffer::Use(env, data, length).ToLocal(&obj))
     return handle_scope.Escape(obj);
   return Local<Object>();
 }

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index d1bb677..118c6b9 100644 (file)
--- 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<Value> argv[] = {
-    Buffer::New(env,
-                reinterpret_cast<char*>(name),
-                kTicketPartSize).ToLocalChecked(),
-    Buffer::New(env,
-                reinterpret_cast<char*>(iv),
-                kTicketPartSize).ToLocalChecked(),
+    Buffer::Copy(env,
+                 reinterpret_cast<char*>(name),
+                 kTicketPartSize).ToLocalChecked(),
+    Buffer::Copy(env,
+                 reinterpret_cast<char*>(iv),
+                 kTicketPartSize).ToLocalChecked(),
     Boolean::New(env->isolate(), enc != 0)
   };
   Local<Value> ret = node::MakeCallback(env,
@@ -1219,7 +1219,7 @@ int SSLWrap<Base>::NewSessionCallback(SSL* s, SSL_SESSION* sess) {
   memset(serialized, 0, size);
   i2d_SSL_SESSION(sess, &serialized);
 
-  Local<Object> session = Buffer::New(
+  Local<Object> session = Buffer::Copy(
       env,
       reinterpret_cast<char*>(sess->session_id),
       sess->session_id_length).ToLocalChecked();
@@ -1240,7 +1240,7 @@ void SSLWrap<Base>::OnClientHello(void* arg,
   Context::Scope context_scope(env->context());
 
   Local<Object> hello_obj = Object::New(env->isolate());
-  Local<Object> buff = Buffer::New(
+  Local<Object> buff = Buffer::Copy(
       env,
       reinterpret_cast<const char*>(hello.session_id()),
       hello.session_size()).ToLocalChecked();
@@ -1703,7 +1703,7 @@ void SSLWrap<Base>::GetTLSTicket(const FunctionCallbackInfo<Value>& args) {
   if (sess == nullptr || sess->tlsext_tick == nullptr)
     return;
 
-  Local<Object> buff = Buffer::New(
+  Local<Object> buff = Buffer::Copy(
       env,
       reinterpret_cast<char*>(sess->tlsext_tick),
       sess->tlsext_ticklen).ToLocalChecked();
@@ -1985,7 +1985,7 @@ int SSLWrap<Base>::TLSExtStatusCallback(SSL* s, void* arg) {
     if (resp == nullptr) {
       arg = Null(env->isolate());
     } else {
-      arg = Buffer::New(
+      arg = Buffer::Copy(
           env,
           reinterpret_cast<char*>(const_cast<unsigned char*>(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<char*>(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<Value>& args) {
   unsigned int out_len = 0;
 
   if (cipher->GetAuthTag(&out, &out_len)) {
-    Local<Object> buf = Buffer::New(env, out, out_len).ToLocalChecked();
+    Local<Object> 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<Value>& args) {
                             "Trying to add data in unsupported state");
   }
 
+  CHECK(out != nullptr || out_len == 0);
   Local<Object> buf =
-      Buffer::New(env, reinterpret_cast<char*>(out), out_len).ToLocalChecked();
+      Buffer::Copy(env, reinterpret_cast<char*>(out), out_len).ToLocalChecked();
   if (out)
     delete[] out;
 
@@ -3198,7 +3200,7 @@ void CipherBase::Final(const FunctionCallbackInfo<Value>& args) {
     }
   }
 
-  Local<Object> buf = Buffer::New(
+  Local<Object> buf = Buffer::Copy(
       env,
       reinterpret_cast<char*>(out_value),
       out_len).ToLocalChecked();
@@ -3980,7 +3982,7 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo<Value>& args) {
     }
   }
 
-  Local<Object> vbuf = Buffer::New(
+  Local<Object> vbuf = Buffer::Copy(
       env,
       reinterpret_cast<char*>(out_value),
       out_len).ToLocalChecked();
@@ -4517,7 +4519,7 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo<Value>& args) {
   }
 
   Local<Object> buf =
-      Buffer::New(env, reinterpret_cast<char*>(out), size).ToLocalChecked();
+      Buffer::Use(env, reinterpret_cast<char*>(out), size).ToLocalChecked();
   args.GetReturnValue().Set(buf);
 }
 

diff --git a/src/node_internals.h b/src/node_internals.h
index aa4474b..f9068b6 100644 (file)
--- a/src/node_internals.h
+++ b/src/node_internals.h
@@ -273,9 +273,8 @@ class NodeInstanceData {
 };
 
 namespace Buffer {
+v8::MaybeLocal<v8::Object> Copy(Environment* env, const char* data, size_t len);
 v8::MaybeLocal<v8::Object> New(Environment* env, size_t size);
-// Makes a copy of |data|.
-v8::MaybeLocal<v8::Object> New(Environment* env, const char* data, size_t len);
 // Takes ownership of |data|.
 v8::MaybeLocal<v8::Object> New(Environment* env,
                                char* data,

diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index fc19a5c..fbb35fd 100644 (file)
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -660,7 +660,7 @@ void TLSWrap::OnReadSelf(ssize_t nread,
   TLSWrap* wrap = static_cast<TLSWrap*>(ctx);
   Local<Object> 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<Object>());
 }