repl: refactor to avoid unsafe array iteration

PR-URL: #36663
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

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;

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,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 = '';

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,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] === '') {
@@ -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();

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;
 

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