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

(coffeescript) Freezing, poorly behaved regex to match regex. #2375

Closed
Labels
bug hotfix bad bug, should fix and release ASAP language

Comments

@xforce
Copy link

xforce commented Jan 31, 2020

Tested in NodeJS 13 and Chrome 79 as well as v8 master.

Input text is around 5k.

In the attached file is a reproduction of a complete freeze when trying to highlight with coffeescript, this also affects auto language detection.
(Complete freeze in this case means, I killed the process after 10 minutes)

From debugging this a bit, it hangs in the exec of the coffeescript regex
var match = matcherRe.exec(s);

I feel like it shouldn't hang and since this hang happens in v8 native code, I am not entirely sure this
is actually an issue with highlightjs, so feel free to close this if appropriate.

Is it expected behavior for a browser to hang in such a case or abort after some amount of time? I am not entirely sure, would appreciate some info on this.

I was thinking about filing a but report on the v8 bug tracker but decided to hold off until I get some feedback here first.

test.txt

@joshgoebel
Copy link
Member

joshgoebel commented Jan 31, 2020

Is this an actual real-life test case or is this from an attack or? More context, please? Obviously that's a pretty convoluted test-case.

@joshgoebel
Copy link
Member

An attachment of just the code itself might also be helpful.

@joshgoebel
Copy link
Member

Is it expected behavior for a browser to hang in such a case or abort after some amount of time? I am not entirely sure, would appreciate some info on this.

I'd say this is either a bug in Highlight.js or a poorly behaved regex combined with worst-case sample data.

@xforce
Copy link
Author

xforce commented Jan 31, 2020

Yes this was a real life case, we have chat application where we use highlight.js for syntax highlighting of code blocks in chat messages and someone pasted the output of some software snapshot creation, which is just a rather long base64 string.

The test.txt attach in the report contains nodejs code that when run will hang.

@joshgoebel
Copy link
Member

The more complex regex match is at fault:

{
          // regex can't start with space to parse x / 2 / 3 as two divisions
          // regex can't start with *, and it supports an "illegal" in the main mode
          begin: /\/(?![ *])(\\\/|.)*?\/[gim]*(?=\W)/
        }

@joshgoebel joshgoebel added bug help welcome Could use help from community language labels Jan 31, 2020
@joshgoebel
Copy link
Member

joshgoebel commented Jan 31, 2020

@xforce It should complete if you give it long enough, though it's hard to say how long that might be. :-) I shortened the sample significantly and it only takes a second or two here in my testing. The problem is the regex is backtracking poorly I think.

I think this is comparable without the problem:

/\/(?![ *]).*?(?![\\]).\/[gim]*(?=\W)/

@egor-rogov Thoughts? That doesn't handle // but we already have a regex for that.

@joshgoebel joshgoebel added the hotfix bad bug, should fix and release ASAP label Jan 31, 2020
@joshgoebel joshgoebel changed the title Freeze in coffeescript highlight. (coffeescript) Freezing, poorly behaved regex to match regex. Jan 31, 2020
@joshgoebel
Copy link
Member

Creating a new "hotfix" category and tagging accordingly. Since this is a freeze/lockup issue I plan to back-port this to the current release series also (9.18).

@joshgoebel
Copy link
Member

@xforce If you're in a hurry you could hotfix your code with the PR I link to here... I'm waiting for @egor-rogov to review it.

It looks like this is a 6+ year old bug though, so can't just go back to a prior version or something.

@xforce
Copy link
Author

xforce commented Jan 31, 2020

@yyyc514 Thanks for the quick fix, I will be applying the PR later today and see how it goes.

@joshgoebel joshgoebel removed the help welcome Could use help from community label Feb 1, 2020
@joshgoebel
Copy link
Member

9.18.1 and 9-18-stable branch now includes this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment