Skip to content

Commit

Permalink
repl: refactor to avoid unsafe array iteration
Browse files Browse the repository at this point in the history
PR-URL: #36663
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
RaisinTen authored and ruyadorno committed Jan 25, 2021
1 parent 93fc295 commit 9951dae
Show file tree
Hide file tree
Showing 5 changed files with 146 additions and 29 deletions.
2 changes: 1 addition & 1 deletion lib/internal/repl/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
9 changes: 5 additions & 4 deletions lib/internal/util/inspect.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -1869,18 +1870,18 @@ function tryStringify(arg) {
}

function format(...args) {
return formatWithOptionsInternal(undefined, ...args);
return formatWithOptionsInternal(undefined, args);
}

function formatWithOptions(inspectOptions, ...args) {
if (typeof inspectOptions !== 'object' || inspectOptions === null) {
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 = '';
Expand Down
45 changes: 22 additions & 23 deletions lib/repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const {
ArrayPrototypeConcat,
ArrayPrototypeFilter,
ArrayPrototypeFindIndex,
ArrayPrototypeForEach,
ArrayPrototypeIncludes,
ArrayPrototypeJoin,
ArrayPrototypeMap,
Expand Down Expand Up @@ -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 ?
Expand All @@ -673,7 +674,7 @@ function REPLServer(prompt,
} else {
errStack += line;
}
}
});
if (!matched) {
const ln = lines.length === 1 ? ' ' : ':\n';
errStack = `Uncaught${ln}${errStack}`;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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', {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -1259,7 +1258,7 @@ function complete(line, callback) {
}
}
}
}
});
if (group.length) {
ArrayPrototypePush(completionGroups, group);
}
Expand All @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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}`;
}
Expand All @@ -1375,37 +1374,38 @@ 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)
);
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] === '') {
Expand Down Expand Up @@ -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();
Expand Down
51 changes: 50 additions & 1 deletion test/parallel/test-repl-history-navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
68 changes: 68 additions & 0 deletions test/parallel/test-repl-unsafe-array-iteration.js
Original file line number Diff line number Diff line change
@@ -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());

0 comments on commit 9951dae

Please sign in to comment.