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

Infinite loop for illegal tokens when ignore_illegals or highlightAuto #2522

Closed
wooorm opened this issue May 2, 2020 · 6 comments · Fixed by #2524
Closed

Infinite loop for illegal tokens when ignore_illegals or highlightAuto #2522

wooorm opened this issue May 2, 2020 · 6 comments · Fixed by #2524
Assignees
Labels
bug hotfix bad bug, should fix and release ASAP parser
Milestone

Comments

@wooorm
Copy link

wooorm commented May 2, 2020

On 10.0.1, the following XML:

<?xml version="1.0"?>
<response value="ok" xml:lang="en">
  <text>Ok</text>
  <comment html_allowed="true"/>
  <ns1:description><![CDATA[
  CDATA is <not> magical.
  ]]></ns1:description>
  <a></a> <a/>
</response>

When passed through highlightAuto or highlight with ignore_illegals turned on, will cause an infinite loop, specifically, because of '$' as an illegal in CSS selector attributes:

illegal: '$',

@joshgoebel
Copy link
Member

Oh yikes, that's quite a find.

@joshgoebel
Copy link
Member

@wooorm Please review fix. Do you think my catch-all is overkill? I really think we've covered everything now with being/end 0 width detection plus illegal 0 width detection. Am I just being paranoid adding a third check?

I just think freezes like this a VERY VERY bad things.

@egor-rogov

@wooorm
Copy link
Author

wooorm commented May 2, 2020

Lgtm. Loop check is very heavy, but may be good indeed.

Weirdly enough, this used to work in 9.16 (or at least in the lowlight clone since when it started). I’m not sure what changed: i looked a bit through git blame and the ‘m’ flag isn’t new, and I didn’t see anything else that was weird 🤷‍♂️

@joshgoebel
Copy link
Member

Loop check is very heavy, but may be good indeed.

Heavy handed perhaps. I'm not sure it's that CPU heavy. I run a few test iterations and couldn't see a measurable difference with the typical marge of changes I usually see.

Oh it could probably be faster if we just started iterations with a negative # and then compared without the multiplication... but now I think i'm micro-optimizing without a real problem. :)

@joshgoebel
Copy link
Member

Weirdly enough, this used to work in 9.16

Yeah, not worth it to figure out why... something prolly changed, but 0 width matches with illegal is definitely an edge case that needs to be handled to make sure the cursor advances.

@joshgoebel joshgoebel added bug hotfix bad bug, should fix and release ASAP parser labels May 2, 2020
@joshgoebel joshgoebel self-assigned this May 2, 2020
@joshgoebel joshgoebel added this to the 10.0.2 milestone May 2, 2020
@wooorm
Copy link
Author

wooorm commented May 2, 2020

Totes! I wasn’t worried about the performance, more code wise heavy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug hotfix bad bug, should fix and release ASAP parser
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants