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
Fix: catastrophic backtracking in astUtils linebreak regex (fixes #7893) #7898
Conversation
This fixes an issue where `astUtils.getLocationFromRangeIndex` and `astUtils.getRangeIndexFromLocation` were using a regular expression susceptible to catastrophic backtracking. The match would take quadratic time in the length of the last line of the file. Since the file in #7893 contains a 1.5 million character source map URL on the last line, rules like `no-multiple-empty-lines` would hang when using ast-utils to split the file into lines. This issue only applies to files without trailing newlines, and is only noticable when the last line of the file contains more than 30000 characters or so. Since only a few rules use these `astUtils` functions, this would only appear when either `no-useless-escape` or `no-multiple-empty-lines` reports an error for the file. Simplified example: Node 7.4.0 hangs when evaluating this expression. ```js /[^\n]*\n/.test('A'.repeat(1000000)) ```
LGTM |
@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @kaicataldo and @alangpierce to be potential reviewers. |
@@ -309,6 +309,11 @@ ruleTester.run("no-multiple-empty-lines", rule, { | |||
errors: [getExpectedError(1)], | |||
options: [{ max: 1 }], | |||
parserOptions: { ecmaVersion: 6 } | |||
}, | |||
{ |
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.
Perhaps we could link to the related issue here, to make sure we don't forget about it?
@@ -290,8 +290,13 @@ const lineIndexCache = new WeakMap(); | |||
function getLineIndices(sourceCode) { | |||
|
|||
if (!lineIndexCache.has(sourceCode)) { | |||
const lineIndices = (sourceCode.text.match(/[^\r\n\u2028\u2029]*(\r\n|\r|\n|\u2028|\u2029)/g) || []) | |||
.reduce((indices, line) => indices.concat(indices[indices.length - 1] + line.length), [0]); | |||
const lineIndices = [0]; |
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.
Can you add a comment here explaining why it's done this way? I can see some well-meaning dev thinking they can simplify this and reverting accidentally.
LGTM |
What is the purpose of this pull request? (put an "X" next to item)
[x] Bug fix (see #7893)
What changes did you make? (Give an overview)
This fixes an issue where
astUtils.getLocationFromRangeIndex
andastUtils.getRangeIndexFromLocation
were using a regular expression susceptible to catastrophic backtracking. The match would take quadratic time in the length of the last line of the file. Since the file in #7893 contains a 1.5 million character source map URL on the last line, rules likeno-multiple-empty-lines
would hang when using ast-utils to split the file into lines.This issue only applies to files without trailing newlines, and is only noticable when the last line of the file contains more than 30000 characters or so. Since only a few rules use these
astUtils
functions, this would only appear when eitherno-useless-escape
orno-multiple-empty-lines
reports an error for the file.Simplified example: Node 7.4.0 hangs when evaluating this expression.
Is there anything you'd like reviewers to focus on?
Nothing in particular