From 2b0871d009a869110a41f8b663bc38209d649600 Mon Sep 17 00:00:00 2001 From: Theodor Steiner Date: Mon, 20 Feb 2023 20:47:33 +0900 Subject: [PATCH 01/12] repl: fix .load infinite loop caused by shared use of lineEnding RegExp Since the lineEnding Regular Expression is declared on the module scope, recursive invocations of its `[kTtyWrite]` method share one instance of this Regular Expression. Since the state of a RegExp is managed by instance, alternately calling RegExpPrototypeExec with the same RegExp on different strings can lead to the state changing unexpectedly. This is the root cause of this infinite loop bug when calling .load on javascript files of certain shapes. --- lib/internal/readline/interface.js | 28 +++++++---- lib/repl.js | 1 + .../test-repl-load-multiline-function.js | 47 +++++++++++++++++++ 3 files changed, 66 insertions(+), 10 deletions(-) create mode 100644 test/parallel/test-repl-load-multiline-function.js diff --git a/lib/internal/readline/interface.js b/lib/internal/readline/interface.js index 45524612aea37b..75843fe96251ec 100644 --- a/lib/internal/readline/interface.js +++ b/lib/internal/readline/interface.js @@ -20,6 +20,7 @@ const { MathMaxApply, NumberIsFinite, ObjectSetPrototypeOf, + RegExp, RegExpPrototypeExec, StringPrototypeCodePointAt, StringPrototypeEndsWith, @@ -72,7 +73,7 @@ const kHistorySize = 30; const kMaxUndoRedoStackSize = 2048; const kMincrlfDelay = 100; // \r\n, \n, or \r followed by something other than \n -const lineEnding = /\r?\n|\r(?!\n)/g; +const lineEndingPattern = '\r?\n|\r(?!\n)'; const kLineObjectStream = Symbol('line object stream'); const kQuestionCancel = Symbol('kQuestionCancel'); @@ -585,6 +586,7 @@ class Interface extends InterfaceConstructor { } // Run test() on the new string chunk, not on the entire line buffer. + const lineEnding = new RegExp(lineEndingPattern, 'g'); let newPartContainsEnding = RegExpPrototypeExec(lineEnding, string); if (newPartContainsEnding !== null) { if (this[kLine_buffer]) { @@ -1322,18 +1324,24 @@ class Interface extends InterfaceConstructor { // falls through default: if (typeof s === 'string' && s) { + /** + * Use Regular Expression scoped to this block, as lastIndex and the state for RegExpPrototypeExec + * will be overwritten if the same RegEx instance is reused in recursive function calls. + */ + const lineEnding = new RegExp(lineEndingPattern, 'g'); let nextMatch = RegExpPrototypeExec(lineEnding, s); - if (nextMatch !== null) { - this[kInsertString](StringPrototypeSlice(s, 0, nextMatch.index)); - let { lastIndex } = lineEnding; - while ((nextMatch = RegExpPrototypeExec(lineEnding, s)) !== null) { - this[kLine](); + + // If no line endings are found, just insert the string as is + if (nextMatch === null) { + this[kInsertString](s); + } else { + // Keep track of the end of the last match + let lastIndex = 0; + do { this[kInsertString](StringPrototypeSlice(s, lastIndex, nextMatch.index)); + this[kLine](); ({ lastIndex } = lineEnding); - } - if (lastIndex === s.length) this[kLine](); - } else { - this[kInsertString](s); + } while ((nextMatch = RegExpPrototypeExec(lineEnding, s)) !== null); } } } diff --git a/lib/repl.js b/lib/repl.js index 241e25f0f2095a..b4322ea23154f2 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -861,6 +861,7 @@ function REPLServer(prompt, // code alignment const matches = self._sawKeyPress ? RegExpPrototypeExec(/^\s+/, cmd) : null; + // Preserve indentation in editorMode if (matches) { const prefix = matches[0]; self.write(prefix); diff --git a/test/parallel/test-repl-load-multiline-function.js b/test/parallel/test-repl-load-multiline-function.js new file mode 100644 index 00000000000000..055cd1cf2fd954 --- /dev/null +++ b/test/parallel/test-repl-load-multiline-function.js @@ -0,0 +1,47 @@ +'use strict'; +const common = require('../common'); +const ArrayStream = require('../common/arraystream'); +const assert = require('assert'); +const join = require('path').join; +const fs = require('fs'); + +common.skipIfDumbTerminal(); + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +const terminalCode = '(\u001b[1G\u001b[0J \u001b[1G)'; +const terminalCodeRegex = new RegExp(terminalCode.replace(/\[/g, '\\['), 'g'); + +const repl = require('repl'); + +const inputStream = new ArrayStream(); +const outputStream = new ArrayStream(); + +const r = repl.start({ + prompt: '', + input: inputStream, + output: outputStream, + terminal: true, + useColors: false +}); + +const testFile = 'function a(b) {\n return b }\na(1)\n'; +const testFileName = join(tmpdir.path, 'foo.js'); +fs.writeFileSync(testFileName, testFile); + +const command = `.load ${testFileName}\n`; +let accum = ''; +outputStream.write = (data) => accum += data.replace('\r', ''); + + +r.write(command); + +const expected = command + +'function a(b) {\n' + +' return b }\n' + +'a(1)\n' + +'\n' + +'1\n'; +assert.strictEqual(accum.replace(terminalCodeRegex, ''), expected); +r.close(); From f5c31901d7510ba02966a6dd8c4cbfb5944a5c26 Mon Sep 17 00:00:00 2001 From: Theodor Steiner Date: Mon, 27 Feb 2023 17:23:13 +0900 Subject: [PATCH 02/12] test: add test that fails without the changes of #46742 --- .../test-readline-recursive-writes.js | 31 ++++++++++++ .../test-repl-load-multiline-function.js | 47 ------------------- 2 files changed, 31 insertions(+), 47 deletions(-) create mode 100644 test/parallel/test-readline-recursive-writes.js delete mode 100644 test/parallel/test-repl-load-multiline-function.js diff --git a/test/parallel/test-readline-recursive-writes.js b/test/parallel/test-readline-recursive-writes.js new file mode 100644 index 00000000000000..21813ab4b89520 --- /dev/null +++ b/test/parallel/test-readline-recursive-writes.js @@ -0,0 +1,31 @@ +'use strict'; +const common = require('../common'); +const ArrayStream = require('../common/arraystream'); +const assert = require('assert'); + +common.skipIfDumbTerminal(); + +const readline = require('readline'); +const rli = new readline.Interface({ + terminal: true, + input: new ArrayStream(), +}); + +let recursionDepth = 0; + +rli.on('line', function onLine() { + // Abort in case of infinite loop + if (recursionDepth > 2) { + return + } + recursionDepth++ + // Write something recursively to readline + rli.write('foo'); + return; +}); + +// minimal reproduction for #46731 +const testInput = ' \n}\n'; +rli.write(testInput); + +assert.strictEqual(recursionDepth, testInput.match(/\n/g).length, "infinite loop"); diff --git a/test/parallel/test-repl-load-multiline-function.js b/test/parallel/test-repl-load-multiline-function.js deleted file mode 100644 index 055cd1cf2fd954..00000000000000 --- a/test/parallel/test-repl-load-multiline-function.js +++ /dev/null @@ -1,47 +0,0 @@ -'use strict'; -const common = require('../common'); -const ArrayStream = require('../common/arraystream'); -const assert = require('assert'); -const join = require('path').join; -const fs = require('fs'); - -common.skipIfDumbTerminal(); - -const tmpdir = require('../common/tmpdir'); -tmpdir.refresh(); - -const terminalCode = '(\u001b[1G\u001b[0J \u001b[1G)'; -const terminalCodeRegex = new RegExp(terminalCode.replace(/\[/g, '\\['), 'g'); - -const repl = require('repl'); - -const inputStream = new ArrayStream(); -const outputStream = new ArrayStream(); - -const r = repl.start({ - prompt: '', - input: inputStream, - output: outputStream, - terminal: true, - useColors: false -}); - -const testFile = 'function a(b) {\n return b }\na(1)\n'; -const testFileName = join(tmpdir.path, 'foo.js'); -fs.writeFileSync(testFileName, testFile); - -const command = `.load ${testFileName}\n`; -let accum = ''; -outputStream.write = (data) => accum += data.replace('\r', ''); - - -r.write(command); - -const expected = command + -'function a(b) {\n' + -' return b }\n' + -'a(1)\n' + -'\n' + -'1\n'; -assert.strictEqual(accum.replace(terminalCodeRegex, ''), expected); -r.close(); From a9010dcc9bd359f4e176fbf52a1eedfd3f61703c Mon Sep 17 00:00:00 2001 From: Theodor Steiner Date: Mon, 27 Feb 2023 17:29:53 +0900 Subject: [PATCH 03/12] chore: remove unrelated comment --- lib/repl.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/repl.js b/lib/repl.js index b4322ea23154f2..241e25f0f2095a 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -861,7 +861,6 @@ function REPLServer(prompt, // code alignment const matches = self._sawKeyPress ? RegExpPrototypeExec(/^\s+/, cmd) : null; - // Preserve indentation in editorMode if (matches) { const prefix = matches[0]; self.write(prefix); From 6797e756ad06b5ee5a49e0469202de2ea6f183b8 Mon Sep 17 00:00:00 2001 From: Theodor Steiner Date: Mon, 27 Feb 2023 17:33:58 +0900 Subject: [PATCH 04/12] chore: fix linting errors and rename test --- ...es.js => test-readline-interface-recursive-writes.js} | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) rename test/parallel/{test-readline-recursive-writes.js => test-readline-interface-recursive-writes.js} (86%) diff --git a/test/parallel/test-readline-recursive-writes.js b/test/parallel/test-readline-interface-recursive-writes.js similarity index 86% rename from test/parallel/test-readline-recursive-writes.js rename to test/parallel/test-readline-interface-recursive-writes.js index 21813ab4b89520..e11d0b5b3d2157 100644 --- a/test/parallel/test-readline-recursive-writes.js +++ b/test/parallel/test-readline-interface-recursive-writes.js @@ -16,16 +16,15 @@ let recursionDepth = 0; rli.on('line', function onLine() { // Abort in case of infinite loop if (recursionDepth > 2) { - return + return; } - recursionDepth++ + recursionDepth++; // Write something recursively to readline rli.write('foo'); - return; }); -// minimal reproduction for #46731 +// Minimal reproduction for #46731 const testInput = ' \n}\n'; rli.write(testInput); -assert.strictEqual(recursionDepth, testInput.match(/\n/g).length, "infinite loop"); +assert.strictEqual(recursionDepth, testInput.match(/\n/g).length); From ba290729612ec1c1528400379e395e562d303494 Mon Sep 17 00:00:00 2001 From: Theodor Steiner <40017636+Theo-Steiner@users.noreply.github.com> Date: Tue, 28 Feb 2023 09:39:32 +0900 Subject: [PATCH 05/12] remove magic number from test Co-authored-by: Antoine du Hamel --- .../test-readline-interface-recursive-writes.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-readline-interface-recursive-writes.js b/test/parallel/test-readline-interface-recursive-writes.js index e11d0b5b3d2157..3a0aee5be9d619 100644 --- a/test/parallel/test-readline-interface-recursive-writes.js +++ b/test/parallel/test-readline-interface-recursive-writes.js @@ -13,9 +13,13 @@ const rli = new readline.Interface({ let recursionDepth = 0; -rli.on('line', function onLine() { +// Minimal reproduction for #46731 +const testInput = ' \n}\n'; +const numberOfExpectedLines = testInput.match(/\n/g).length; + +rli.on('line', () => { // Abort in case of infinite loop - if (recursionDepth > 2) { + if (recursionDepth > numberOfExpectedLines) { return; } recursionDepth++; @@ -23,8 +27,7 @@ rli.on('line', function onLine() { rli.write('foo'); }); -// Minimal reproduction for #46731 -const testInput = ' \n}\n'; + rli.write(testInput); -assert.strictEqual(recursionDepth, testInput.match(/\n/g).length); +assert.strictEqual(recursionDepth, numberOfExpectedLines); From ced509760ab83c424329992eeb07fcf5a6bd4ab1 Mon Sep 17 00:00:00 2001 From: Theodor Steiner Date: Tue, 28 Feb 2023 15:16:02 +0900 Subject: [PATCH 06/12] fix: refactor to use module scope instance of RegExp safely for performance considerations --- lib/internal/readline/interface.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/internal/readline/interface.js b/lib/internal/readline/interface.js index 75843fe96251ec..4d2d70c303042d 100644 --- a/lib/internal/readline/interface.js +++ b/lib/internal/readline/interface.js @@ -73,7 +73,7 @@ const kHistorySize = 30; const kMaxUndoRedoStackSize = 2048; const kMincrlfDelay = 100; // \r\n, \n, or \r followed by something other than \n -const lineEndingPattern = '\r?\n|\r(?!\n)'; +const lineEnding = /\r?\n|\r(?!\n)/g; const kLineObjectStream = Symbol('line object stream'); const kQuestionCancel = Symbol('kQuestionCancel'); @@ -586,7 +586,6 @@ class Interface extends InterfaceConstructor { } // Run test() on the new string chunk, not on the entire line buffer. - const lineEnding = new RegExp(lineEndingPattern, 'g'); let newPartContainsEnding = RegExpPrototypeExec(lineEnding, string); if (newPartContainsEnding !== null) { if (this[kLine_buffer]) { @@ -1328,7 +1327,6 @@ class Interface extends InterfaceConstructor { * Use Regular Expression scoped to this block, as lastIndex and the state for RegExpPrototypeExec * will be overwritten if the same RegEx instance is reused in recursive function calls. */ - const lineEnding = new RegExp(lineEndingPattern, 'g'); let nextMatch = RegExpPrototypeExec(lineEnding, s); // If no line endings are found, just insert the string as is @@ -1339,8 +1337,9 @@ class Interface extends InterfaceConstructor { let lastIndex = 0; do { this[kInsertString](StringPrototypeSlice(s, lastIndex, nextMatch.index)); - this[kLine](); ({ lastIndex } = lineEnding); + this[kLine](); + lineEnding.lastIndex = lastIndex } while ((nextMatch = RegExpPrototypeExec(lineEnding, s)) !== null); } } From 24a0fb6f337ac58570a5a537e051404d9e08bd7a Mon Sep 17 00:00:00 2001 From: Theodor Steiner Date: Tue, 28 Feb 2023 15:17:54 +0900 Subject: [PATCH 07/12] chore: fix linting errors --- lib/internal/readline/interface.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/internal/readline/interface.js b/lib/internal/readline/interface.js index 4d2d70c303042d..e2f57c20813600 100644 --- a/lib/internal/readline/interface.js +++ b/lib/internal/readline/interface.js @@ -20,7 +20,6 @@ const { MathMaxApply, NumberIsFinite, ObjectSetPrototypeOf, - RegExp, RegExpPrototypeExec, StringPrototypeCodePointAt, StringPrototypeEndsWith, @@ -1339,7 +1338,7 @@ class Interface extends InterfaceConstructor { this[kInsertString](StringPrototypeSlice(s, lastIndex, nextMatch.index)); ({ lastIndex } = lineEnding); this[kLine](); - lineEnding.lastIndex = lastIndex + lineEnding.lastIndex = lastIndex; } while ((nextMatch = RegExpPrototypeExec(lineEnding, s)) !== null); } } From d002103213f02f5326b31479020c7315b8033a19 Mon Sep 17 00:00:00 2001 From: Theodor Steiner Date: Tue, 28 Feb 2023 15:52:02 +0900 Subject: [PATCH 08/12] docs: update comments --- lib/internal/readline/interface.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/internal/readline/interface.js b/lib/internal/readline/interface.js index e2f57c20813600..b20b28f874769d 100644 --- a/lib/internal/readline/interface.js +++ b/lib/internal/readline/interface.js @@ -1322,12 +1322,7 @@ class Interface extends InterfaceConstructor { // falls through default: if (typeof s === 'string' && s) { - /** - * Use Regular Expression scoped to this block, as lastIndex and the state for RegExpPrototypeExec - * will be overwritten if the same RegEx instance is reused in recursive function calls. - */ let nextMatch = RegExpPrototypeExec(lineEnding, s); - // If no line endings are found, just insert the string as is if (nextMatch === null) { this[kInsertString](s); @@ -1338,6 +1333,7 @@ class Interface extends InterfaceConstructor { this[kInsertString](StringPrototypeSlice(s, lastIndex, nextMatch.index)); ({ lastIndex } = lineEnding); this[kLine](); + // Restore lastIndex locally as the RegExp's state might have been overwritten by recursive calls lineEnding.lastIndex = lastIndex; } while ((nextMatch = RegExpPrototypeExec(lineEnding, s)) !== null); } From 1cc6e4e814f2c88071b44bdb46120b06acd76b77 Mon Sep 17 00:00:00 2001 From: Theodor Steiner <40017636+Theo-Steiner@users.noreply.github.com> Date: Tue, 28 Feb 2023 22:55:35 +0900 Subject: [PATCH 09/12] chore: make finishing touches on comments Co-authored-by: Antoine du Hamel --- lib/internal/readline/interface.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/internal/readline/interface.js b/lib/internal/readline/interface.js index b20b28f874769d..e14fe88c216dfe 100644 --- a/lib/internal/readline/interface.js +++ b/lib/internal/readline/interface.js @@ -1323,17 +1323,17 @@ class Interface extends InterfaceConstructor { default: if (typeof s === 'string' && s) { let nextMatch = RegExpPrototypeExec(lineEnding, s); - // If no line endings are found, just insert the string as is + // If no line endings are found, just insert the string as is. if (nextMatch === null) { this[kInsertString](s); } else { - // Keep track of the end of the last match + // Keep track of the end of the last match. let lastIndex = 0; do { this[kInsertString](StringPrototypeSlice(s, lastIndex, nextMatch.index)); ({ lastIndex } = lineEnding); this[kLine](); - // Restore lastIndex locally as the RegExp's state might have been overwritten by recursive calls + // Restore lastIndex as the call to kLine could have mutated it. lineEnding.lastIndex = lastIndex; } while ((nextMatch = RegExpPrototypeExec(lineEnding, s)) !== null); } From 6699359c7c15b92256c1c3c9813bd50f05a5606f Mon Sep 17 00:00:00 2001 From: Theodor Steiner Date: Wed, 1 Mar 2023 11:49:30 +0900 Subject: [PATCH 10/12] test: fix test failing due to difference in terminal codes --- test/parallel/test-repl-load-multiline.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-repl-load-multiline.js b/test/parallel/test-repl-load-multiline.js index 4fcf206bef1be1..092f049ab00a31 100644 --- a/test/parallel/test-repl-load-multiline.js +++ b/test/parallel/test-repl-load-multiline.js @@ -8,8 +8,11 @@ const repl = require('repl'); common.skipIfDumbTerminal(); const command = `.load ${fixtures.path('repl-load-multiline.js')}`; -const terminalCode = '\u001b[1G\u001b[0J \u001b[1G'; -const terminalCodeRegex = new RegExp(terminalCode.replace(/\[/g, '\\['), 'g'); + +// \u001b[nG - Move the cursor to nth column. +// \u001b[0J - Clear from cursor to end of screen. +/* eslint-disable-next-line no-control-regex */ +const terminalCodeRegex = /(\u001b\[0J|\u001b\[\d+G) ?/g; const expected = `${command} const getLunch = () => From d8d6375b729b6f50045ea297a8a3f105c39c0c0b Mon Sep 17 00:00:00 2001 From: Theodor Steiner Date: Wed, 1 Mar 2023 15:54:34 +0900 Subject: [PATCH 11/12] Revert "test: fix test failing due to difference in terminal codes" This reverts commit 6699359c7c15b92256c1c3c9813bd50f05a5606f. --- test/parallel/test-repl-load-multiline.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-repl-load-multiline.js b/test/parallel/test-repl-load-multiline.js index 092f049ab00a31..4fcf206bef1be1 100644 --- a/test/parallel/test-repl-load-multiline.js +++ b/test/parallel/test-repl-load-multiline.js @@ -8,11 +8,8 @@ const repl = require('repl'); common.skipIfDumbTerminal(); const command = `.load ${fixtures.path('repl-load-multiline.js')}`; - -// \u001b[nG - Move the cursor to nth column. -// \u001b[0J - Clear from cursor to end of screen. -/* eslint-disable-next-line no-control-regex */ -const terminalCodeRegex = /(\u001b\[0J|\u001b\[\d+G) ?/g; +const terminalCode = '\u001b[1G\u001b[0J \u001b[1G'; +const terminalCodeRegex = new RegExp(terminalCode.replace(/\[/g, '\\['), 'g'); const expected = `${command} const getLunch = () => From 395054215dc3b053a17ae3955a8cb4902940ad0e Mon Sep 17 00:00:00 2001 From: Theodor Steiner Date: Wed, 1 Mar 2023 15:56:00 +0900 Subject: [PATCH 12/12] fix: erase lastIndex state for previous searches --- lib/internal/readline/interface.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/internal/readline/interface.js b/lib/internal/readline/interface.js index e14fe88c216dfe..4a5ec4973695fa 100644 --- a/lib/internal/readline/interface.js +++ b/lib/internal/readline/interface.js @@ -1322,6 +1322,8 @@ class Interface extends InterfaceConstructor { // falls through default: if (typeof s === 'string' && s) { + // Erase state of previous searches. + lineEnding.lastIndex = 0; let nextMatch = RegExpPrototypeExec(lineEnding, s); // If no line endings are found, just insert the string as is. if (nextMatch === null) {