From 52eb6035107c1a35fe0d5875c4f6c985826843d9 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 14 May 2018 14:30:24 +0200 Subject: [PATCH 1/3] test: remove untested knownGlobals These values are all non-enumerable and will never be checked. By removing them, we make sure they will not become enumerable unnoticed. --- test/common/index.js | 25 ------------------------- test/common/index.mjs | 25 ------------------------- test/message/console_low_stack_space.js | 9 ++++++--- 3 files changed, 6 insertions(+), 53 deletions(-) diff --git a/test/common/index.js b/test/common/index.js index 07c0992d65e8d2..743bf15fb8301d 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -310,8 +310,6 @@ let knownGlobals = [ clearImmediate, clearInterval, clearTimeout, - console, - constructor, // Enumerable in V8 3.21. global, process, setImmediate, @@ -341,29 +339,6 @@ if (global.COUNTER_NET_SERVER_CONNECTION) { knownGlobals.push(COUNTER_HTTP_CLIENT_RESPONSE); } -if (global.ArrayBuffer) { - knownGlobals.push(ArrayBuffer); - knownGlobals.push(Int8Array); - knownGlobals.push(Uint8Array); - knownGlobals.push(Uint8ClampedArray); - knownGlobals.push(Int16Array); - knownGlobals.push(Uint16Array); - knownGlobals.push(Int32Array); - knownGlobals.push(Uint32Array); - knownGlobals.push(Float32Array); - knownGlobals.push(Float64Array); - knownGlobals.push(DataView); -} - -// Harmony features. -if (global.Proxy) { - knownGlobals.push(Proxy); -} - -if (global.Symbol) { - knownGlobals.push(Symbol); -} - if (process.env.NODE_TEST_KNOWN_GLOBALS) { const knownFromEnv = process.env.NODE_TEST_KNOWN_GLOBALS.split(','); allowGlobals(...knownFromEnv); diff --git a/test/common/index.mjs b/test/common/index.mjs index 49b9e56bd9a10b..dd4ee2146e0720 100644 --- a/test/common/index.mjs +++ b/test/common/index.mjs @@ -8,8 +8,6 @@ let knownGlobals = [ clearImmediate, clearInterval, clearTimeout, - console, - constructor, // Enumerable in V8 3.21. global, process, setImmediate, @@ -50,29 +48,6 @@ export function leakedGlobals() { knownGlobals.push(COUNTER_HTTP_CLIENT_RESPONSE); } - if (global.ArrayBuffer) { - knownGlobals.push(ArrayBuffer); - knownGlobals.push(Int8Array); - knownGlobals.push(Uint8Array); - knownGlobals.push(Uint8ClampedArray); - knownGlobals.push(Int16Array); - knownGlobals.push(Uint16Array); - knownGlobals.push(Int32Array); - knownGlobals.push(Uint32Array); - knownGlobals.push(Float32Array); - knownGlobals.push(Float64Array); - knownGlobals.push(DataView); - } - - // Harmony features. - if (global.Proxy) { - knownGlobals.push(Proxy); - } - - if (global.Symbol) { - knownGlobals.push(Symbol); - } - const leaked = []; for (const val in global) { diff --git a/test/message/console_low_stack_space.js b/test/message/console_low_stack_space.js index 9128eb49d120e0..4834ffd9cd0b36 100644 --- a/test/message/console_low_stack_space.js +++ b/test/message/console_low_stack_space.js @@ -1,8 +1,11 @@ 'use strict'; -// copy console accessor because requiring ../common touches it +// Copy console accessor because requiring ../common touches it const consoleDescriptor = Object.getOwnPropertyDescriptor(global, 'console'); -delete global.console; -global.console = {}; +Object.defineProperty(global, 'console', { + configurable: true, + writable: true, + value: {} +}); require('../common'); From e0ffaf6a6052d6023bb680ec13ab1d1eed181882 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 14 May 2018 14:51:50 +0200 Subject: [PATCH 2/3] repl: make console, module and require non-enumerable This aligns these globals with the regular context. --- lib/repl.js | 15 +++++-- test/parallel/test-repl-console.js | 44 ------------------- test/parallel/test-repl-context.js | 39 +++++++++------- ...test-repl-function-definition-edge-case.js | 4 +- test/parallel/test-repl-let-process.js | 2 - test/parallel/test-repl-mode.js | 4 +- test/parallel/test-repl-options.js | 2 - test/parallel/test-repl-reset-event.js | 1 - 8 files changed, 36 insertions(+), 75 deletions(-) delete mode 100644 test/parallel/test-repl-console.js diff --git a/lib/repl.js b/lib/repl.js index 600816a6058fbc..6b5f4803870c4a 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -783,7 +783,7 @@ REPLServer.prototype.createContext = function() { const _console = new Console(this.outputStream); Object.defineProperty(context, 'console', { configurable: true, - enumerable: true, + writable: true, value: _console }); @@ -803,9 +803,16 @@ REPLServer.prototype.createContext = function() { module.paths = CJSModule._resolveLookupPaths('', parentModule, true) || []; - var require = makeRequireFunction(module); - context.module = module; - context.require = require; + Object.defineProperty(context, 'module', { + configurable: true, + writable: true, + value: module + }); + Object.defineProperty(context, 'require', { + configurable: true, + writable: true, + value: makeRequireFunction(module) + }); addBuiltinLibsToObject(context); diff --git a/test/parallel/test-repl-console.js b/test/parallel/test-repl-console.js deleted file mode 100644 index 94547e4768bb76..00000000000000 --- a/test/parallel/test-repl-console.js +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright Joyent, Inc. and other Node contributors. -// -// Permission is hereby granted, free of charge, to any person obtaining a -// copy of this software and associated documentation files (the -// "Software"), to deal in the Software without restriction, including -// without limitation the rights to use, copy, modify, merge, publish, -// distribute, sublicense, and/or sell copies of the Software, and to permit -// persons to whom the Software is furnished to do so, subject to the -// following conditions: -// -// The above copyright notice and this permission notice shall be included -// in all copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS -// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF -// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN -// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, -// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR -// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE -// USE OR OTHER DEALINGS IN THE SOFTWARE. - -'use strict'; -const common = require('../common'); -const assert = require('assert'); -const repl = require('repl'); - -// Create a dummy stream that does nothing -const stream = new common.ArrayStream(); - -const r = repl.start({ - input: stream, - output: stream, - useGlobal: false -}); - - -// ensure that the repl context gets its own "console" instance -assert(r.context.console); - -// ensure that the repl console instance is not the global one -assert.notStrictEqual(r.context.console, console); - -// ensure that the repl console instance does not have a setter -assert.throws(() => r.context.console = 'foo', TypeError); diff --git a/test/parallel/test-repl-context.js b/test/parallel/test-repl-context.js index 9d18067bc2aca4..914aa563bd50fb 100644 --- a/test/parallel/test-repl-context.js +++ b/test/parallel/test-repl-context.js @@ -4,32 +4,39 @@ const assert = require('assert'); const repl = require('repl'); const vm = require('vm'); -// Create a dummy stream that does nothing +// Create a dummy stream that does nothing. const stream = new common.ArrayStream(); -// Test when useGlobal is false -testContext(repl.start({ - input: stream, - output: stream, - useGlobal: false -})); +// Test context when useGlobal is false. +{ + const r = repl.start({ + input: stream, + output: stream, + useGlobal: false + }); -function testContext(repl) { - const context = repl.createContext(); - // ensure that the repl context gets its own "console" instance + // Ensure that the repl context gets its own "console" instance. + assert(r.context.console); + + // Ensure that the repl console instance is not the global one. + assert.notStrictEqual(r.context.console, console); + + const context = r.createContext(); + // Ensure that the repl context gets its own "console" instance. assert(context.console instanceof require('console').Console); - // ensure that the repl's global property is the context + // Ensure that the repl's global property is the context. assert.strictEqual(context.global, context); - // ensure that the repl console instance does not have a setter - assert.throws(() => context.console = 'foo', TypeError); - repl.close(); + // Ensure that the repl console instance is writable. + context.console = 'foo'; + r.close(); } -testContextSideEffects(repl.start({ input: stream, output: stream })); +// Test for context side effects. +{ + const server = repl.start({ input: stream, output: stream }); -function testContextSideEffects(server) { assert.ok(!server.underscoreAssigned); assert.strictEqual(server.lines.length, 0); diff --git a/test/parallel/test-repl-function-definition-edge-case.js b/test/parallel/test-repl-function-definition-edge-case.js index 1e3063e3db53ff..952fba4103cc26 100644 --- a/test/parallel/test-repl-function-definition-edge-case.js +++ b/test/parallel/test-repl-function-definition-edge-case.js @@ -1,12 +1,10 @@ // Reference: https://github.com/nodejs/node/pull/7624 'use strict'; -const common = require('../common'); +require('../common'); const assert = require('assert'); const repl = require('repl'); const stream = require('stream'); -common.globalCheck = false; - const r = initRepl(); r.input.emit('data', 'function a() { return 42; } (1)\n'); diff --git a/test/parallel/test-repl-let-process.js b/test/parallel/test-repl-let-process.js index 3e6c3e85665be1..dd8fa60f463d8b 100644 --- a/test/parallel/test-repl-let-process.js +++ b/test/parallel/test-repl-let-process.js @@ -2,8 +2,6 @@ const common = require('../common'); const repl = require('repl'); -common.globalCheck = false; - // Regression test for https://github.com/nodejs/node/issues/6802 const input = new common.ArrayStream(); repl.start({ input, output: process.stdout, useGlobal: true }); diff --git a/test/parallel/test-repl-mode.js b/test/parallel/test-repl-mode.js index 60b430d8c7ee31..df84d86a3c8dc3 100644 --- a/test/parallel/test-repl-mode.js +++ b/test/parallel/test-repl-mode.js @@ -1,11 +1,9 @@ 'use strict'; -const common = require('../common'); +require('../common'); const assert = require('assert'); const Stream = require('stream'); const repl = require('repl'); -common.globalCheck = false; - const tests = [ testSloppyMode, testStrictMode, diff --git a/test/parallel/test-repl-options.js b/test/parallel/test-repl-options.js index 6bd908aadf935d..1de49c8e861391 100644 --- a/test/parallel/test-repl-options.js +++ b/test/parallel/test-repl-options.js @@ -24,8 +24,6 @@ const common = require('../common'); const assert = require('assert'); const repl = require('repl'); -common.globalCheck = false; - // Create a dummy stream that does nothing const stream = new common.ArrayStream(); diff --git a/test/parallel/test-repl-reset-event.js b/test/parallel/test-repl-reset-event.js index 83ab7fd6a3ec52..4b3a04aeeb88c5 100644 --- a/test/parallel/test-repl-reset-event.js +++ b/test/parallel/test-repl-reset-event.js @@ -21,7 +21,6 @@ 'use strict'; const common = require('../common'); -common.globalCheck = false; const assert = require('assert'); const repl = require('repl'); From 6af7bdad8c0b0bb38b6620045291c7720192d7f5 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 14 May 2018 16:16:33 +0200 Subject: [PATCH 3/3] test: remove common.globalCheck This flag is partially used in tests where it was not necessary and it is always possible to replace this flag with `common.allowGlobals`. This makes sure all globals are truly tested for. --- test/common/README.md | 5 ----- test/common/index.js | 6 ------ test/common/index.mjs | 4 ---- test/parallel/test-domain-crypto.js | 2 +- test/parallel/test-global.js | 2 +- test/parallel/test-repl-autolibs.js | 4 +--- test/parallel/test-repl-envvars.js | 5 +---- test/parallel/test-repl-history-perm.js | 3 --- test/parallel/test-repl-persistent-history.js | 3 --- test/parallel/test-repl-reset-event.js | 2 ++ test/parallel/test-repl-use-global.js | 3 --- test/parallel/test-repl.js | 3 +-- test/parallel/test-require-deps-deprecation.js | 5 +++-- test/parallel/test-timer-immediate.js | 2 +- test/parallel/test-vm-new-script-this-context.js | 10 ++++++++-- test/parallel/test-vm-run-in-new-context.js | 9 +++++++-- test/parallel/test-vm-static-this.js | 9 +++++++-- 17 files changed, 33 insertions(+), 44 deletions(-) diff --git a/test/common/README.md b/test/common/README.md index 08d7bbb762fbb4..111ce45b00360c 100644 --- a/test/common/README.md +++ b/test/common/README.md @@ -150,11 +150,6 @@ Attempts to get a valid TTY file descriptor. Returns `-1` if it fails. The TTY file descriptor is assumed to be capable of being writable. -### globalCheck -* [<boolean>] - -Set to `false` if the test should not check for global leaks. - ### hasCrypto * [<boolean>] diff --git a/test/common/index.js b/test/common/index.js index 743bf15fb8301d..85d11ec093414c 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -366,21 +366,15 @@ function leakedGlobals() { } exports.leakedGlobals = leakedGlobals; -// Turn this off if the test should not check for global leaks. -exports.globalCheck = true; - process.on('exit', function() { - if (!exports.globalCheck) return; const leaked = leakedGlobals(); if (leaked.length > 0) { assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`); } }); - const mustCallChecks = []; - function runCallChecks(exitCode) { if (exitCode !== 0) return; diff --git a/test/common/index.mjs b/test/common/index.mjs index dd4ee2146e0720..b7ac556bca52c3 100644 --- a/test/common/index.mjs +++ b/test/common/index.mjs @@ -63,11 +63,7 @@ export function leakedGlobals() { } } -// Turn this off if the test should not check for global leaks. -export let globalCheck = true; // eslint-disable-line - process.on('exit', function() { - if (!globalCheck) return; const leaked = leakedGlobals(); if (leaked.length > 0) { assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`); diff --git a/test/parallel/test-domain-crypto.js b/test/parallel/test-domain-crypto.js index 3890a7b4641ade..26ee6b888efb2d 100644 --- a/test/parallel/test-domain-crypto.js +++ b/test/parallel/test-domain-crypto.js @@ -29,7 +29,7 @@ if (!common.hasCrypto) const crypto = require('crypto'); // Pollution of global is intentional as part of test. -common.globalCheck = false; +common.allowGlobals(require('domain')); // See https://github.com/nodejs/node/commit/d1eff9ab global.domain = require('domain'); diff --git a/test/parallel/test-global.js b/test/parallel/test-global.js index 46db72eeceb586..b139a6287488d2 100644 --- a/test/parallel/test-global.js +++ b/test/parallel/test-global.js @@ -28,7 +28,7 @@ const fixtures = require('../common/fixtures'); const assert = require('assert'); -common.globalCheck = false; +common.allowGlobals('bar', 'foo'); baseFoo = 'foo'; // eslint-disable-line no-undef global.baseBar = 'bar'; diff --git a/test/parallel/test-repl-autolibs.js b/test/parallel/test-repl-autolibs.js index 52234deb5e732e..024dd971bf44e1 100644 --- a/test/parallel/test-repl-autolibs.js +++ b/test/parallel/test-repl-autolibs.js @@ -25,9 +25,6 @@ const assert = require('assert'); const util = require('util'); const repl = require('repl'); -// This test adds global variables -common.globalCheck = false; - const putIn = new common.ArrayStream(); repl.start('', putIn, null, true); @@ -65,6 +62,7 @@ function test2() { }; const val = {}; global.url = val; + common.allowGlobals(val); assert(!gotWrite); putIn.run(['url']); assert(gotWrite); diff --git a/test/parallel/test-repl-envvars.js b/test/parallel/test-repl-envvars.js index d29e7b3574c1f2..d6a45a664fa3b7 100644 --- a/test/parallel/test-repl-envvars.js +++ b/test/parallel/test-repl-envvars.js @@ -2,7 +2,7 @@ // Flags: --expose-internals -const common = require('../common'); +require('../common'); const stream = require('stream'); const REPL = require('internal/repl'); const assert = require('assert'); @@ -47,9 +47,6 @@ function run(test) { REPL.createInternalRepl(env, opts, function(err, repl) { assert.ifError(err); - // The REPL registers 'module' and 'require' globals - common.allowGlobals(repl.context.module, repl.context.require); - assert.strictEqual(expected.terminal, repl.terminal, `Expected ${inspect(expected)} with ${inspect(env)}`); assert.strictEqual(expected.useColors, repl.useColors, diff --git a/test/parallel/test-repl-history-perm.js b/test/parallel/test-repl-history-perm.js index b125fa551dc858..03ce1435d9ba4e 100644 --- a/test/parallel/test-repl-history-perm.js +++ b/test/parallel/test-repl-history-perm.js @@ -38,9 +38,6 @@ const replHistoryPath = path.join(tmpdir.path, '.node_repl_history'); const checkResults = common.mustCall(function(err, r) { assert.ifError(err); - // The REPL registers 'module' and 'require' globals - common.allowGlobals(r.context.module, r.context.require); - r.input.end(); const stat = fs.statSync(replHistoryPath); const fileMode = stat.mode & 0o777; diff --git a/test/parallel/test-repl-persistent-history.js b/test/parallel/test-repl-persistent-history.js index 4d0330272ab3c6..bb10085eccfcf6 100644 --- a/test/parallel/test-repl-persistent-history.js +++ b/test/parallel/test-repl-persistent-history.js @@ -216,9 +216,6 @@ function runTest(assertCleaned) { throw err; } - // The REPL registers 'module' and 'require' globals - common.allowGlobals(repl.context.module, repl.context.require); - repl.once('close', () => { if (repl._flushing) { repl.once('flushHistory', onClose); diff --git a/test/parallel/test-repl-reset-event.js b/test/parallel/test-repl-reset-event.js index 4b3a04aeeb88c5..96d1d199af34c9 100644 --- a/test/parallel/test-repl-reset-event.js +++ b/test/parallel/test-repl-reset-event.js @@ -26,6 +26,8 @@ const assert = require('assert'); const repl = require('repl'); const util = require('util'); +common.allowGlobals(42); + // Create a dummy stream that does nothing const dummy = new common.ArrayStream(); diff --git a/test/parallel/test-repl-use-global.js b/test/parallel/test-repl-use-global.js index c76505272b2682..947adc7f255035 100644 --- a/test/parallel/test-repl-use-global.js +++ b/test/parallel/test-repl-use-global.js @@ -18,9 +18,6 @@ const globalTest = (useGlobal, cb, output) => (err, repl) => { if (err) return cb(err); - // The REPL registers 'module' and 'require' globals - common.allowGlobals(repl.context.module, repl.context.require); - let str = ''; output.on('data', (data) => (str += data)); global.lunch = 'tacos'; diff --git a/test/parallel/test-repl.js b/test/parallel/test-repl.js index e14398541dd8de..5459371f00c24d 100644 --- a/test/parallel/test-repl.js +++ b/test/parallel/test-repl.js @@ -26,7 +26,6 @@ const assert = require('assert'); const net = require('net'); const repl = require('repl'); -common.globalCheck = false; common.crashOnUnhandledRejection(); const message = 'Read, Eval, Print Loop'; @@ -41,7 +40,6 @@ global.invoke_me = function(arg) { return `invoked ${arg}`; }; - // Helpers for describing the expected output: const kArrow = /^ *\^+ *$/; // Arrow of ^ pointing to syntax error location const kSource = Symbol('kSource'); // Placeholder standing for input readback @@ -755,6 +753,7 @@ const tcpTests = [ socket.end(); } + common.allowGlobals(...Object.values(global)); })(); function startTCPRepl() { diff --git a/test/parallel/test-require-deps-deprecation.js b/test/parallel/test-require-deps-deprecation.js index 24a2e86e6a42ee..80bf66d7f6b74f 100644 --- a/test/parallel/test-require-deps-deprecation.js +++ b/test/parallel/test-require-deps-deprecation.js @@ -2,8 +2,6 @@ const common = require('../common'); const assert = require('assert'); -// The v8 modules when imported leak globals. Disable global check. -common.globalCheck = false; const deprecatedModules = [ 'node-inspect/lib/_inspect', @@ -53,3 +51,6 @@ for (const m of deps) { } assert.notStrictEqual(path, m); } + +// The V8 modules add the WebInspector to the globals. +common.allowGlobals(global.WebInspector); diff --git a/test/parallel/test-timer-immediate.js b/test/parallel/test-timer-immediate.js index 385fa4baca4ee5..b0f52db1b713d3 100644 --- a/test/parallel/test-timer-immediate.js +++ b/test/parallel/test-timer-immediate.js @@ -1,5 +1,5 @@ 'use strict'; const common = require('../common'); -common.globalCheck = false; global.process = {}; // Boom! +common.allowGlobals(global.process); setImmediate(common.mustCall()); diff --git a/test/parallel/test-vm-new-script-this-context.js b/test/parallel/test-vm-new-script-this-context.js index 6da47e67249c2b..2c0c21690e1821 100644 --- a/test/parallel/test-vm-new-script-this-context.js +++ b/test/parallel/test-vm-new-script-this-context.js @@ -24,8 +24,6 @@ const common = require('../common'); const assert = require('assert'); const Script = require('vm').Script; -common.globalCheck = false; - // Run a string let script = new Script('\'passed\';'); const result = script.runInThisContext(script); @@ -60,3 +58,11 @@ global.f = function() { global.foo = 100; }; script = new Script('f()'); script.runInThisContext(script); assert.strictEqual(100, global.foo); + +common.allowGlobals( + global.hello, + global.code, + global.foo, + global.obj, + global.f +); diff --git a/test/parallel/test-vm-run-in-new-context.js b/test/parallel/test-vm-run-in-new-context.js index 1edb061ea6a871..e844cd6816bba3 100644 --- a/test/parallel/test-vm-run-in-new-context.js +++ b/test/parallel/test-vm-run-in-new-context.js @@ -29,8 +29,6 @@ const vm = require('vm'); assert.strictEqual(typeof global.gc, 'function', 'Run this test with --expose-gc'); -common.globalCheck = false; - // Run a string const result = vm.runInNewContext('\'passed\';'); assert.strictEqual(result, 'passed'); @@ -93,3 +91,10 @@ fn(); return true; }); } + +common.allowGlobals( + global.hello, + global.code, + global.foo, + global.obj +); diff --git a/test/parallel/test-vm-static-this.js b/test/parallel/test-vm-static-this.js index 5306e31dc0e4a7..d4530604dc06f0 100644 --- a/test/parallel/test-vm-static-this.js +++ b/test/parallel/test-vm-static-this.js @@ -24,8 +24,6 @@ const common = require('../common'); const assert = require('assert'); const vm = require('vm'); -common.globalCheck = false; - // Run a string const result = vm.runInThisContext('\'passed\';'); assert.strictEqual('passed', result); @@ -58,3 +56,10 @@ assert.strictEqual(1, global.foo); global.f = function() { global.foo = 100; }; vm.runInThisContext('f()'); assert.strictEqual(100, global.foo); + +common.allowGlobals( + global.hello, + global.foo, + global.obj, + global.f +);