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

@@ -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,15 +439,15 @@ 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) {
         length += list[i].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) {

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

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

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

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