From d29c0a9bb70587a4f380ad5643fbfe1267701043 Mon Sep 17 00:00:00 2001 From: raisinten Date: Mon, 28 Dec 2020 20:04:04 +0530 Subject: [PATCH] repl: refactor to avoid unsafe array iteration PR-URL: https://github.com/nodejs/node/pull/36663 Reviewed-By: Antoine du Hamel Reviewed-By: James M Snell --- lib/internal/repl/utils.js | 2 +- lib/internal/util/inspect.js | 9 +-- lib/repl.js | 45 ++++++------ test/parallel/test-repl-history-navigation.js | 51 +++++++++++++- .../test-repl-unsafe-array-iteration.js | 68 +++++++++++++++++++ 5 files changed, 146 insertions(+), 29 deletions(-) create mode 100644 test/parallel/test-repl-unsafe-array-iteration.js diff --git a/lib/internal/repl/utils.js b/lib/internal/repl/utils.js index 594b6a0c4485c7..8fee6d40123c49 100644 --- a/lib/internal/repl/utils.js +++ b/lib/internal/repl/utils.js @@ -245,7 +245,7 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) { } // Result and the text that was completed. - const [rawCompletions, completeOn] = data; + const { 0: rawCompletions, 1: completeOn } = data; if (!rawCompletions || rawCompletions.length === 0) { return; diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js index b03380030a6be5..b20ce167f9da44 100644 --- a/lib/internal/util/inspect.js +++ b/lib/internal/util/inspect.js @@ -309,7 +309,8 @@ function inspect(value, opts) { ctx.showHidden = opts; } else if (opts) { const optKeys = ObjectKeys(opts); - for (const key of optKeys) { + for (let i = 0; i < optKeys.length; ++i) { + const key = optKeys[i]; // TODO(BridgeAR): Find a solution what to do about stylize. Either make // this function public or add a new API with a similar or better // functionality. @@ -1869,7 +1870,7 @@ function tryStringify(arg) { } function format(...args) { - return formatWithOptionsInternal(undefined, ...args); + return formatWithOptionsInternal(undefined, args); } function formatWithOptions(inspectOptions, ...args) { @@ -1877,10 +1878,10 @@ function formatWithOptions(inspectOptions, ...args) { throw new ERR_INVALID_ARG_TYPE( 'inspectOptions', 'object', inspectOptions); } - return formatWithOptionsInternal(inspectOptions, ...args); + return formatWithOptionsInternal(inspectOptions, args); } -function formatWithOptionsInternal(inspectOptions, ...args) { +function formatWithOptionsInternal(inspectOptions, args) { const first = args[0]; let a = 0; let str = ''; diff --git a/lib/repl.js b/lib/repl.js index 3368b5997ae01d..b1905195ab4b47 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -46,6 +46,7 @@ const { ArrayPrototypeConcat, ArrayPrototypeFilter, ArrayPrototypeFindIndex, + ArrayPrototypeForEach, ArrayPrototypeIncludes, ArrayPrototypeJoin, ArrayPrototypeMap, @@ -663,7 +664,7 @@ function REPLServer(prompt, let matched = false; errStack = ''; - for (const line of lines) { + ArrayPrototypeForEach(lines, (line) => { if (!matched && RegExpPrototypeTest(/^\[?([A-Z][a-z0-9_]*)*Error/, line)) { errStack += writer.options.breakLength >= line.length ? @@ -673,7 +674,7 @@ function REPLServer(prompt, } else { errStack += line; } - } + }); if (!matched) { const ln = lines.length === 1 ? ' ' : ':\n'; errStack = `Uncaught${ln}${errStack}`; @@ -754,9 +755,7 @@ function REPLServer(prompt, const prioritizedSigintQueue = new SafeSet(); self.on('SIGINT', function onSigInt() { if (prioritizedSigintQueue.size > 0) { - for (const task of prioritizedSigintQueue) { - task(); - } + ArrayPrototypeForEach(prioritizedSigintQueue, (task) => task()); return; } @@ -1010,13 +1009,13 @@ REPLServer.prototype.createContext = function() { }, () => { context = vm.createContext(); }); - for (const name of ObjectGetOwnPropertyNames(global)) { + ArrayPrototypeForEach(ObjectGetOwnPropertyNames(global), (name) => { // Only set properties that do not already exist as a global builtin. if (!globalBuiltins.has(name)) { ObjectDefineProperty(context, name, ObjectGetOwnPropertyDescriptor(global, name)); } - } + }); context.global = context; const _console = new Console(this.output); ObjectDefineProperty(context, 'console', { @@ -1231,7 +1230,7 @@ function complete(line, callback) { paths = ArrayPrototypeConcat(module.paths, CJSModule.globalPaths); } - for (let dir of paths) { + ArrayPrototypeForEach(paths, (dir) => { dir = path.resolve(dir, subdir); const dirents = gracefulReaddir(dir, { withFileTypes: true }) || []; for (const dirent of dirents) { @@ -1259,7 +1258,7 @@ function complete(line, callback) { } } } - } + }); if (group.length) { ArrayPrototypePush(completionGroups, group); } @@ -1269,7 +1268,7 @@ function complete(line, callback) { } } else if (RegExpPrototypeTest(fsAutoCompleteRE, line) && this.allowBlockingCompletions) { - [completionGroups, completeOn] = completeFSFunctions(line); + ({ 0: completionGroups, 1: completeOn } = completeFSFunctions(line)); // Handle variable member lookup. // We support simple chained expressions like the following (no function // calls, etc.). That is for simplicity and also because we *eval* that @@ -1282,7 +1281,7 @@ function complete(line, callback) { // foo.<|> # completions for 'foo' with filter '' } else if (line.length === 0 || RegExpPrototypeTest(/\w|\.|\$/, line[line.length - 1])) { - const [match] = RegExpPrototypeExec(simpleExpressionRE, line) || ['']; + const { 0: match } = RegExpPrototypeExec(simpleExpressionRE, line) || ['']; if (line.length !== 0 && !match) { completionGroupsLoaded(); return; @@ -1352,11 +1351,11 @@ function complete(line, callback) { if (memberGroups.length) { expr += chaining; - for (const group of memberGroups) { + ArrayPrototypeForEach(memberGroups, (group) => { ArrayPrototypePush(completionGroups, ArrayPrototypeMap(group, (member) => `${expr}${member}`)); - } + }); if (filter) { filter = `${expr}${filter}`; } @@ -1375,7 +1374,7 @@ function complete(line, callback) { // Filter, sort (within each group), uniq and merge the completion groups. if (completionGroups.length && filter) { const newCompletionGroups = []; - for (const group of completionGroups) { + ArrayPrototypeForEach(completionGroups, (group) => { const filteredGroup = ArrayPrototypeFilter( group, (str) => StringPrototypeStartsWith(str, filter) @@ -1383,29 +1382,30 @@ function complete(line, callback) { if (filteredGroup.length) { ArrayPrototypePush(newCompletionGroups, filteredGroup); } - } + }); completionGroups = newCompletionGroups; } const completions = []; // Unique completions across all groups. - const uniqueSet = new SafeSet(['']); + const uniqueSet = new SafeSet(); + uniqueSet.add(''); // Completion group 0 is the "closest" (least far up the inheritance // chain) so we put its completions last: to be closest in the REPL. - for (const group of completionGroups) { + ArrayPrototypeForEach(completionGroups, (group) => { ArrayPrototypeSort(group, (a, b) => (b > a ? 1 : -1)); const setSize = uniqueSet.size; - for (const entry of group) { + ArrayPrototypeForEach(group, (entry) => { if (!uniqueSet.has(entry)) { ArrayPrototypeUnshift(completions, entry); uniqueSet.add(entry); } - } + }); // Add a separator between groups. if (uniqueSet.size !== setSize) { ArrayPrototypeUnshift(completions, ''); } - } + }); // Remove obsolete group entry, if present. if (completions[0] === '') { @@ -1569,14 +1569,13 @@ function defineDefaultCommands(repl) { const longestNameLength = MathMax( ...ArrayPrototypeMap(names, (name) => name.length) ); - for (let n = 0; n < names.length; n++) { - const name = names[n]; + ArrayPrototypeForEach(names, (name) => { const cmd = this.commands[name]; const spaces = StringPrototypeRepeat(' ', longestNameLength - name.length + 3); const line = `.${name}${cmd.help ? spaces + cmd.help : ''}\n`; this.output.write(line); - } + }); this.output.write('\nPress Ctrl+C to abort current expression, ' + 'Ctrl+D to exit the REPL\n'); this.displayPrompt(); diff --git a/test/parallel/test-repl-history-navigation.js b/test/parallel/test-repl-history-navigation.js index fa40ac624000f4..4dd9c350229b19 100644 --- a/test/parallel/test-repl-history-navigation.js +++ b/test/parallel/test-repl-history-navigation.js @@ -504,7 +504,56 @@ const tests = [ prompt, ], clean: true - } + }, + { + env: { NODE_REPL_HISTORY: defaultHistoryPath }, + test: (function*() { + // Deleting Array iterator should not break history feature. + // + // Using a generator function instead of an object to allow the test to + // keep iterating even when Array.prototype[Symbol.iterator] has been + // deleted. + yield 'const ArrayIteratorPrototype ='; + yield ' Object.getPrototypeOf(Array.prototype[Symbol.iterator]());'; + yield ENTER; + yield 'const {next} = ArrayIteratorPrototype;'; + yield ENTER; + yield 'const realArrayIterator = Array.prototype[Symbol.iterator];'; + yield ENTER; + yield 'delete Array.prototype[Symbol.iterator];'; + yield ENTER; + yield 'delete ArrayIteratorPrototype.next;'; + yield ENTER; + yield UP; + yield UP; + yield DOWN; + yield DOWN; + yield 'fu'; + yield 'n'; + yield RIGHT; + yield BACKSPACE; + yield LEFT; + yield LEFT; + yield 'A'; + yield BACKSPACE; + yield GO_TO_END; + yield BACKSPACE; + yield WORD_LEFT; + yield WORD_RIGHT; + yield ESCAPE; + yield ENTER; + yield 'Array.proto'; + yield RIGHT; + yield '.pu'; + yield ENTER; + yield 'ArrayIteratorPrototype.next = next;'; + yield ENTER; + yield 'Array.prototype[Symbol.iterator] = realArrayIterator;'; + yield ENTER; + })(), + expected: [], + clean: false + }, ]; const numtests = tests.length; diff --git a/test/parallel/test-repl-unsafe-array-iteration.js b/test/parallel/test-repl-unsafe-array-iteration.js new file mode 100644 index 00000000000000..3fc65f54cf1f37 --- /dev/null +++ b/test/parallel/test-repl-unsafe-array-iteration.js @@ -0,0 +1,68 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const { spawn } = require('child_process'); + +const replProcess = spawn(process.argv0, ['--interactive'], { + stdio: ['pipe', 'pipe', 'inherit'], + windowsHide: true, +}); + +replProcess.on('error', common.mustNotCall()); + +const replReadyState = (async function* () { + let ready; + const SPACE = ' '.charCodeAt(); + const BRACKET = '>'.charCodeAt(); + const DOT = '.'.charCodeAt(); + replProcess.stdout.on('data', (data) => { + ready = data[data.length - 1] === SPACE && ( + data[data.length - 2] === BRACKET || ( + data[data.length - 2] === DOT && + data[data.length - 3] === DOT && + data[data.length - 4] === DOT + )); + }); + + const processCrashed = new Promise((resolve, reject) => + replProcess.on('exit', reject) + ); + while (true) { + await Promise.race([new Promise(setImmediate), processCrashed]); + if (ready) { + ready = false; + yield; + } + } +})(); +async function writeLn(data, expectedOutput) { + await replReadyState.next(); + if (expectedOutput) { + replProcess.stdout.once('data', common.mustCall((data) => + assert.match(data.toString('utf8'), expectedOutput) + )); + } + await new Promise((resolve, reject) => replProcess.stdin.write( + `${data}\n`, + (err) => (err ? reject(err) : resolve()) + )); +} + +async function main() { + await writeLn( + 'const ArrayIteratorPrototype =' + + ' Object.getPrototypeOf(Array.prototype[Symbol.iterator]());' + ); + await writeLn('delete Array.prototype[Symbol.iterator];'); + await writeLn('delete ArrayIteratorPrototype.next;'); + + await writeLn( + 'for(const x of [3, 2, 1]);', + /Uncaught TypeError: \[3,2,1\] is not iterable/ + ); + await writeLn('.exit'); + + assert(!replProcess.connected); +} + +main().then(common.mustCall());