Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Eliminate catastrophic backtracking that hangs Atom #119

Merged
nathansobo merged 6 commits into from
Aug 13, 2017
Merged

Eliminate catastrophic backtracking that hangs Atom #119

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
138a4fb
Add test that reproduces catastrophic backtracking
Aug 12, 2017
6d041f3
Narrow down test case
Aug 12, 2017
22b2ae0
Comment out the cause of the hang
Aug 12, 2017
02c1eb4
Use an atomic group to prevent catastrophic backtracking
Aug 12, 2017
d8afb7b
Simplify test
Aug 12, 2017
d809274
Add spec for atom/atom#14586
Aug 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion grammars/css.cson
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1913,7 +1913,8 @@
'match': '''(?x)
(-?(?!\\d)(?:[\\w-]|[^\\x00-\\x7F]|\\\\(?:[0-9a-fA-F]{1,6}|.))+)
\\s*
(?=[~|^\\]$*=]|/\\*)
Copy link
Copy Markdown
Contributor

@Alhadis Alhadis Aug 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

What the HELL, that quantifier shouldn't be there. :| Oops.

@nathansobo Try removing $*, that's a zero-length match that'll match anything. Really bad news.

As in, it should just be $, not $*.

Copy link
Copy Markdown
Contributor

@Alhadis Alhadis Aug 12, 2017

Choose a reason for hiding this comment

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

In fact, I bet dollars to donuts that's what's causing the regression.

Copy link
Copy Markdown
Contributor Author

@nathansobo nathansobo Aug 12, 2017

Choose a reason for hiding this comment

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

Thanks for your prompt reply. Watching my daughter so I'll give it a try later. Thanks again.

Copy link
Copy Markdown
Contributor

@winstliu winstliu Aug 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Removing the asterisk does not fix the issue. EDIT: That's because it's in the character class, so it's interpreted literally.

Copy link
Copy Markdown
Contributor

@winstliu winstliu Aug 12, 2017

Choose a reason for hiding this comment

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

About to eat, but changing (?:[\\w-]...) to an atomic group (?>[\\w-]...) works. Didn't run specs though.

Copy link
Copy Markdown
Contributor

@Alhadis Alhadis Aug 12, 2017

Choose a reason for hiding this comment

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

@nathansobo I'll wait for you (or somebody else) to confirm this before attempting a different solution. Trying to tie up loose ends elsewhere ATM.

Copy link
Copy Markdown
Contributor Author

@nathansobo nathansobo Aug 12, 2017

Choose a reason for hiding this comment

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

Removing the * after the $ didn't seem to help. 😞

# Uncommenting the following line causes the test case to hang
# (?=[~|^\\]$*=]|/\\*)
'''
}
]
Expand Down
13 changes: 13 additions & 0 deletions spec/css-spec.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3520,3 +3520,16 @@ describe 'CSS grammar', ->
"""
for line in invalid.split /\n/
expect(grammar.firstLineRegex.scanner.findNextMatchSync(line)).toBeNull()

describe "performance regressions", ->
it "does not hang on invalid input preceding an equals sign", ->
cssGrammar = atom.grammars.grammarForScopeName('source.css')
grammar = atom.grammars.grammarForScopeName('text.html.basic')

# This works fine
cssGrammar.tokenizeLine('<![CDATA[啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊"')

# This hangs
start = Date.now()
cssGrammar.tokenizeLine('<![CDATA[啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊啊"=')
expect(Date.now() - start).toBeLessThan(5000)