zlib: improve zlib errors

- Use assert to check mode in the Zlib constructor since it should only be passed by us. - Introduce checkRangesOrGetDefault() and checkFiniteNumber() to simplify type and range checking for numeric arguments - Instead of `ERR_INVALID_OPT_VALUE`, throw `ERR_OUT_OF_RANGE` and `ERR_INVALID_ARG_TYPE` with descriptions of the expected ranges or types to make the errors more user-friendly. - Add message tests for the changed errors PR-URL: https://github.com/nodejs/node/pull/18675 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
2018-02-09 00:26:59 +08:00 · 2018-02-09 00:26:59 +08:00 · da886d9a4c
commit da886d9a4c
parent 15e41a9951
4 changed files with 317 additions and 130 deletions
--- a/lib/zlib.js
+++ b/lib/zlib.js
@ -156,6 +156,52 @@ function flushCallback(level, strategy, callback) {
  }
 }

+// 1. Returns false for undefined and NaN
+// 2. Returns true for finite numbers
+// 3. Throws ERR_INVALID_ARG_TYPE for non-numbers
+// 4. Throws ERR_OUT_OF_RANGE for infinite numbers
+function checkFiniteNumber(number, name) {
+  // Common case
+  if (number === undefined || Number.isNaN(number)) {
+    return false;
+  }
+
+  if (Number.isFinite(number)) {
+    return true;  // is a valid number
+  }
+
+  // Other non-numbers
+  if (typeof number !== 'number') {
+    const err = new errors.TypeError('ERR_INVALID_ARG_TYPE', name,
+                                     'number', number);
+    Error.captureStackTrace(err, checkFiniteNumber);
+    throw err;
+  }
+
+  // Infinite numbers
+  const err = new errors.RangeError('ERR_OUT_OF_RANGE', name,
+                                    'a finite number', number);
+  Error.captureStackTrace(err, checkFiniteNumber);
+  throw err;
+}
+
+// 1. Returns def for undefined and NaN
+// 2. Returns number for finite numbers >= lower and <= upper
+// 3. Throws ERR_INVALID_ARG_TYPE for non-numbers
+// 4. Throws ERR_OUT_OF_RANGE for infinite numbers or numbers > upper or < lower
+function checkRangesOrGetDefault(number, name, lower, upper, def) {
+  if (!checkFiniteNumber(number, name, lower, upper)) {
+    return def;
+  }
+  if (number < lower || number > upper) {
+    const err = new errors.RangeError('ERR_OUT_OF_RANGE', name,
+                                      `>= ${lower} and <= ${upper}`, number);
+    Error.captureStackTrace(err, checkRangesOrGetDefault);
+    throw err;
+  }
+  return number;
+}
+
 // the Zlib class they all inherit from
 // This thing manages the queue of requests, and returns
 // true or false if there is anything in the queue when
@ -170,94 +216,52 @@ function Zlib(opts, mode) {
  var strategy = Z_DEFAULT_STRATEGY;
  var dictionary;

-  if (typeof mode !== 'number')
-    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'mode', 'number');
-  if (mode < DEFLATE || mode > UNZIP)
-    throw new errors.RangeError('ERR_OUT_OF_RANGE', 'mode');
+  // The Zlib class is not exported to user land, the mode should only be
+  // passed in by us.
+  assert(typeof mode === 'number');
+  assert(mode >= DEFLATE && mode <= UNZIP);

  if (opts) {
    chunkSize = opts.chunkSize;
-    if (chunkSize !== undefined && !Number.isNaN(chunkSize)) {
-      if (chunkSize < Z_MIN_CHUNK || !Number.isFinite(chunkSize))
-        throw new errors.RangeError('ERR_INVALID_OPT_VALUE',
-                                    'chunkSize',
-                                    chunkSize);
-    } else {
+    if (!checkFiniteNumber(chunkSize, 'options.chunkSize')) {
      chunkSize = Z_DEFAULT_CHUNK;
+    } else if (chunkSize < Z_MIN_CHUNK) {
+      throw new errors.RangeError('ERR_OUT_OF_RANGE', 'options.chunkSize',
+                                  `>= ${Z_MIN_CHUNK}`, chunkSize);
    }

-    flush = opts.flush;
-    if (flush !== undefined && !Number.isNaN(flush)) {
-      if (flush < Z_NO_FLUSH || flush > Z_BLOCK || !Number.isFinite(flush))
-        throw new errors.RangeError('ERR_INVALID_OPT_VALUE', 'flush', flush);
-    } else {
-      flush = Z_NO_FLUSH;
-    }
+    flush = checkRangesOrGetDefault(
+      opts.flush, 'options.flush',
+      Z_NO_FLUSH, Z_BLOCK, Z_NO_FLUSH);

-    finishFlush = opts.finishFlush;
-    if (finishFlush !== undefined && !Number.isNaN(finishFlush)) {
-      if (finishFlush < Z_NO_FLUSH || finishFlush > Z_BLOCK ||
-          !Number.isFinite(finishFlush)) {
-        throw new errors.RangeError('ERR_INVALID_OPT_VALUE',
-                                    'finishFlush',
-                                    finishFlush);
-      }
-    } else {
-      finishFlush = Z_FINISH;
-    }
+    finishFlush = checkRangesOrGetDefault(
+      opts.finishFlush, 'options.finishFlush',
+      Z_NO_FLUSH, Z_BLOCK, Z_FINISH);

-    windowBits = opts.windowBits;
-    if (windowBits !== undefined && !Number.isNaN(windowBits)) {
-      if (windowBits < Z_MIN_WINDOWBITS || windowBits > Z_MAX_WINDOWBITS ||
-          !Number.isFinite(windowBits)) {
-        throw new errors.RangeError('ERR_INVALID_OPT_VALUE',
-                                    'windowBits',
-                                    windowBits);
-      }
-    } else {
-      windowBits = Z_DEFAULT_WINDOWBITS;
-    }
+    windowBits = checkRangesOrGetDefault(
+      opts.windowBits, 'options.windowBits',
+      Z_MIN_WINDOWBITS, Z_MAX_WINDOWBITS, Z_DEFAULT_WINDOWBITS);

-    level = opts.level;
-    if (level !== undefined && !Number.isNaN(level)) {
-      if (level < Z_MIN_LEVEL || level > Z_MAX_LEVEL ||
-          !Number.isFinite(level)) {
-        throw new errors.RangeError('ERR_INVALID_OPT_VALUE',
-                                    'level', level);
-      }
-    } else {
-      level = Z_DEFAULT_COMPRESSION;
-    }
+    level = checkRangesOrGetDefault(
+      opts.level, 'options.level',
+      Z_MIN_LEVEL, Z_MAX_LEVEL, Z_DEFAULT_COMPRESSION);

-    memLevel = opts.memLevel;
-    if (memLevel !== undefined && !Number.isNaN(memLevel)) {
-      if (memLevel < Z_MIN_MEMLEVEL || memLevel > Z_MAX_MEMLEVEL ||
-          !Number.isFinite(memLevel)) {
-        throw new errors.RangeError('ERR_INVALID_OPT_VALUE',
-                                    'memLevel', memLevel);
-      }
-    } else {
-      memLevel = Z_DEFAULT_MEMLEVEL;
-    }
+    memLevel = checkRangesOrGetDefault(
+      opts.memLevel, 'options.memLevel',
+      Z_MIN_MEMLEVEL, Z_MAX_MEMLEVEL, Z_DEFAULT_MEMLEVEL);

-    strategy = opts.strategy;
-    if (strategy !== undefined && !Number.isNaN(strategy)) {
-      if (strategy < Z_DEFAULT_STRATEGY || strategy > Z_FIXED ||
-          !Number.isFinite(strategy)) {
-        throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
-                                   'strategy', strategy);
-      }
-    } else {
-      strategy = Z_DEFAULT_STRATEGY;
-    }
+    strategy = checkRangesOrGetDefault(
+      opts.strategy, 'options.strategy',
+      Z_DEFAULT_STRATEGY, Z_FIXED, Z_DEFAULT_STRATEGY);

    dictionary = opts.dictionary;
    if (dictionary !== undefined && !isArrayBufferView(dictionary)) {
      if (isAnyArrayBuffer(dictionary)) {
        dictionary = Buffer.from(dictionary);
      } else {
-        throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
-                                   'dictionary',
+        throw new errors.TypeError(
+          'ERR_INVALID_ARG_TYPE', 'options.dictionary',
+          ['Buffer', 'TypedArray', 'DataView', 'ArrayBuffer'],
          dictionary);
      }
    }
@ -310,14 +314,8 @@ Object.defineProperty(Zlib.prototype, '_closed', {
 });

 Zlib.prototype.params = function params(level, strategy, callback) {
-  if (level < Z_MIN_LEVEL || level > Z_MAX_LEVEL)
-    throw new errors.RangeError('ERR_INVALID_ARG_VALUE', 'level', level);
-
-  if (strategy !== undefined &&
-      (strategy < Z_DEFAULT_STRATEGY || strategy > Z_FIXED ||
-       !Number.isFinite(strategy))) {
-    throw new errors.TypeError('ERR_INVALID_ARG_VALUE', 'strategy', strategy);
-  }
+  checkRangesOrGetDefault(level, 'level', Z_MIN_LEVEL, Z_MAX_LEVEL);
+  checkRangesOrGetDefault(strategy, 'strategy', Z_DEFAULT_STRATEGY, Z_FIXED);

  if (this._level !== level || this._strategy !== strategy) {
    this.flush(Z_SYNC_FLUSH,
--- a/test/parallel/test-zlib-deflate-constructors.js
+++ b/test/parallel/test-zlib-deflate-constructors.js
@ -12,83 +12,201 @@ assert.ok(new zlib.Deflate() instanceof zlib.Deflate);
 assert.ok(zlib.DeflateRaw() instanceof zlib.DeflateRaw);
 assert.ok(new zlib.DeflateRaw() instanceof zlib.DeflateRaw);

-// Throws if `opts.chunkSize` is invalid
+// Throws if `options.chunkSize` is invalid
+common.expectsError(
+  () => new zlib.Deflate({ chunkSize: 'test' }),
+  {
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError,
+    message: 'The "options.chunkSize" property must be of type number. ' +
+             'Received type string'
+  }
+);
+
 common.expectsError(
  () => new zlib.Deflate({ chunkSize: -Infinity }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.chunkSize" is out of range. It must ' +
+             'be a finite number. Received -Infinity'
+  }
+);
+
+common.expectsError(
+  () => new zlib.Deflate({ chunkSize: 0 }),
+  {
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.chunkSize" is out of range. It must ' +
+             'be >= 64. Received 0'
  }
 );

 // Confirm that maximum chunk size cannot be exceeded because it is `Infinity`.
 assert.strictEqual(zlib.constants.Z_MAX_CHUNK, Infinity);

-// Throws if `opts.windowBits` is invalid
+// Throws if `options.windowBits` is invalid
+common.expectsError(
+  () => new zlib.Deflate({ windowBits: 'test' }),
+  {
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError,
+    message: 'The "options.windowBits" property must be of type number. ' +
+             'Received type string'
+  }
+);
+
 common.expectsError(
  () => new zlib.Deflate({ windowBits: -Infinity }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.windowBits" is out of range. It must ' +
+             'be a finite number. Received -Infinity'
  }
 );

 common.expectsError(
  () => new zlib.Deflate({ windowBits: Infinity }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.windowBits" is out of range. It must ' +
+             'be a finite number. Received Infinity'
+  }
+);
+
+common.expectsError(
+  () => new zlib.Deflate({ windowBits: 0 }),
+  {
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.windowBits" is out of range. It must ' +
+             'be >= 8 and <= 15. Received 0'
+  }
+);
+
+// Throws if `options.level` is invalid
+common.expectsError(
+  () => new zlib.Deflate({ level: 'test' }),
+  {
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError,
+    message: 'The "options.level" property must be of type number. ' +
+             'Received type string'
  }
 );

-// Throws if `opts.level` is invalid
 common.expectsError(
  () => new zlib.Deflate({ level: -Infinity }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.level" is out of range. It must ' +
+             'be a finite number. Received -Infinity'
  }
 );

 common.expectsError(
  () => new zlib.Deflate({ level: Infinity }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.level" is out of range. It must ' +
+             'be a finite number. Received Infinity'
+  }
+);
+
+common.expectsError(
+  () => new zlib.Deflate({ level: -2 }),
+  {
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.level" is out of range. It must ' +
+             'be >= -1 and <= 9. Received -2'
+  }
+);
+
+// Throws if `level` invalid in  `Deflate.prototype.params()`
+common.expectsError(
+  () => new zlib.Deflate().params('test'),
+  {
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError,
+    message: 'The "level" argument must be of type number. ' +
+             'Received type string'
  }
 );

-// Throws a RangeError if `level` invalid in  `Deflate.prototype.params()`
 common.expectsError(
  () => new zlib.Deflate().params(-Infinity),
  {
-    code: 'ERR_INVALID_ARG_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "level" is out of range. It must ' +
+             'be a finite number. Received -Infinity'
  }
 );

 common.expectsError(
  () => new zlib.Deflate().params(Infinity),
  {
-    code: 'ERR_INVALID_ARG_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "level" is out of range. It must ' +
+             'be a finite number. Received Infinity'
+  }
+);
+
+common.expectsError(
+  () => new zlib.Deflate().params(-2),
+  {
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "level" is out of range. It must ' +
+             'be >= -1 and <= 9. Received -2'
+  }
+);
+
+// Throws if options.memLevel is invalid
+common.expectsError(
+  () => new zlib.Deflate({ memLevel: 'test' }),
+  {
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError,
+    message: 'The "options.memLevel" property must be of type number. ' +
+             'Received type string'
  }
 );

-// Throws if `opts.memLevel` is invalid
 common.expectsError(
  () => new zlib.Deflate({ memLevel: -Infinity }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.memLevel" is out of range. It must ' +
+             'be a finite number. Received -Infinity'
  }
 );

 common.expectsError(
  () => new zlib.Deflate({ memLevel: Infinity }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.memLevel" is out of range. It must ' +
+             'be a finite number. Received Infinity'
+  }
+);
+
+common.expectsError(
+  () => new zlib.Deflate({ memLevel: -2 }),
+  {
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.memLevel" is out of range. It must ' +
+             'be >= 1 and <= 9. Received -2'
  }
 );

@ -99,30 +217,85 @@ new zlib.Deflate({ strategy: zlib.constants.Z_RLE });
 new zlib.Deflate({ strategy: zlib.constants.Z_FIXED });
 new zlib.Deflate({ strategy: zlib.constants.Z_DEFAULT_STRATEGY });

-// Throws if opt.strategy is the wrong type.
+// Throws if options.strategy is invalid
 common.expectsError(
-  () => new zlib.Deflate({ strategy: String(zlib.constants.Z_RLE) }),
+  () => new zlib.Deflate({ strategy: 'test' }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: TypeError
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError,
+    message: 'The "options.strategy" property must be of type number. ' +
+             'Received type string'
  }
 );

-// Throws if opts.strategy is invalid
 common.expectsError(
-  () => new zlib.Deflate({ strategy: 'this is a bogus strategy' }),
+  () => new zlib.Deflate({ strategy: -Infinity }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: TypeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.strategy" is out of range. It must ' +
+             'be a finite number. Received -Infinity'
+  }
+);
+
+common.expectsError(
+  () => new zlib.Deflate({ strategy: Infinity }),
+  {
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.strategy" is out of range. It must ' +
+             'be a finite number. Received Infinity'
+  }
+);
+
+common.expectsError(
+  () => new zlib.Deflate({ strategy: -2 }),
+  {
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.strategy" is out of range. It must ' +
+             'be >= 0 and <= 4. Received -2'
  }
 );

 // Throws TypeError if `strategy` is invalid in `Deflate.prototype.params()`
 common.expectsError(
-  () => new zlib.Deflate().params(0, 'I am an invalid strategy'),
+  () => new zlib.Deflate().params(0, 'test'),
  {
-    code: 'ERR_INVALID_ARG_VALUE',
-    type: TypeError
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError,
+    message: 'The "strategy" argument must be of type number. ' +
+             'Received type string'
+  }
+);
+
+common.expectsError(
+  () => new zlib.Deflate().params(0, -Infinity),
+  {
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "strategy" is out of range. It must ' +
+             'be a finite number. Received -Infinity'
+  }
+);
+
+common.expectsError(
+  () => new zlib.Deflate().params(0, Infinity),
+  {
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "strategy" is out of range. It must ' +
+             'be a finite number. Received Infinity'
+  }
+);
+
+common.expectsError(
+  () => new zlib.Deflate().params(0, -2),
+  {
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "strategy" is out of range. It must ' +
+             'be >= 0 and <= 4. Received -2'
  }
 );

@ -130,7 +303,9 @@ common.expectsError(
 common.expectsError(
  () => new zlib.Deflate({ dictionary: 'not a buffer' }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: TypeError
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError,
+    message: 'The "options.dictionary" property must be one of type Buffer, ' +
+             'TypedArray, DataView, or ArrayBuffer. Received type string'
  }
 );
--- a/test/parallel/test-zlib-failed-init.js
+++ b/test/parallel/test-zlib-failed-init.js
@ -8,24 +8,30 @@ const zlib = require('zlib');
 common.expectsError(
  () => zlib.createGzip({ chunkSize: 0 }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.chunkSize" is out of range. It must ' +
+             'be >= 64. Received 0'
  }
 );

 common.expectsError(
  () => zlib.createGzip({ windowBits: 0 }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.windowBits" is out of range. It must ' +
+             'be >= 8 and <= 15. Received 0'
  }
 );

 common.expectsError(
  () => zlib.createGzip({ memLevel: 0 }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.memLevel" is out of range. It must ' +
+             'be >= 1 and <= 9. Received 0'
  }
 );

--- a/test/parallel/test-zlib-flush-flags.js
+++ b/test/parallel/test-zlib-flush-flags.js
@ -7,16 +7,20 @@ zlib.createGzip({ flush: zlib.constants.Z_SYNC_FLUSH });
 common.expectsError(
  () => zlib.createGzip({ flush: 'foobar' }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError,
+    message: 'The "options.flush" property must be of type number. ' +
+             'Received type string'
  }
 );

 common.expectsError(
  () => zlib.createGzip({ flush: 10000 }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.flush" is out of range. It must ' +
+             'be >= 0 and <= 5. Received 10000'
  }
 );

@ -25,15 +29,19 @@ zlib.createGzip({ finishFlush: zlib.constants.Z_SYNC_FLUSH });
 common.expectsError(
  () => zlib.createGzip({ finishFlush: 'foobar' }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError,
+    message: 'The "options.finishFlush" property must be of type number. ' +
+             'Received type string'
  }
 );

 common.expectsError(
  () => zlib.createGzip({ finishFlush: 10000 }),
  {
-    code: 'ERR_INVALID_OPT_VALUE',
-    type: RangeError
+    code: 'ERR_OUT_OF_RANGE',
+    type: RangeError,
+    message: 'The value of "options.finishFlush" is out of range. It must ' +
+             'be >= 0 and <= 5. Received 10000'
  }
 );