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: #2308 PR-URL: #2352 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
nodejs · Aug 17, 2015 · fd63e1c · fd63e1c
1 parent 5586cec
commit fd63e1c
Show file tree

Hide file tree

Showing 5 changed files with 24 additions and 23 deletions.
diff --git 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
@@ -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
@@ -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();
@@ -1701,7 +1701,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();
@@ -1983,7 +1983,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();
@@ -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<char*>(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<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");
@@ -3120,8 +3121,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;
 
@@ -3196,7 +3198,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();
@@ -3978,7 +3980,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();
@@ -4515,7 +4517,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
@@ -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
@@ -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>());
 }