buffer: use stricter range checks

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

lib/buffer.js

@@ -64,17 +64,18 @@ const {
 const {
   codes: {
     ERR_BUFFER_OUT_OF_BOUNDS,
-    ERR_OUT_OF_RANGE,
     ERR_INVALID_ARG_TYPE,
     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 {
+  validateInt32,
+  validateString
+} = require('internal/validators');
 
 const {
   FastBuffer,
@@ -445,7 +446,7 @@ Buffer.concat = function concat(list, length) {
       }
     }
   } else {
-    length = length >>> 0;
+    validateInt32(length, 'length', 0);
   }
 
   const buffer = Buffer.allocUnsafe(length);
@@ -688,35 +689,27 @@ 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
-    targetStart >>>= 0;
+    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
-    targetEnd >>>= 0;
+    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
-    sourceStart >>>= 0;
+    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
-    sourceEnd >>>= 0;
+    validateInt32(sourceEnd, 'sourceEnd', 0, this.length);
 
   if (sourceStart >= sourceEnd)
     return (targetStart >= targetEnd ? 0 : -1);
-  else if (targetStart >= targetEnd)
+  if (targetStart >= targetEnd)
     return 1;
 
   return compareOffset(this, target, targetStart, sourceStart, targetEnd,
@@ -855,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 {
-      if (end > buf.length || end < 0)
-        throw new ERR_OUT_OF_RANGE('end', `>= 0 and <= ${buf.length}`, end);
-      end = end >>> 0;
+      validateInt32(end, 'end', 0, buf.length);
     }
-    offset = offset >>> 0;
     if (offset >= end)
       return buf;
   }
@@ -884,39 +873,37 @@ 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;
+      validateInt32(offset, 'offset', 0, this.length);
     }
 
     const remaining = this.length - offset;
-    if (length === undefined || length > remaining)
-      length = remaining;
 
-    if (string.length > 0 && (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 (length === undefined) {
+      length = remaining;
+    } else if (typeof length === 'string') {
+      encoding = length;
+      length = remaining;
+    } else {
+      validateInt32(length, 'length', 0, this.length);
+      if (length > remaining)
+        length = remaining;
+    }
   }
 
-  if (!encoding) return this.utf8Write(string, offset, length);
+  if (!encoding)
+    return this.utf8Write(string, offset, length);
 
   encoding += '';
   switch (encoding.length) {

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');

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);
@@ -59,8 +65,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('');
@@ -74,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);
@@ -110,8 +110,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');

test/parallel/test-buffer-compare-offset.js

@@ -10,15 +10,18 @@ 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).
 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,24 +48,41 @@ 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' }
+);
+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);

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

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])],