From 5b67c335d1cf54fc2ff058c577f7f7d1b53c4a97 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.
---
 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 d4f7c751634b4d..99a2f28dfb4a2d 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);
@@ -283,14 +285,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;
   }
@@ -300,14 +305,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>();
 }
 
 
@@ -327,6 +328,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>();
   }
 
@@ -334,8 +336,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;
@@ -346,14 +350,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>();
 }
 
 
@@ -380,6 +380,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>();
   }
 
@@ -391,11 +393,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());
 }