Skip to content

Commit a48e2d6

Browse files
RaisinTentargos
authored andcommitted
repl: refactor to avoid unsafe array iteration
PR-URL: #36663 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 20ffadf commit a48e2d6

File tree

5 files changed

+143
-27
lines changed

5 files changed

+143
-27
lines changed

lib/internal/repl/utils.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) {
245245
}
246246

247247
// Result and the text that was completed.
248-
const [rawCompletions, completeOn] = data;
248+
const { 0: rawCompletions, 1: completeOn } = data;
249249

250250
if (!rawCompletions || rawCompletions.length === 0) {
251251
return;

lib/internal/util/inspect.js

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,8 @@ function inspect(value, opts) {
296296
ctx.showHidden = opts;
297297
} else if (opts) {
298298
const optKeys = ObjectKeys(opts);
299-
for (const key of optKeys) {
299+
for (let i = 0; i < optKeys.length; ++i) {
300+
const key = optKeys[i];
300301
// TODO(BridgeAR): Find a solution what to do about stylize. Either make
301302
// this function public or add a new API with a similar or better
302303
// functionality.
@@ -1847,18 +1848,18 @@ function tryStringify(arg) {
18471848
}
18481849

18491850
function format(...args) {
1850-
return formatWithOptionsInternal(undefined, ...args);
1851+
return formatWithOptionsInternal(undefined, args);
18511852
}
18521853

18531854
function formatWithOptions(inspectOptions, ...args) {
18541855
if (typeof inspectOptions !== 'object' || inspectOptions === null) {
18551856
throw new ERR_INVALID_ARG_TYPE(
18561857
'inspectOptions', 'object', inspectOptions);
18571858
}
1858-
return formatWithOptionsInternal(inspectOptions, ...args);
1859+
return formatWithOptionsInternal(inspectOptions, args);
18591860
}
18601861

1861-
function formatWithOptionsInternal(inspectOptions, ...args) {
1862+
function formatWithOptionsInternal(inspectOptions, args) {
18621863
const first = args[0];
18631864
let a = 0;
18641865
let str = '';

lib/repl.js

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -675,7 +675,7 @@ function REPLServer(prompt,
675675
let matched = false;
676676

677677
errStack = '';
678-
for (const line of lines) {
678+
ArrayPrototypeForEach(lines, (line) => {
679679
if (!matched &&
680680
RegExpPrototypeTest(/^\[?([A-Z][a-z0-9_]*)*Error/, line)) {
681681
errStack += writer.options.breakLength >= line.length ?
@@ -685,7 +685,7 @@ function REPLServer(prompt,
685685
} else {
686686
errStack += line;
687687
}
688-
}
688+
});
689689
if (!matched) {
690690
const ln = lines.length === 1 ? ' ' : ':\n';
691691
errStack = `Uncaught${ln}${errStack}`;
@@ -780,9 +780,7 @@ function REPLServer(prompt,
780780
const prioritizedSigintQueue = new SafeSet();
781781
self.on('SIGINT', function onSigInt() {
782782
if (prioritizedSigintQueue.size > 0) {
783-
for (const task of prioritizedSigintQueue) {
784-
task();
785-
}
783+
ArrayPrototypeForEach(prioritizedSigintQueue, (task) => task());
786784
return;
787785
}
788786

@@ -1265,7 +1263,7 @@ function complete(line, callback) {
12651263
paths = ArrayPrototypeConcat(module.paths, CJSModule.globalPaths);
12661264
}
12671265

1268-
for (let dir of paths) {
1266+
ArrayPrototypeForEach(paths, (dir) => {
12691267
dir = path.resolve(dir, subdir);
12701268
const dirents = gracefulReaddir(dir, { withFileTypes: true }) || [];
12711269
for (const dirent of dirents) {
@@ -1293,7 +1291,7 @@ function complete(line, callback) {
12931291
}
12941292
}
12951293
}
1296-
}
1294+
});
12971295
if (group.length) {
12981296
ArrayPrototypePush(completionGroups, group);
12991297
}
@@ -1303,7 +1301,7 @@ function complete(line, callback) {
13031301
}
13041302
} else if (RegExpPrototypeTest(fsAutoCompleteRE, line) &&
13051303
this.allowBlockingCompletions) {
1306-
[completionGroups, completeOn] = completeFSFunctions(line);
1304+
({ 0: completionGroups, 1: completeOn } = completeFSFunctions(line));
13071305
// Handle variable member lookup.
13081306
// We support simple chained expressions like the following (no function
13091307
// calls, etc.). That is for simplicity and also because we *eval* that
@@ -1316,7 +1314,7 @@ function complete(line, callback) {
13161314
// foo.<|> # completions for 'foo' with filter ''
13171315
} else if (line.length === 0 ||
13181316
RegExpPrototypeTest(/\w|\.|\$/, line[line.length - 1])) {
1319-
const [match] = RegExpPrototypeExec(simpleExpressionRE, line) || [''];
1317+
const { 0: match } = RegExpPrototypeExec(simpleExpressionRE, line) || [''];
13201318
if (line.length !== 0 && !match) {
13211319
completionGroupsLoaded();
13221320
return;
@@ -1386,11 +1384,11 @@ function complete(line, callback) {
13861384

13871385
if (memberGroups.length) {
13881386
expr += chaining;
1389-
for (const group of memberGroups) {
1387+
ArrayPrototypeForEach(memberGroups, (group) => {
13901388
ArrayPrototypePush(completionGroups,
13911389
ArrayPrototypeMap(group,
13921390
(member) => `${expr}${member}`));
1393-
}
1391+
});
13941392
if (filter) {
13951393
filter = `${expr}${filter}`;
13961394
}
@@ -1409,37 +1407,38 @@ function complete(line, callback) {
14091407
// Filter, sort (within each group), uniq and merge the completion groups.
14101408
if (completionGroups.length && filter) {
14111409
const newCompletionGroups = [];
1412-
for (const group of completionGroups) {
1410+
ArrayPrototypeForEach(completionGroups, (group) => {
14131411
const filteredGroup = ArrayPrototypeFilter(
14141412
group,
14151413
(str) => StringPrototypeStartsWith(str, filter)
14161414
);
14171415
if (filteredGroup.length) {
14181416
ArrayPrototypePush(newCompletionGroups, filteredGroup);
14191417
}
1420-
}
1418+
});
14211419
completionGroups = newCompletionGroups;
14221420
}
14231421

14241422
const completions = [];
14251423
// Unique completions across all groups.
1426-
const uniqueSet = new SafeSet(['']);
1424+
const uniqueSet = new SafeSet();
1425+
uniqueSet.add('');
14271426
// Completion group 0 is the "closest" (least far up the inheritance
14281427
// chain) so we put its completions last: to be closest in the REPL.
1429-
for (const group of completionGroups) {
1428+
ArrayPrototypeForEach(completionGroups, (group) => {
14301429
ArrayPrototypeSort(group, (a, b) => (b > a ? 1 : -1));
14311430
const setSize = uniqueSet.size;
1432-
for (const entry of group) {
1431+
ArrayPrototypeForEach(group, (entry) => {
14331432
if (!uniqueSet.has(entry)) {
14341433
ArrayPrototypeUnshift(completions, entry);
14351434
uniqueSet.add(entry);
14361435
}
1437-
}
1436+
});
14381437
// Add a separator between groups.
14391438
if (uniqueSet.size !== setSize) {
14401439
ArrayPrototypeUnshift(completions, '');
14411440
}
1442-
}
1441+
});
14431442

14441443
// Remove obsolete group entry, if present.
14451444
if (completions[0] === '') {
@@ -1608,14 +1607,13 @@ function defineDefaultCommands(repl) {
16081607
const longestNameLength = MathMax(
16091608
...ArrayPrototypeMap(names, (name) => name.length)
16101609
);
1611-
for (let n = 0; n < names.length; n++) {
1612-
const name = names[n];
1610+
ArrayPrototypeForEach(names, (name) => {
16131611
const cmd = this.commands[name];
16141612
const spaces =
16151613
StringPrototypeRepeat(' ', longestNameLength - name.length + 3);
16161614
const line = `.${name}${cmd.help ? spaces + cmd.help : ''}\n`;
16171615
this.output.write(line);
1618-
}
1616+
});
16191617
this.output.write('\nPress Ctrl+C to abort current expression, ' +
16201618
'Ctrl+D to exit the REPL\n');
16211619
this.displayPrompt();

test/parallel/test-repl-history-navigation.js

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -504,7 +504,56 @@ const tests = [
504504
prompt,
505505
],
506506
clean: true
507-
}
507+
},
508+
{
509+
env: { NODE_REPL_HISTORY: defaultHistoryPath },
510+
test: (function*() {
511+
// Deleting Array iterator should not break history feature.
512+
//
513+
// Using a generator function instead of an object to allow the test to
514+
// keep iterating even when Array.prototype[Symbol.iterator] has been
515+
// deleted.
516+
yield 'const ArrayIteratorPrototype =';
517+
yield ' Object.getPrototypeOf(Array.prototype[Symbol.iterator]());';
518+
yield ENTER;
519+
yield 'const {next} = ArrayIteratorPrototype;';
520+
yield ENTER;
521+
yield 'const realArrayIterator = Array.prototype[Symbol.iterator];';
522+
yield ENTER;
523+
yield 'delete Array.prototype[Symbol.iterator];';
524+
yield ENTER;
525+
yield 'delete ArrayIteratorPrototype.next;';
526+
yield ENTER;
527+
yield UP;
528+
yield UP;
529+
yield DOWN;
530+
yield DOWN;
531+
yield 'fu';
532+
yield 'n';
533+
yield RIGHT;
534+
yield BACKSPACE;
535+
yield LEFT;
536+
yield LEFT;
537+
yield 'A';
538+
yield BACKSPACE;
539+
yield GO_TO_END;
540+
yield BACKSPACE;
541+
yield WORD_LEFT;
542+
yield WORD_RIGHT;
543+
yield ESCAPE;
544+
yield ENTER;
545+
yield 'Array.proto';
546+
yield RIGHT;
547+
yield '.pu';
548+
yield ENTER;
549+
yield 'ArrayIteratorPrototype.next = next;';
550+
yield ENTER;
551+
yield 'Array.prototype[Symbol.iterator] = realArrayIterator;';
552+
yield ENTER;
553+
})(),
554+
expected: [],
555+
clean: false
556+
},
508557
];
509558
const numtests = tests.length;
510559

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
const { spawn } = require('child_process');
5+
6+
const replProcess = spawn(process.argv0, ['--interactive'], {
7+
stdio: ['pipe', 'pipe', 'inherit'],
8+
windowsHide: true,
9+
});
10+
11+
replProcess.on('error', common.mustNotCall());
12+
13+
const replReadyState = (async function* () {
14+
let ready;
15+
const SPACE = ' '.charCodeAt();
16+
const BRACKET = '>'.charCodeAt();
17+
const DOT = '.'.charCodeAt();
18+
replProcess.stdout.on('data', (data) => {
19+
ready = data[data.length - 1] === SPACE && (
20+
data[data.length - 2] === BRACKET || (
21+
data[data.length - 2] === DOT &&
22+
data[data.length - 3] === DOT &&
23+
data[data.length - 4] === DOT
24+
));
25+
});
26+
27+
const processCrashed = new Promise((resolve, reject) =>
28+
replProcess.on('exit', reject)
29+
);
30+
while (true) {
31+
await Promise.race([new Promise(setImmediate), processCrashed]);
32+
if (ready) {
33+
ready = false;
34+
yield;
35+
}
36+
}
37+
})();
38+
async function writeLn(data, expectedOutput) {
39+
await replReadyState.next();
40+
if (expectedOutput) {
41+
replProcess.stdout.once('data', common.mustCall((data) =>
42+
assert.match(data.toString('utf8'), expectedOutput)
43+
));
44+
}
45+
await new Promise((resolve, reject) => replProcess.stdin.write(
46+
`${data}\n`,
47+
(err) => (err ? reject(err) : resolve())
48+
));
49+
}
50+
51+
async function main() {
52+
await writeLn(
53+
'const ArrayIteratorPrototype =' +
54+
' Object.getPrototypeOf(Array.prototype[Symbol.iterator]());'
55+
);
56+
await writeLn('delete Array.prototype[Symbol.iterator];');
57+
await writeLn('delete ArrayIteratorPrototype.next;');
58+
59+
await writeLn(
60+
'for(const x of [3, 2, 1]);',
61+
/Uncaught TypeError: \[3,2,1\] is not iterable/
62+
);
63+
await writeLn('.exit');
64+
65+
assert(!replProcess.connected);
66+
}
67+
68+
main().then(common.mustCall());

0 commit comments

Comments
 (0)