repl: fix .load infinite loop caused by shared use of lineEnding RegExp #46742
Conversation
nodejs-github-bot
added
needs-ci
Theo-Steiner
force-pushed
the
fix/repl-infinite-loop
branch
from
f919b51
to
831d193
Compare
|
To be honest, I am not quite sure, why calling |
|
cc @nodejs/repl |
|
#46731 (comment) EDIT: Turns out avoiding using methods that depend on the RegExp prototype was what introduced this bug in the first place. #36593 -> #45614 |
Theo-Steiner
force-pushed
the
fix/repl-infinite-loop
branch
from
7a8ce5f
to
578c040
Compare
Theo-Steiner
changed the title
github-actions
bot
removed
the
request-ci
Yes, I did not manage to reproduce the infinite loop in a test. The problem is that the infinite loop only occurs if the code block starting on this line Line 858 in b85b5ba I've tried passing {terminal: true} to the REPL, but that did not do it either. Do you have an idea for what could be the difference between programmatic and interactive REPL? I won't be able to further investigate this until Monday, I hope that is alright. |
Of course it's alright, take all the time you need! Node.js is striving from free contributions from contributors like you and me, the only expectations we set is to comply to the Code of Conduct, we don't have any expectations regarding the quantity or the rapidity of work :) |
|
I'm not sure if it's going to perform better, but the bug can be resolved with a more minimal diff: diff --git a/lib/internal/readline/interface.js b/lib/internal/readline/interface.js
index 45524612ae..f69d6b14b0 100644
--- a/lib/internal/readline/interface.js
+++ b/lib/internal/readline/interface.js
@@ -1327,9 +1327,11 @@ class Interface extends InterfaceConstructor {
this[kInsertString](StringPrototypeSlice(s, 0, nextMatch.index));
let { lastIndex } = lineEnding;
while ((nextMatch = RegExpPrototypeExec(lineEnding, s)) !== null) {
+ const previousLastIndex = lineEnding.lastIndex;
this[kLine]();
this[kInsertString](StringPrototypeSlice(s, lastIndex, nextMatch.index));
- ({ lastIndex } = lineEnding);
+ lastIndex = previousLastIndex;
+ lineEnding.lastIndex = previousLastIndex;
}
if (lastIndex === s.length) this[kLine]();
} else {When it comes to the repro, you may have more luck with trying to reproduce the bug in |
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.
Theo-Steiner
force-pushed
the
fix/repl-infinite-loop
branch
from
0a4fcbf
to
3cc2379
Compare
Theo-Steiner
force-pushed
the
fix/repl-infinite-loop
branch
from
3cc2379
to
a9010dc
Compare
|
@aduh95 thank you for your advice. I intentionally changed the implementation to use a separate instance of the RegExp, since I thought a similar problem could occur in the future if we fix it by just shuffling the variable declarations around. If keeping diffs small is desirable however, I totally understand and will make the changes as suggested by you. Just let me know how to proceed 🥳 |
cola119
added
the
readline
|
I added a line to reset the RegExp's lastIndex property to 0 before commencing the search. |
github-actions
bot
removed
the
request-ci
cola119
added
the
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/46742
✔ Done loading data for nodejs/node/pull/46742
----------------------------------- PR info ------------------------------------
Title repl: fix .load infinite loop caused by shared use of lineEnding RegExp (#46742)
⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch Theo-Steiner:fix/repl-infinite-loop -> nodejs:main
Labels readline, repl, author ready, needs-ci
Commits 12
- repl: fix .load infinite loop caused by shared use of lineEnding RegExp
- test: add test that fails without the changes of #46742
- chore: remove unrelated comment
- chore: fix linting errors and rename test
- remove magic number from test
- fix: refactor to use module scope instance of RegExp safely for perfo…
- chore: fix linting errors
- docs: update comments
- chore: make finishing touches on comments
- test: fix test failing due to difference in terminal codes
- Revert "test: fix test failing due to difference in terminal codes"
- fix: erase lastIndex state for previous searches
Committers 2
- Theodor Steiner
- GitHub
PR-URL: https://github.com/nodejs/node/pull/46742
Fixes: https://github.com/nodejs/node/issues/46731
Reviewed-By: Kohei Ueno
Reviewed-By: Antoine du Hamel
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46742
Fixes: https://github.com/nodejs/node/issues/46731
Reviewed-By: Kohei Ueno
Reviewed-By: Antoine du Hamel
--------------------------------------------------------------------------------
ℹ This PR was created on Mon, 20 Feb 2023 11:54:47 GMT
✔ Approvals: 2
✔ - Kohei Ueno (@cola119): https://github.com/nodejs/node/pull/46742#pullrequestreview-1319782434
✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/46742#pullrequestreview-1319512073
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2023-03-01T08:33:26Z: https://ci.nodejs.org/job/node-test-pull-request/50125/
ℹ Last Benchmark CI on 2023-02-27T19:56:39Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1299/
- Querying data for job/node-test-pull-request/50125/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ No git cherry-pick in progress
✔ No git am in progress
✔ No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
* branch main -> FETCH_HEAD
✔ origin/main is now up-to-date
- Downloading patch for 46742
From https://github.com/nodejs/node
* branch refs/pull/46742/merge -> FETCH_HEAD
✔ Fetched commits as da0bc6db98ce..395054215dc3
--------------------------------------------------------------------------------
[main b230fffbcf] repl: fix .load infinite loop caused by shared use of lineEnding RegExp
Author: Theodor Steiner
Date: Mon Feb 20 20:47:33 2023 +0900
3 files changed, 66 insertions(+), 10 deletions(-)
create mode 100644 test/parallel/test-repl-load-multiline-function.js
[main 645a7b54da] test: add test that fails without the changes of #46742
Author: Theodor Steiner
Date: Mon Feb 27 17:23:13 2023 +0900
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
[main 12cec6217c] chore: remove unrelated comment
Author: Theodor Steiner
Date: Mon Feb 27 17:29:53 2023 +0900
1 file changed, 1 deletion(-)
[main ac24d34df7] chore: fix linting errors and rename test
Author: Theodor Steiner
Date: Mon Feb 27 17:33:58 2023 +0900
1 file changed, 4 insertions(+), 5 deletions(-)
rename test/parallel/{test-readline-recursive-writes.js => test-readline-interface-recursive-writes.js} (86%)
[main 8029f97d09] remove magic number from test
Author: Theodor Steiner <40017636+Theo-Steiner@users.noreply.github.com>
Date: Tue Feb 28 09:39:32 2023 +0900
1 file changed, 8 insertions(+), 5 deletions(-)
[main 8ec5286f41] fix: refactor to use module scope instance of RegExp safely for performance considerations
Author: Theodor Steiner
Date: Tue Feb 28 15:16:02 2023 +0900
1 file changed, 3 insertions(+), 4 deletions(-)
[main 7871c71f83] chore: fix linting errors
Author: Theodor Steiner
Date: Tue Feb 28 15:17:54 2023 +0900
1 file changed, 1 insertion(+), 2 deletions(-)
[main ace780692a] docs: update comments
Author: Theodor Steiner
Date: Tue Feb 28 15:52:02 2023 +0900
1 file changed, 1 insertion(+), 5 deletions(-)
[main fdbc00369a] chore: make finishing touches on comments
Author: Theodor Steiner <40017636+Theo-Steiner@users.noreply.github.com>
Date: Tue Feb 28 22:55:35 2023 +0900
1 file changed, 3 insertions(+), 3 deletions(-)
[main e8d71d9d6a] test: fix test failing due to difference in terminal codes
Author: Theodor Steiner
Date: Wed Mar 1 11:49:30 2023 +0900
1 file changed, 5 insertions(+), 2 deletions(-)
[main 7e6509411b] Revert "test: fix test failing due to difference in terminal codes"
Author: Theodor Steiner
Date: Wed Mar 1 15:54:34 2023 +0900
1 file changed, 2 insertions(+), 5 deletions(-)
[main 5c29f7d92d] fix: erase lastIndex state for previous searches
Author: Theodor Steiner
Date: Wed Mar 1 15:56:00 2023 +0900
1 file changed, 2 insertions(+)
✔ Patches applied
There are 12 commits in the PR. Attempting autorebase.
Rebasing (2/24)
https://github.com/nodejs/node/actions/runs/4303874198 |
cola119
added
commit-queue-squash
aduh95
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 333aff0 |
|
Thanks a lot for your work here, and congrats for your first merged PR for Node.js 🎉 |
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. PR-URL: #46742 Fixes: #46731 Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
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. PR-URL: #46742 Fixes: #46731 Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
|
I see that this fix is in the v19 changelog, but I don't see it in the v18 changelog. As mentioned earlier, this bug exists starting after 18.12.0. Considering that v18 is currently in active LTS status, can we get the fix in v18 as well please? Related Stack Overflow questions: |
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. PR-URL: #46742 Fixes: #46731 Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
repl: fix .load infinite loop caused by shared use of lineEnding RegExp
Since the lineEnding is declared on the module scope of
lib/internal/readline/interface.js,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 was the root cause of this infinite loop bug when calling .load on javascript files of certain shapes.
Fixes: #46731
I attempted to write a test for this, mimicking
test/parallel/test-repl-load-multiline.js.However, I failed reproducing the infinite loop bug from the programatic REPL, as the problematic indentation preservation is not executed for some reason I have yet to understand. I thought setting
terminal: truemight produce the intended result, but I had no luck with it for now.If anyone has ideas for how to test for this, please let me know!
Edit 4: I found a solution to the core issue and updated this PR accordingly
Edit 5: also found a way to test this