Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repl: refactor to avoid unsafe array iteration #36663

Closed
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];
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
// 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);
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
}

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());