From 916c16187faeb06259d74b6eb493f8c68c9140de Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 29 Feb 2020 15:05:28 -0800 Subject: [PATCH 1/3] fs: throw rather than assert on errant monkey-patching It is possible to make `.close()` assert on an FSWatcher if the `_handle` property is monkey-patched. Assertions should be reserved for things that are believed to be logically impossible. Since we know end users can cause the assertion, change it to a throw. --- doc/api/errors.md | 6 ++++++ lib/internal/errors.js | 3 +++ lib/internal/fs/watchers.js | 9 ++++++--- test/sequential/test-fs-watch.js | 8 ++++++-- 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 11d0036386cc45..a1bdbb8e2f54e9 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1966,6 +1966,12 @@ from another module. A string that contained unescaped characters was received. + +### `ERR_UNEXPECTED_INSTANCE` + +A value's prototype chain lacks an expected constructor. This could be a result +of monkey-patching gone awry or prototype poisoning. + ### `ERR_UNHANDLED_ERROR` diff --git a/lib/internal/errors.js b/lib/internal/errors.js index be7385644b07b9..93f405d77a132c 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1348,6 +1348,9 @@ E('ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET', 'callback was already active', Error); E('ERR_UNESCAPED_CHARACTERS', '%s contains unescaped characters', TypeError); +E('ERR_UNEXPECTED_INSTANCE', + '%s must have %s in its prototype chain', + TypeError); E('ERR_UNHANDLED_ERROR', // Using a default argument here is important so the argument is not counted // towards `Function#length`. diff --git a/lib/internal/fs/watchers.js b/lib/internal/fs/watchers.js index 118f85d9ada181..bf00ee5b6151cd 100644 --- a/lib/internal/fs/watchers.js +++ b/lib/internal/fs/watchers.js @@ -24,7 +24,6 @@ const { } = require('internal/async_hooks'); const { toNamespacedPath } = require('path'); const { validateUint32 } = require('internal/validators'); -const assert = require('internal/assert'); const kOldStatus = Symbol('kOldStatus'); const kUseBigint = Symbol('kUseBigint'); @@ -163,7 +162,9 @@ FSWatcher.prototype[kFSWatchStart] = function(filename, if (this._handle === null) { // closed return; } - assert(this._handle instanceof FSEvent, 'handle must be a FSEvent'); + if (!(this._handle instanceof FSEvent)) { + throw new errors.codes.ERR_UNEXPECTED_INSTANCE('handle', 'FSEvent'); + } if (this._handle.initialized) { // already started return; } @@ -199,7 +200,9 @@ FSWatcher.prototype.close = function() { if (this._handle === null) { // closed return; } - assert(this._handle instanceof FSEvent, 'handle must be a FSEvent'); + if (!(this._handle instanceof FSEvent)) { + throw new errors.codes.ERR_UNEXPECTED_INSTANCE('handle', 'FSEvent'); + } if (!this._handle.initialized) { // not started return; } diff --git a/test/sequential/test-fs-watch.js b/test/sequential/test-fs-watch.js index 031e92c61c0b3c..73e4df9accc128 100644 --- a/test/sequential/test-fs-watch.js +++ b/test/sequential/test-fs-watch.js @@ -117,14 +117,18 @@ tmpdir.refresh(); // https://github.com/joyent/node/issues/6690 { let oldhandle; - common.expectsInternalAssertion( + assert.throws( () => { const w = fs.watch(__filename, common.mustNotCall()); oldhandle = w._handle; w._handle = { close: w._handle.close }; w.close(); }, - 'handle must be a FSEvent' + { + name: 'TypeError', + code: 'ERR_UNEXPECTED_INSTANCE', + message: 'handle must have FSEvent in its prototype chain', + } ); oldhandle.close(); // clean up } From 659f06ce4e0b0c50e85e64afdaf81ebcb4165bab Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 29 Feb 2020 15:14:38 -0800 Subject: [PATCH 2/3] test: remove common.expectsInternal Assertion Remove convenience function for internal assertions. It is only used once. --- test/common/index.js | 14 -------------- test/parallel/test-internal-errors.js | 11 +++++++---- 2 files changed, 7 insertions(+), 18 deletions(-) diff --git a/test/common/index.js b/test/common/index.js index 653de4685ca7a0..28ce841c48cc3f 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -547,19 +547,6 @@ function expectsError(validator, exact) { }, exact); } -const suffix = 'This is caused by either a bug in Node.js ' + - 'or incorrect usage of Node.js internals.\n' + - 'Please open an issue with this stack trace at ' + - 'https://github.com/nodejs/node/issues\n'; - -function expectsInternalAssertion(fn, message) { - assert.throws(fn, { - message: `${message}\n${suffix}`, - name: 'Error', - code: 'ERR_INTERNAL_ASSERTION' - }); -} - function skipIfInspectorDisabled() { if (!process.features.inspector) { skip('V8 inspector is disabled'); @@ -680,7 +667,6 @@ const common = { createZeroFilledFile, disableCrashOnUnhandledRejection, expectsError, - expectsInternalAssertion, expectWarning, getArrayBufferViews, getBufferSources, diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index fbb8a0a86a3852..7bcc7dcc330c2c 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -1,6 +1,6 @@ // Flags: --expose-internals 'use strict'; -const common = require('../common'); +require('../common'); const { hijackStdout, restoreStdout, @@ -50,10 +50,13 @@ errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`, Error); } { - common.expectsInternalAssertion( + assert.throws( () => new errors.codes.TEST_ERROR_1(), - 'Code: TEST_ERROR_1; The provided arguments ' + - 'length (0) does not match the required ones (1).' + { + message: /^Code: TEST_ERROR_1; The provided arguments length \(0\) does not match the required ones \(1\)\./, + name: 'Error', + code: 'ERR_INTERNAL_ASSERTION' + } ); } From 92ffbb8817daf90361487b4710feb56a8974e36c Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 29 Feb 2020 16:19:17 -0800 Subject: [PATCH 3/3] test: add coverage for FSWatcher exception Cover an previously uncovered exception possible in the internal start function for FSWatcher. --- test/sequential/test-fs-watch.js | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/sequential/test-fs-watch.js b/test/sequential/test-fs-watch.js index 73e4df9accc128..388fb74865019e 100644 --- a/test/sequential/test-fs-watch.js +++ b/test/sequential/test-fs-watch.js @@ -132,3 +132,25 @@ tmpdir.refresh(); ); oldhandle.close(); // clean up } + +{ + let oldhandle; + assert.throws( + () => { + const w = fs.watch(__filename, common.mustNotCall()); + oldhandle = w._handle; + const protoSymbols = + Object.getOwnPropertySymbols(Object.getPrototypeOf(w)); + const kFSWatchStart = + protoSymbols.find((val) => val.toString() === 'Symbol(kFSWatchStart)'); + w._handle = {}; + w[kFSWatchStart](); + }, + { + name: 'TypeError', + code: 'ERR_UNEXPECTED_INSTANCE', + message: 'handle must have FSEvent in its prototype chain', + } + ); + oldhandle.close(); // clean up +}