buffer: throw on failed fill attempts

If fill() attempts to write a string to a buffer, but fails
silently, then uninitialized memory could be leaked. This commit
causes fill() to throw if the string write operation fails.

Refs: #17423
PR-URL: #17427
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>

doc/api/buffer.md

@@ -515,6 +515,10 @@ changes:
     pr-url: https://github.com/nodejs/node/pull/17428
     description: Specifying an invalid string for `fill` now results in a
                  zero-filled buffer.
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/17427
+    description: Specifying an invalid string for `fill` triggers a thrown
+                 exception.
 -->
 
 * `size` {integer} The desired length of the new `Buffer`.
@@ -1225,6 +1229,10 @@ changes:
   - version: v5.7.0
     pr-url: https://github.com/nodejs/node/pull/4935
     description: The `encoding` parameter is supported now.
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/17427
+    description: Specifying an invalid string for `value` triggers a thrown
+                 exception.
 -->
 
 * `value` {string|Buffer|integer} The value to fill `buf` with.
@@ -1260,15 +1268,15 @@ console.log(Buffer.allocUnsafe(3).fill('\u0222'));
 ```
 
 If `value` contains invalid characters, it is truncated; if no valid
-fill data remains, no filling is performed:
+fill data remains, an exception is thrown:
 
 ```js
 const buf = Buffer.allocUnsafe(5);
 // Prints: <Buffer 61 61 61 61 61>
 console.log(buf.fill('a'));
 // Prints: <Buffer aa aa aa aa aa>
 console.log(buf.fill('aazz', 'hex'));
-// Prints: <Buffer aa aa aa aa aa>
+// Throws a exception.
 console.log(buf.fill('zz', 'hex'));
 ```
 

lib/buffer.js

@@ -892,8 +892,12 @@ function fill_(buf, val, start, end, encoding) {
 
   start = start >>> 0;
   end = end === undefined ? buf.length : end >>> 0;
+  const fillLength = bindingFill(buf, val, start, end, encoding);
 
-  return bindingFill(buf, val, start, end, encoding);
+  if (fillLength === -1)
+    throw new errors.TypeError('ERR_INVALID_ARG_VALUE', 'value', val);
+
+  return fillLength;
 }
 
 

src/node_buffer.cc

@@ -643,11 +643,12 @@ void Fill(const FunctionCallbackInfo<Value>& args) {
                                     enc,
                                     nullptr);
     // This check is also needed in case Write() returns that no bytes could
-    // be written.
-    // TODO(trevnorris): Should this throw? Because of the string length was
-    // greater than 0 but couldn't be written then the string was invalid.
+    // 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(0);
+      return args.GetReturnValue().Set(-1);
   }
 
  start_fill:

test/parallel/test-buffer-alloc.js

@@ -1006,13 +1006,16 @@ assert.strictEqual(Buffer.prototype.toLocaleString, Buffer.prototype.toString);
   assert.strictEqual(buf.toLocaleString(), buf.toString());
 }
 
-{
-  // Ref: https://github.com/nodejs/node/issues/17423
-  const buf = Buffer.alloc(0x1000, 'This is not correctly encoded', 'hex');
-  assert(buf.every((byte) => byte === 0), `Buffer was not zeroed out: ${buf}`);
-}
+common.expectsError(() => {
+  Buffer.alloc(0x1000, 'This is not correctly encoded', 'hex');
+}, {
+  code: 'ERR_INVALID_ARG_VALUE',
+  type: TypeError
+});
 
-{
-  const buf = Buffer.alloc(0x1000, 'c', 'hex');
-  assert(buf.every((byte) => byte === 0), `Buffer was not zeroed out: ${buf}`);
-}
+common.expectsError(() => {
+  Buffer.alloc(0x1000, 'c', 'hex');
+}, {
+  code: 'ERR_INVALID_ARG_VALUE',
+  type: TypeError
+});

test/parallel/test-buffer-fill.js

@@ -134,20 +134,23 @@ testBufs('61c8b462c8b563c8b6', 4, -1, 'hex');
 testBufs('61c8b462c8b563c8b6', 4, 1, 'hex');
 testBufs('61c8b462c8b563c8b6', 12, 1, 'hex');
 
-{
+common.expectsError(() => {
   const buf = Buffer.allocUnsafe(SIZE);
-  assert.doesNotThrow(() => {
-    // Make sure this operation doesn't go on forever.
-    buf.fill('yKJh', 'hex');
-  });
-}
 
-{
+  buf.fill('yKJh', 'hex');
+}, {
+  code: 'ERR_INVALID_ARG_VALUE',
+  type: TypeError
+});
+
+common.expectsError(() => {
   const buf = Buffer.allocUnsafe(SIZE);
-  assert.doesNotThrow(() => {
-    buf.fill('\u0222', 'hex');
-  });
-}
+
+  buf.fill('\u0222', 'hex');
+}, {
+  code: 'ERR_INVALID_ARG_VALUE',
+  type: TypeError
+});
 
 // BASE64
 testBufs('YWJj', 'ucs2');
@@ -469,8 +472,11 @@ assert.strictEqual(
   Buffer.allocUnsafeSlow(16).fill('Љ', 'utf8').toString('utf8'),
   'Љ'.repeat(8));
 
-{
+common.expectsError(() => {
   const buf = Buffer.from('a'.repeat(1000));
+
   buf.fill('This is not correctly encoded', 'hex');
-  assert.strictEqual(buf.toString(), 'a'.repeat(1000));
-}
+}, {
+  code: 'ERR_INVALID_ARG_VALUE',
+  type: TypeError
+});