-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: fix .load infinite loop caused by shared use of lineEnding RegExp #46742
repl: fix .load infinite loop caused by shared use of lineEnding RegExp #46742
Conversation
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 |
7a8ce5f
to
578c040
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for investigating that and sending a PR. It seems that the added test already passes on main
, which makes me think that it's not able to reproduce the issue. Could you have a look?
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.
0a4fcbf
to
3cc2379
Compare
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 🥳 |
I added a line to reset the RegExp's lastIndex property to 0 before commencing the search. |
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 |
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: true
might 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