Skip to content
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

readline: fix issue with newline-less last line #47317

Merged
merged 4 commits into from
Apr 14, 2023

Conversation

harrisi
Copy link
Contributor

@harrisi harrisi commented Mar 30, 2023

The logic for reading lines was slightly flawed, in that it assumed there would be a final new line. It handled the case where there are no new lines, but this then broke if there were some new lines.

The fix in logic is basically removing the special case where there are no new lines by changing it to always read the final line with no new lines. This works because if a file contains no new lines, the final line is the first line, and all is well.

There is some subtlety in this functioning, however. If the last line contains no new lines, then lastIndex will be the start of the last line, and kInsertString will be called from that point. If it does contain a new line, lastIndex will be equal to s.length, so the slice will be the empty string.

Fixes: #47305

The logic for reading lines was slightly flawed, in that it assumed
there would be a final new line. It handled the case where there are no
new lines, but this then broke if there were some new lines.

The fix in logic is basically removing the special case where there are
no new lines by changing it to always read the final line with no new
lines. This works because if a file contains no new lines, the final
line is the first line, and all is well.

There is some subtlety in this functioning, however. If the last line
contains no new lines, then `lastIndex` will be the start of the last
line, and `kInsertString` will be called from that point. If it does
contain a new line, `lastIndex` will be equal to `s.length`, so the
slice will be the empty string.

Fixes: nodejs#47305
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. readline Issues and PRs related to the built-in readline module. labels Mar 30, 2023
@harrisi
Copy link
Contributor Author

harrisi commented Mar 30, 2023

I haven't included a test because I'm not sure what it should be. Since I found this after the fix to the previous infinite loop bug I found (#46731), I'm hesitant to just copy and paste the multi-line test and remove the trailing newline.

I also am about to go home for the night but wanted to get this up so nobody wastes their time trying to figure out #47305, if this does actually fix it.

Further, with the amount of issues around this, I'm not sure if someone wants a closer eye on what's happening and why these bugs are getting through.

@aduh95
Copy link
Contributor

aduh95 commented Apr 1, 2023

A test is definitely needed here, otherwise the issue is going to come back eventually.

The repl test probably isn't strictly necessary, but this area's tests
has been lacking sufficiently varying cases which have caused a few bugs
to be missed, so it seems reasonable to include.
@harrisi
Copy link
Contributor Author

harrisi commented Apr 2, 2023

I added a repl test which is virtually identical to the multiline test that already existed. It might be overkill, but I figure just in case. The readline test checks the minimal issue I posted previously. Both tests fail without the update to readline/interface.js.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM if tests are passing. Two comments:

  • Could you add a comment in test/fixtures/repl-load-multiline-no-trailing-newline.js that the missing EOL is on purpose (sure it's in the name of the file, but that can easily be missed).
  • Can you run the linter to fix all the reported style nits?

@harrisi harrisi requested a review from aduh95 April 2, 2023 20:06
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 2, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 2, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@cola119 cola119 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@Theo-Steiner Theo-Steiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think the fixed code is more readable than the original, while still correctly keeping track of the state of the RegExp.
LGTM! 🥳

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 14, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 14, 2023
@nodejs-github-bot nodejs-github-bot merged commit 9decb70 into nodejs:main Apr 14, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 9decb70

targos pushed a commit that referenced this pull request May 2, 2023
The logic for reading lines was slightly flawed, in that it assumed
there would be a final new line. It handled the case where there are no
new lines, but this then broke if there were some new lines.

The fix in logic is basically removing the special case where there are
no new lines by changing it to always read the final line with no new
lines. This works because if a file contains no new lines, the final
line is the first line, and all is well.

There is some subtlety in this functioning, however. If the last line
contains no new lines, then `lastIndex` will be the start of the last
line, and `kInsertString` will be called from that point. If it does
contain a new line, `lastIndex` will be equal to `s.length`, so the
slice will be the empty string.

Fixes: #47305
PR-URL: #47317
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos mentioned this pull request May 2, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
The logic for reading lines was slightly flawed, in that it assumed
there would be a final new line. It handled the case where there are no
new lines, but this then broke if there were some new lines.

The fix in logic is basically removing the special case where there are
no new lines by changing it to always read the final line with no new
lines. This works because if a file contains no new lines, the final
line is the first line, and all is well.

There is some subtlety in this functioning, however. If the last line
contains no new lines, then `lastIndex` will be the start of the last
line, and `kInsertString` will be called from that point. If it does
contain a new line, `lastIndex` will be equal to `s.length`, so the
slice will be the empty string.

Fixes: #47305
PR-URL: #47317
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
The logic for reading lines was slightly flawed, in that it assumed
there would be a final new line. It handled the case where there are no
new lines, but this then broke if there were some new lines.

The fix in logic is basically removing the special case where there are
no new lines by changing it to always read the final line with no new
lines. This works because if a file contains no new lines, the final
line is the first line, and all is well.

There is some subtlety in this functioning, however. If the last line
contains no new lines, then `lastIndex` will be the start of the last
line, and `kInsertString` will be called from that point. If it does
contain a new line, `lastIndex` will be equal to `s.length`, so the
slice will be the empty string.

Fixes: nodejs#47305
PR-URL: nodejs#47317
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.load ignores last line of file when it doesn't include a newline.
5 participants