From ebcba82a93317688ac51bf50015276c2aa344610 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Tue, 2 Apr 2019 00:01:29 +0200
Subject: [PATCH 1/2] buffer: use stricter range checks

This adds support to use offset and length arguments above uint32
and it validates the input to make sure the arguments do not overflow.
Before, if the input would overflow, it would cause the write to be
performt in the wrong spot / result in unexpected behavior.
Instead, just use a strict number validation.

Fixes: https://github.com/nodejs/node/issues/27043
---
 doc/api/errors.md                           | 12 ++--
 lib/buffer.js                               | 61 +++++++++++----------
 lib/internal/errors.js                      |  1 -
 test/parallel/test-buffer-alloc.js          | 11 ++--
 test/parallel/test-buffer-compare-offset.js | 37 +++++++++----
 test/parallel/test-buffer-fill.js           | 23 +-------
 6 files changed, 75 insertions(+), 70 deletions(-)

diff --git a/doc/api/errors.md b/doc/api/errors.md
index 2ad0bead8ec0bb..e93797d9d3ba62 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -1574,12 +1574,6 @@ OpenSSL crypto support.
 An attempt was made to use features that require [ICU][], but Node.js was not
 compiled with ICU support.
 
-<a id="ERR_NO_LONGER_SUPPORTED"></a>
-### ERR_NO_LONGER_SUPPORTED
-
-A Node.js API was called in an unsupported manner, such as
-`Buffer.write(string, encoding, offset[, length])`.
-
 <a id="ERR_OUT_OF_RANGE"></a>
 ### ERR_OUT_OF_RANGE
 
@@ -2095,6 +2089,12 @@ removed: v10.0.0
 
 Used by the `N-API` when `Constructor.prototype` is not an object.
 
+<a id="ERR_NO_LONGER_SUPPORTED"></a>
+### ERR_NO_LONGER_SUPPORTED
+
+A Node.js API was called in an unsupported manner, such as
+`Buffer.write(string, encoding, offset[, length])`.
+
 <a id="ERR_OUTOFMEMORY"></a>
 ### ERR_OUTOFMEMORY
 <!-- YAML
diff --git a/lib/buffer.js b/lib/buffer.js
index b5d89625b2bc69..2c66c701cb95ff 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -69,12 +69,14 @@ const {
     ERR_INVALID_ARG_VALUE,
     ERR_INVALID_BUFFER_SIZE,
     ERR_INVALID_OPT_VALUE,
-    ERR_NO_LONGER_SUPPORTED,
     ERR_UNKNOWN_ENCODING
   },
   hideStackFrames
 } = require('internal/errors');
-const { validateString } = require('internal/validators');
+const {
+  validateInteger,
+  validateString
+} = require('internal/validators');
 
 const {
   FastBuffer,
@@ -437,7 +439,7 @@ Buffer.concat = function concat(list, length) {
   if (list.length === 0)
     return new FastBuffer();
 
-  if (length === undefined) {
+  if (length == null) {
     length = 0;
     for (i = 0; i < list.length; i++) {
       if (list[i].length) {
@@ -445,7 +447,7 @@ Buffer.concat = function concat(list, length) {
       }
     }
   } else {
-    length = length >>> 0;
+    validateInteger(length, 'length');
   }
 
   const buffer = Buffer.allocUnsafe(length);
@@ -691,32 +693,32 @@ Buffer.prototype.compare = function compare(target,
   else if (targetStart < 0)
     throw new ERR_OUT_OF_RANGE('targetStart', '>= 0', targetStart);
   else
-    targetStart >>>= 0;
+    validateInteger(targetStart, 'targetStart');
 
   if (targetEnd === undefined)
     targetEnd = target.length;
   else if (targetEnd > target.length)
     throw new ERR_OUT_OF_RANGE('targetEnd', `<= ${target.length}`, targetEnd);
   else
-    targetEnd >>>= 0;
+    validateInteger(targetEnd, 'targetEnd');
 
   if (sourceStart === undefined)
     sourceStart = 0;
   else if (sourceStart < 0)
     throw new ERR_OUT_OF_RANGE('sourceStart', '>= 0', sourceStart);
   else
-    sourceStart >>>= 0;
+    validateInteger(sourceStart, 'sourceStart');
 
   if (sourceEnd === undefined)
     sourceEnd = this.length;
   else if (sourceEnd > this.length)
     throw new ERR_OUT_OF_RANGE('sourceEnd', `<= ${this.length}`, sourceEnd);
   else
-    sourceEnd >>>= 0;
+    validateInteger(sourceEnd, 'sourceEnd');
 
   if (sourceStart >= sourceEnd)
     return (targetStart >= targetEnd ? 0 : -1);
-  else if (targetStart >= targetEnd)
+  if (targetStart >= targetEnd)
     return 1;
 
   return compareOffset(this, target, targetStart, sourceStart, targetEnd,
@@ -861,11 +863,11 @@ function _fill(buf, value, offset, end, encoding) {
     if (end === undefined) {
       end = buf.length;
     } else {
+      validateInteger(end, 'end');
       if (end > buf.length || end < 0)
         throw new ERR_OUT_OF_RANGE('end', `>= 0 and <= ${buf.length}`, end);
-      end = end >>> 0;
     }
-    offset = offset >>> 0;
+    validateInteger(offset, 'offset');
     if (offset >= end)
       return buf;
   }
@@ -884,39 +886,40 @@ Buffer.prototype.write = function write(string, offset, length, encoding) {
   // Buffer#write(string);
   if (offset === undefined) {
     return this.utf8Write(string, 0, this.length);
-
+  }
   // Buffer#write(string, encoding)
-  } else if (length === undefined && typeof offset === 'string') {
+  if (length === undefined && typeof offset === 'string') {
     encoding = offset;
     length = this.length;
     offset = 0;
 
   // Buffer#write(string, offset[, length][, encoding])
-  } else if (isFinite(offset)) {
-    offset = offset >>> 0;
-    if (isFinite(length)) {
-      length = length >>> 0;
+  } else {
+    if (offset === undefined) {
+      offset = 0;
     } else {
-      encoding = length;
-      length = undefined;
+      validateInteger(offset, 'offset');
     }
 
     const remaining = this.length - offset;
-    if (length === undefined || length > remaining)
+
+    if (length === undefined) {
       length = remaining;
+    } else if (typeof length === 'string') {
+      encoding = length;
+      length = remaining;
+    } else {
+      validateInteger(length, 'length');
+      if (length > remaining)
+        length = remaining;
+    }
 
-    if (string.length > 0 && (length < 0 || offset < 0))
+    if (length < 0 || offset < 0)
       throw new ERR_BUFFER_OUT_OF_BOUNDS();
-  } else {
-    // If someone is still calling the obsolete form of write(), tell them.
-    // we don't want eg buf.write("foo", "utf8", 10) to silently turn into
-    // buf.write("foo", "utf8"), so we can't ignore extra args
-    throw new ERR_NO_LONGER_SUPPORTED(
-      'Buffer.write(string, encoding, offset[, length])'
-    );
   }
 
-  if (!encoding) return this.utf8Write(string, offset, length);
+  if (!encoding)
+    return this.utf8Write(string, offset, length);
 
   encoding += '';
   switch (encoding.length) {
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index c7dac93783fed3..ea62c24d608000 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -992,7 +992,6 @@ E('ERR_NO_CRYPTO',
   'Node.js is not compiled with OpenSSL crypto support', Error);
 E('ERR_NO_ICU',
   '%s is not supported on Node.js compiled without ICU', TypeError);
-E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported', Error);
 E('ERR_OUT_OF_RANGE',
   (str, range, input, replaceDefaultBoolean = false) => {
     assert(range, 'Missing "range" argument');
diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js
index 8369936b4d8eca..2804fcb6f2e13e 100644
--- a/test/parallel/test-buffer-alloc.js
+++ b/test/parallel/test-buffer-alloc.js
@@ -59,8 +59,7 @@ assert.throws(() => b.write('test string', 0, 5, 'invalid'),
               /Unknown encoding: invalid/);
 // Unsupported arguments for Buffer.write
 assert.throws(() => b.write('test', 'utf8', 0),
-              /is no longer supported/);
-
+              { code: 'ERR_INVALID_ARG_TYPE' });
 
 // Try to create 0-length buffers. Should not throw.
 Buffer.from('');
@@ -110,8 +109,12 @@ b.copy(Buffer.alloc(1), 0, 2048, 2048);
 {
   const writeTest = Buffer.from('abcdes');
   writeTest.write('n', 'ascii');
-  writeTest.write('o', '1', 'ascii');
-  writeTest.write('d', '2', 'ascii');
+  assert.throws(
+    () => writeTest.write('o', '1', 'ascii'),
+    { code: 'ERR_INVALID_ARG_TYPE' }
+  );
+  writeTest.write('o', 1, 'ascii');
+  writeTest.write('d', 2, 'ascii');
   writeTest.write('e', 3, 'ascii');
   writeTest.write('j', 4, 'ascii');
   assert.strictEqual(writeTest.toString(), 'nodejs');
diff --git a/test/parallel/test-buffer-compare-offset.js b/test/parallel/test-buffer-compare-offset.js
index 8c590f52ed480c..04d3195019f5be 100644
--- a/test/parallel/test-buffer-compare-offset.js
+++ b/test/parallel/test-buffer-compare-offset.js
@@ -10,7 +10,7 @@ assert.strictEqual(a.compare(b), -1);
 
 // Equivalent to a.compare(b).
 assert.strictEqual(a.compare(b, 0), -1);
-assert.strictEqual(a.compare(b, '0'), -1);
+assert.throws(() => a.compare(b, '0'), { code: 'ERR_INVALID_ARG_TYPE' });
 assert.strictEqual(a.compare(b, undefined), -1);
 
 // Equivalent to a.compare(b).
@@ -18,7 +18,10 @@ assert.strictEqual(a.compare(b, 0, undefined, 0), -1);
 
 // Zero-length target, return 1
 assert.strictEqual(a.compare(b, 0, 0, 0), 1);
-assert.strictEqual(a.compare(b, '0', '0', '0'), 1);
+assert.throws(
+  () => a.compare(b, 0, '0', '0'),
+  { code: 'ERR_INVALID_ARG_TYPE' }
+);
 
 // Equivalent to Buffer.compare(a, b.slice(6, 10))
 assert.strictEqual(a.compare(b, 6, 10), 1);
@@ -45,19 +48,33 @@ assert.strictEqual(a.compare(b, 0, 7, 4), -1);
 // Equivalent to Buffer.compare(a.slice(4, 6), b.slice(0, 7));
 assert.strictEqual(a.compare(b, 0, 7, 4, 6), -1);
 
-// zero length target
-assert.strictEqual(a.compare(b, 0, null), 1);
+// Null is ambiguous.
+assert.throws(
+  () => a.compare(b, 0, null),
+  { code: 'ERR_INVALID_ARG_TYPE' }
+);
 
-// Coerces to targetEnd == 5
-assert.strictEqual(a.compare(b, 0, { valueOf: () => 5 }), -1);
+// Values do not get coerced.
+assert.throws(
+  () => a.compare(b, 0, { valueOf: () => 5 }),
+  { code: 'ERR_INVALID_ARG_TYPE' }
+);
 
-// zero length target
-assert.strictEqual(a.compare(b, Infinity, -Infinity), 1);
+// Infinity should not be coerced.
+assert.throws(
+  () => a.compare(b, Infinity, -Infinity),
+  { code: 'ERR_OUT_OF_RANGE' }
+);
 
 // Zero length target because default for targetEnd <= targetSource
-assert.strictEqual(a.compare(b, '0xff'), 1);
+assert.strictEqual(a.compare(b, 0xff), 1);
 
-const oor = common.expectsError({ code: 'ERR_OUT_OF_RANGE' }, 7);
+assert.throws(
+  () => a.compare(b, '0xff'),
+  { code: 'ERR_INVALID_ARG_TYPE' }
+);
+
+const oor = { code: 'ERR_OUT_OF_RANGE' };
 
 assert.throws(() => a.compare(b, 0, 100, 0), oor);
 assert.throws(() => a.compare(b, 0, 1, 0, 100), oor);
diff --git a/test/parallel/test-buffer-fill.js b/test/parallel/test-buffer-fill.js
index 61ff6bbb501f4d..3dd11bce267a82 100644
--- a/test/parallel/test-buffer-fill.js
+++ b/test/parallel/test-buffer-fill.js
@@ -333,34 +333,17 @@ assert.strictEqual(
 // Make sure "end" is properly checked, even if it's magically mangled using
 // Symbol.toPrimitive.
 {
-  let elseWasLast = false;
-  const expectedErrorMessage =
-    'The value of "end" is out of range. It must be >= 0 and <= 1. Received -1';
-
   common.expectsError(() => {
-    let ctr = 0;
     const end = {
       [Symbol.toPrimitive]() {
-        // We use this condition to get around the check in lib/buffer.js
-        if (ctr === 0) {
-          elseWasLast = false;
-          ctr++;
-          return 1;
-        }
-        elseWasLast = true;
-        // Once buffer.js calls the C++ implementation of fill, return -1
-        return -1;
+        return 1;
       }
     };
     Buffer.alloc(1).fill(Buffer.alloc(1), 0, end);
   }, {
-    code: 'ERR_OUT_OF_RANGE',
-    type: RangeError,
-    message: expectedErrorMessage
+    code: 'ERR_INVALID_ARG_TYPE',
+    message: 'The "end" argument must be of type number. Received type object'
   });
-  // Make sure -1 is making it to Buffer::Fill().
-  assert.ok(elseWasLast,
-            'internal API changed, -1 no longer in correct location');
 }
 
 // Testing process.binding. Make sure "end" is properly checked for -1 wrap

From 3111f1bf24760260c30153ff039b9774737a8a36 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Tue, 2 Apr 2019 17:06:35 +0200
Subject: [PATCH 2/2] fixup! buffer: use stricter range checks

---
 lib/buffer.js                               | 38 ++++++---------------
 test/parallel/test-buffer-alloc.js          | 19 ++++++-----
 test/parallel/test-buffer-compare-offset.js |  5 ++-
 test/parallel/test-buffer-write.js          | 19 ++++++-----
 4 files changed, 36 insertions(+), 45 deletions(-)

diff --git a/lib/buffer.js b/lib/buffer.js
index 2c66c701cb95ff..3be73555bf798b 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -64,7 +64,6 @@ const {
 const {
   codes: {
     ERR_BUFFER_OUT_OF_BOUNDS,
-    ERR_OUT_OF_RANGE,
     ERR_INVALID_ARG_TYPE,
     ERR_INVALID_ARG_VALUE,
     ERR_INVALID_BUFFER_SIZE,
@@ -74,7 +73,7 @@ const {
   hideStackFrames
 } = require('internal/errors');
 const {
-  validateInteger,
+  validateInt32,
   validateString
 } = require('internal/validators');
 
@@ -439,7 +438,7 @@ Buffer.concat = function concat(list, length) {
   if (list.length === 0)
     return new FastBuffer();
 
-  if (length == null) {
+  if (length === undefined) {
     length = 0;
     for (i = 0; i < list.length; i++) {
       if (list[i].length) {
@@ -447,7 +446,7 @@ Buffer.concat = function concat(list, length) {
       }
     }
   } else {
-    validateInteger(length, 'length');
+    validateInt32(length, 'length', 0);
   }
 
   const buffer = Buffer.allocUnsafe(length);
@@ -690,31 +689,23 @@ Buffer.prototype.compare = function compare(target,
 
   if (targetStart === undefined)
     targetStart = 0;
-  else if (targetStart < 0)
-    throw new ERR_OUT_OF_RANGE('targetStart', '>= 0', targetStart);
   else
-    validateInteger(targetStart, 'targetStart');
+    validateInt32(targetStart, 'targetStart', 0);
 
   if (targetEnd === undefined)
     targetEnd = target.length;
-  else if (targetEnd > target.length)
-    throw new ERR_OUT_OF_RANGE('targetEnd', `<= ${target.length}`, targetEnd);
   else
-    validateInteger(targetEnd, 'targetEnd');
+    validateInt32(targetEnd, 'targetEnd', 0, target.length);
 
   if (sourceStart === undefined)
     sourceStart = 0;
-  else if (sourceStart < 0)
-    throw new ERR_OUT_OF_RANGE('sourceStart', '>= 0', sourceStart);
   else
-    validateInteger(sourceStart, 'sourceStart');
+    validateInt32(sourceStart, 'sourceStart', 0);
 
   if (sourceEnd === undefined)
     sourceEnd = this.length;
-  else if (sourceEnd > this.length)
-    throw new ERR_OUT_OF_RANGE('sourceEnd', `<= ${this.length}`, sourceEnd);
   else
-    validateInteger(sourceEnd, 'sourceEnd');
+    validateInt32(sourceEnd, 'sourceEnd', 0, this.length);
 
   if (sourceStart >= sourceEnd)
     return (targetStart >= targetEnd ? 0 : -1);
@@ -857,17 +848,13 @@ function _fill(buf, value, offset, end, encoding) {
     offset = 0;
     end = buf.length;
   } else {
+    validateInt32(offset, 'offset', 0);
     // Invalid ranges are not set to a default, so can range check early.
-    if (offset < 0)
-      throw new ERR_OUT_OF_RANGE('offset', '>= 0', offset);
     if (end === undefined) {
       end = buf.length;
     } else {
-      validateInteger(end, 'end');
-      if (end > buf.length || end < 0)
-        throw new ERR_OUT_OF_RANGE('end', `>= 0 and <= ${buf.length}`, end);
+      validateInt32(end, 'end', 0, buf.length);
     }
-    validateInteger(offset, 'offset');
     if (offset >= end)
       return buf;
   }
@@ -898,7 +885,7 @@ Buffer.prototype.write = function write(string, offset, length, encoding) {
     if (offset === undefined) {
       offset = 0;
     } else {
-      validateInteger(offset, 'offset');
+      validateInt32(offset, 'offset', 0, this.length);
     }
 
     const remaining = this.length - offset;
@@ -909,13 +896,10 @@ Buffer.prototype.write = function write(string, offset, length, encoding) {
       encoding = length;
       length = remaining;
     } else {
-      validateInteger(length, 'length');
+      validateInt32(length, 'length', 0, this.length);
       if (length > remaining)
         length = remaining;
     }
-
-    if (length < 0 || offset < 0)
-      throw new ERR_BUFFER_OUT_OF_BOUNDS();
   }
 
   if (!encoding)
diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js
index 2804fcb6f2e13e..75c2cae2b0ed83 100644
--- a/test/parallel/test-buffer-alloc.js
+++ b/test/parallel/test-buffer-alloc.js
@@ -5,6 +5,12 @@ const vm = require('vm');
 
 const SlowBuffer = require('buffer').SlowBuffer;
 
+// Verify the maximum Uint8Array size. There is no concrete limit by spec. The
+// internal limits should be updated if this fails.
+assert.throws(
+  () => new Uint8Array(2 ** 31),
+  { message: 'Invalid typed array length: 2147483648' }
+);
 
 const b = Buffer.allocUnsafe(1024);
 assert.strictEqual(b.length, 1024);
@@ -73,27 +79,22 @@ new Buffer('', 'latin1');
 new Buffer('', 'binary');
 Buffer(0);
 
-const outOfBoundsError = {
-  code: 'ERR_BUFFER_OUT_OF_BOUNDS',
-  type: RangeError
-};
-
 const outOfRangeError = {
   code: 'ERR_OUT_OF_RANGE',
   type: RangeError
 };
 
 // Try to write a 0-length string beyond the end of b
-common.expectsError(() => b.write('', 2048), outOfBoundsError);
+common.expectsError(() => b.write('', 2048), outOfRangeError);
 
 // Throw when writing to negative offset
-common.expectsError(() => b.write('a', -1), outOfBoundsError);
+common.expectsError(() => b.write('a', -1), outOfRangeError);
 
 // Throw when writing past bounds from the pool
-common.expectsError(() => b.write('a', 2048), outOfBoundsError);
+common.expectsError(() => b.write('a', 2048), outOfRangeError);
 
 // Throw when writing to negative offset
-common.expectsError(() => b.write('a', -1), outOfBoundsError);
+common.expectsError(() => b.write('a', -1), outOfRangeError);
 
 // Try to copy 0 bytes worth of data into an empty buffer
 b.copy(Buffer.alloc(0), 0, 0, 0);
diff --git a/test/parallel/test-buffer-compare-offset.js b/test/parallel/test-buffer-compare-offset.js
index 04d3195019f5be..3769e4d41ac27e 100644
--- a/test/parallel/test-buffer-compare-offset.js
+++ b/test/parallel/test-buffer-compare-offset.js
@@ -73,13 +73,16 @@ assert.throws(
   () => a.compare(b, '0xff'),
   { code: 'ERR_INVALID_ARG_TYPE' }
 );
+assert.throws(
+  () => a.compare(b, 0, '0xff'),
+  { code: 'ERR_INVALID_ARG_TYPE' }
+);
 
 const oor = { code: 'ERR_OUT_OF_RANGE' };
 
 assert.throws(() => a.compare(b, 0, 100, 0), oor);
 assert.throws(() => a.compare(b, 0, 1, 0, 100), oor);
 assert.throws(() => a.compare(b, -1), oor);
-assert.throws(() => a.compare(b, 0, '0xff'), oor);
 assert.throws(() => a.compare(b, 0, Infinity), oor);
 assert.throws(() => a.compare(b, 0, 1, -1), oor);
 assert.throws(() => a.compare(b, -Infinity, Infinity), oor);
diff --git a/test/parallel/test-buffer-write.js b/test/parallel/test-buffer-write.js
index a0b86844d4dbe0..e99cdb358a209e 100644
--- a/test/parallel/test-buffer-write.js
+++ b/test/parallel/test-buffer-write.js
@@ -3,14 +3,17 @@
 const common = require('../common');
 const assert = require('assert');
 
-const outsideBounds = common.expectsError({
-  code: 'ERR_BUFFER_OUT_OF_BOUNDS',
-  type: RangeError,
-  message: 'Attempt to write outside buffer bounds'
-}, 2);
-
-assert.throws(() => Buffer.alloc(9).write('foo', -1), outsideBounds);
-assert.throws(() => Buffer.alloc(9).write('foo', 10), outsideBounds);
+[-1, 10].forEach((offset) => {
+  assert.throws(
+    () => Buffer.alloc(9).write('foo', offset),
+    {
+      code: 'ERR_OUT_OF_RANGE',
+      name: 'RangeError',
+      message: 'The value of "offset" is out of range. ' +
+               `It must be >= 0 && <= 9. Received ${offset}`
+    }
+  );
+});
 
 const resultMap = new Map([
   ['utf8', Buffer.from([102, 111, 111, 0, 0, 0, 0, 0, 0])],