From c6d29ccf5a1aa079d624b23854640225d6eadb55 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sun, 28 Oct 2018 16:22:44 +0100
Subject: [PATCH] buffer: do proper error propagation in addon methods

- Always fulfill the `MaybeLocal<>` contract by scheduling an
  exception when returning an empty value. This was previously
  inconsistent, with no way to know whether an exception was
  be scheduled or not in case of failure.
- Make sure that memory is released exactly once in case of
  failure. Previously, some exit conditions would have leaked
  memory or attempted to free it multiple times.

This should not really affect how `Buffer`s are created by
addons in practice, due to the low frequency with which
these errors would typically occur.

PR-URL: https://github.com/nodejs/node/pull/23939
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
---
 src/node_buffer.cc | 48 ++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/src/node_buffer.cc b/src/node_buffer.cc
index 2a2fb45e7abf15..59605ff50a8968 100644
--- a/src/node_buffer.cc
+++ b/src/node_buffer.cc
@@ -243,8 +243,10 @@ MaybeLocal<Object> New(Isolate* isolate,
   if (length > 0) {
     data = static_cast<char*>(BufferMalloc(length));
 
-    if (data == nullptr)
+    if (data == nullptr) {
+      THROW_ERR_MEMORY_ALLOCATION_FAILED(isolate);
       return Local<Object>();
+    }
 
     actual = StringBytes::Write(isolate, data, length, string, enc);
     CHECK(actual <= length);
@@ -286,14 +288,17 @@ MaybeLocal<Object> New(Environment* env, size_t length) {
 
   // V8 currently only allows a maximum Typed Array index of max Smi.
   if (length > kMaxLength) {
+    env->isolate()->ThrowException(ERR_BUFFER_TOO_LARGE(env->isolate()));
     return Local<Object>();
   }
 
   void* data;
   if (length > 0) {
     data = BufferMalloc(length);
-    if (data == nullptr)
+    if (data == nullptr) {
+      THROW_ERR_MEMORY_ALLOCATION_FAILED(env);
       return Local<Object>();
+    }
   } else {
     data = nullptr;
   }
@@ -303,14 +308,10 @@ MaybeLocal<Object> New(Environment* env, size_t length) {
         data,
         length,
         ArrayBufferCreationMode::kInternalized);
-  MaybeLocal<Uint8Array> 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<Uint8Array>()));
+  Local<Object> obj;
+  if (Buffer::New(env, ab, 0, length).ToLocal(&obj))
+    return scope.Escape(obj);
+  return Local<Object>();
 }
 
 
@@ -333,6 +334,7 @@ MaybeLocal<Object> Copy(Environment* env, const char* data, size_t length) {
 
   // V8 currently only allows a maximum Typed Array index of max Smi.
   if (length > kMaxLength) {
+    env->isolate()->ThrowException(ERR_BUFFER_TOO_LARGE(env->isolate()));
     return Local<Object>();
   }
 
@@ -340,8 +342,10 @@ MaybeLocal<Object> Copy(Environment* env, const char* data, size_t length) {
   if (length > 0) {
     CHECK_NOT_NULL(data);
     new_data = node::UncheckedMalloc(length);
-    if (new_data == nullptr)
+    if (new_data == nullptr) {
+      THROW_ERR_MEMORY_ALLOCATION_FAILED(env);
       return Local<Object>();
+    }
     memcpy(new_data, data, length);
   } else {
     new_data = nullptr;
@@ -352,14 +356,10 @@ MaybeLocal<Object> Copy(Environment* env, const char* data, size_t length) {
         new_data,
         length,
         ArrayBufferCreationMode::kInternalized);
-  MaybeLocal<Uint8Array> ui = Buffer::New(env, ab, 0, length);
-
-  if (ui.IsEmpty()) {
-    // Object failed to be created. Clean up resources.
-    free(new_data);
-  }
-
-  return scope.Escape(ui.FromMaybe(Local<Uint8Array>()));
+  Local<Object> obj;
+  if (Buffer::New(env, ab, 0, length).ToLocal(&obj))
+    return scope.Escape(obj);
+  return Local<Object>();
 }
 
 
@@ -390,6 +390,8 @@ MaybeLocal<Object> New(Environment* env,
   EscapableHandleScope scope(env->isolate());
 
   if (length > kMaxLength) {
+    env->isolate()->ThrowException(ERR_BUFFER_TOO_LARGE(env->isolate()));
+    callback(data, hint);
     return Local<Object>();
   }
 
@@ -401,11 +403,11 @@ MaybeLocal<Object> New(Environment* env,
     ab->Neuter();
   MaybeLocal<Uint8Array> ui = Buffer::New(env, ab, 0, length);
 
-  if (ui.IsEmpty()) {
-    return Local<Object>();
-  }
-
   CallbackInfo::New(env->isolate(), ab, callback, data, hint);
+
+  if (ui.IsEmpty())
+    return MaybeLocal<Object>();
+
   return scope.Escape(ui.ToLocalChecked());
 }