buffer: zero-fill buffer allocated with invalid content

addaleax · evanlucas · commit 15bf64066857 · 2017-12-07T12:50:09.000-06:00
Zero-fill when `Buffer.alloc()` receives invalid fill data. A solution like #17427 which switches to throwing makes sense, but is likely a breaking change. This suggestion leaves the behaviour of `buffer.fill()` untouched, since any change to it would be a breaking change, and lets `Buffer.alloc()` check whether any filling took place or not. PR-URL: #17428 Refs: #17427 Refs: #17423 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
diff --git a/doc/api/buffer.md b/doc/api/buffer.md
@@ -510,6 +510,11 @@ console.log(buf2.toString());
 ### Class Method: Buffer.alloc(size[, fill[, encoding]])
 <!-- YAML
 added: v5.10.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/17428
+    description: Specifying an invalid string for `fill` now results in a
+                 zero-filled buffer.
 -->
 
 * `size` {integer} The desired length of the new `Buffer`.
@@ -1253,7 +1258,7 @@ Example: Fill a `Buffer` with a two-byte character
 console.log(Buffer.allocUnsafe(3).fill('\u0222'));
 ```
 
-If `value` is contains invalid characters, it is truncated; if no valid
+If `value` contains invalid characters, it is truncated; if no valid
 fill data remains, no filling is performed:
 
 ```js
diff --git a/lib/buffer.js b/lib/buffer.js
@@ -27,7 +27,7 @@ const {
   compare: _compare,
   compareOffset,
   createFromString,
-  fill: _fill,
+  fill: bindingFill,
   indexOfBuffer,
   indexOfNumber,
   indexOfString,
@@ -266,7 +266,9 @@ Buffer.alloc = function alloc(size, fill, encoding) {
     // be interpreted as a start offset.
     if (typeof encoding !== 'string')
       encoding = undefined;
-    return createUnsafeBuffer(size).fill(fill, encoding);
+    const ret = createUnsafeBuffer(size);
+    if (fill_(ret, fill, encoding) > 0)
+      return ret;
   }
   return new FastBuffer(size);
 };
@@ -840,15 +842,20 @@ Buffer.prototype.includes = function includes(val, byteOffset, encoding) {
 //    buffer.fill(buffer[, offset[, end]])
 //    buffer.fill(string[, offset[, end]][, encoding])
 Buffer.prototype.fill = function fill(val, start, end, encoding) {
+  fill_(this, val, start, end, encoding);
+  return this;
+};
+
+function fill_(buf, val, start, end, encoding) {
   // Handle string cases:
   if (typeof val === 'string') {
     if (typeof start === 'string') {
       encoding = start;
       start = 0;
-      end = this.length;
+      end = buf.length;
     } else if (typeof end === 'string') {
       encoding = end;
-      end = this.length;
+      end = buf.length;
     }
 
     if (encoding !== undefined && typeof encoding !== 'string') {
@@ -877,19 +884,17 @@ Buffer.prototype.fill = function fill(val, start, end, encoding) {
   }
 
   // Invalid ranges are not set to a default, so can range check early.
-  if (start < 0 || end > this.length)
+  if (start < 0 || end > buf.length)
     throw new errors.RangeError('ERR_INDEX_OUT_OF_RANGE');
 
   if (end <= start)
-    return this;
+    return 0;
 
   start = start >>> 0;
-  end = end === undefined ? this.length : end >>> 0;
+  end = end === undefined ? buf.length : end >>> 0;
 
-  _fill(this, val, start, end, encoding);
-
-  return this;
-};
+  return bindingFill(buf, val, start, end, encoding);
+}
 
 
 Buffer.prototype.write = function write(string, offset, length, encoding) {
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
@@ -591,6 +591,8 @@ void Fill(const FunctionCallbackInfo<Value>& args) {
   THROW_AND_RETURN_IF_OOB(start <= end);
   THROW_AND_RETURN_IF_OOB(fill_length + start <= ts_obj_length);
 
+  args.GetReturnValue().Set(static_cast<double>(fill_length));
+
   // First check if Buffer has been passed.
   if (Buffer::HasInstance(args[1])) {
     SPREAD_BUFFER_ARG(args[1], fill_obj);
@@ -612,8 +614,10 @@ void Fill(const FunctionCallbackInfo<Value>& args) {
       enc == UTF8 ? str_obj->Utf8Length() :
       enc == UCS2 ? str_obj->Length() * sizeof(uint16_t) : str_obj->Length();
 
-  if (str_length == 0)
+  if (str_length == 0) {
+    args.GetReturnValue().Set(0);
     return;
+  }
 
   // Can't use StringBytes::Write() in all cases. For example if attempting
   // to write a two byte character into a one byte Buffer.
@@ -643,7 +647,7 @@ void Fill(const FunctionCallbackInfo<Value>& args) {
     // TODO(trevnorris): Should this throw? Because of the string length was
     // greater than 0 but couldn't be written then the string was invalid.
     if (str_length == 0)
-      return;
+      return args.GetReturnValue().Set(0);
   }
 
  start_fill:
diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js
@@ -1005,3 +1005,14 @@ assert.strictEqual(Buffer.prototype.toLocaleString, Buffer.prototype.toString);
   const buf = Buffer.from('test');
   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}`);
+}
+
+{
+  const buf = Buffer.alloc(0x1000, 'c', 'hex');
+  assert(buf.every((byte) => byte === 0), `Buffer was not zeroed out: ${buf}`);
+}
diff --git a/test/parallel/test-buffer-fill.js b/test/parallel/test-buffer-fill.js
@@ -468,3 +468,9 @@ assert.strictEqual(
 assert.strictEqual(
   Buffer.allocUnsafeSlow(16).fill('Љ', 'utf8').toString('utf8'),
   'Љ'.repeat(8));
+
+{
+  const buf = Buffer.from('a'.repeat(1000));
+  buf.fill('This is not correctly encoded', 'hex');
+  assert.strictEqual(buf.toString(), 'a'.repeat(1000));
+}
