buffer: throw when filling with empty buffers

Prior to this commit, Node would enter an infinite loop when attempting to fill a non-zero length buffer with a zero length buffer. This commit introduces a thrown exception in this scenario. PR-URL: #18129 Fixes: #18128 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
nodejs · Jan 17, 2018 · 1e80253 · 1e80253
1 parent db9c556
commit 1e80253
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 7 deletions.
diff --git a/doc/api/buffer.md b/doc/api/buffer.md
@@ -519,6 +519,10 @@ changes:
     pr-url: https://github.com/nodejs/node/pull/17427
     description: Specifying an invalid string for `fill` triggers a thrown
                  exception.
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/18129
+    description: Attempting to fill a non-zero length buffer with a zero length
+                 buffer triggers a thrown exception.
 -->
 
 * `size` {integer} The desired length of the new `Buffer`.
@@ -1231,6 +1235,10 @@ changes:
     pr-url: https://github.com/nodejs/node/pull/17427
     description: Specifying an invalid string for `value` triggers a thrown
                  exception.
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/18129
+    description: Attempting to fill a non-zero length buffer with a zero length
+                 buffer triggers a thrown exception.
 -->
 
 * `value` {string|Buffer|integer} The value to fill `buf` with.

diff --git a/src/node_buffer.cc b/src/node_buffer.cc
@@ -642,20 +642,20 @@ void Fill(const FunctionCallbackInfo<Value>& args) {
                                     str_obj,
                                     enc,
                                     nullptr);
-    // This check is also needed in case Write() returns that no bytes could
-    // be written. If no bytes could be written, then return -1 because the
-    // string is invalid. This will trigger a throw in JavaScript. Silently
-    // failing should be avoided because it can lead to buffers with unexpected
-    // contents.
-    if (str_length == 0)
-      return args.GetReturnValue().Set(-1);
   }
 
  start_fill:
 
   if (str_length >= fill_length)
     return;
 
+  // If str_length is zero, then either an empty buffer was provided, or Write()
+  // indicated that no bytes could be written. If no bytes could be written,
+  // then return -1 because the fill value is invalid. This will trigger a throw
+  // in JavaScript. Silently failing should be avoided because it can lead to
+  // buffers with unexpected contents.
+  if (str_length == 0)
+    return args.GetReturnValue().Set(-1);
 
   size_t in_there = str_length;
   char* ptr = ts_obj_data + start + str_length;

diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js
@@ -1024,3 +1024,10 @@ common.expectsError(() => {
   code: 'ERR_INVALID_ARG_VALUE',
   type: TypeError
 });
+
+common.expectsError(() => {
+  Buffer.alloc(1, Buffer.alloc(0));
+}, {
+  code: 'ERR_INVALID_ARG_VALUE',
+  type: TypeError
+});