bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

[Colin]

Hi,


I have the following snippet in example.scss file:

```

/*
#ffffff #ffffff
#ffffff #ffffff
#ffffff #ffffff
#ffffff #ffffff
#ffffff #ffffff
*/
```

On opening the file with emacs -Q (27.1 and a fresh build of git master 
(29.0.50), emacs hangs.

If I remove one/two lines, it appears to burn some CPU and then work, so 
it looks like an exponential search or something.

For now I'll remove the snippet from my code, but an interesting bug 
nonetheless ;)

bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

[Eli Zaretskii]

> Date: Wed, 12 Jan 2022 17:20:24 +1000
> From: Colin <my.old.email.sucked@gmail.com>
> 
> I have the following snippet in example.scss file:
> 
> ```
> 
> /*
> #ffffff #ffffff
> #ffffff #ffffff
> #ffffff #ffffff
> #ffffff #ffffff
> #ffffff #ffffff
> */
> ```
> 
> On opening the file with emacs -Q (27.1 and a fresh build of git master 
> (29.0.50), emacs hangs.
> 
> If I remove one/two lines, it appears to burn some CPU and then work, so 
> it looks like an exponential search or something.
> 
> For now I'll remove the snippet from my code, but an interesting bug 
> nonetheless ;)

It seems to infloop in JIT font-lock, and the culprit seems to be this
part of font-lock-keywords:

       ;; Even though pseudo-elements should be prefixed by ::, a
       ;; single colon is accepted for backward compatibility.
       "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
                                       css-pseudo-element-ids)
                               t)

bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> It seems to infloop in JIT font-lock, and the culprit seems to be this
> part of font-lock-keywords:
>
>        ;; Even though pseudo-elements should be prefixed by ::, a
>        ;; single colon is accepted for backward compatibility.
>        "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
>                                        css-pseudo-element-ids)
>                                t)

Trying to understand the regexp used for scss here, I think that bit is
somewhat innocuous -- it just matches those words.

       ;; Even though pseudo-elements should be prefixed by ::, a
       ;; single colon is accepted for backward compatibility.
       "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
                                       css-pseudo-element-ids)
                               t)
       "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)"

But then we get:

       "\\(?:([^)]+)\\)?"
       (if (not sassy)
           "[^:{}()\n]*"
         (concat "[^:{}()\n#]*\\(?:" scss--hash-re "[^:{}()\n#]*\\)*"))
       "\\)*"

Which is a whole lot of backtracking, presumably exacerbated by the
previous ids bit of the regexp.

But I've repressed all I once knew about the scss language -- what is it
really trying to match here?  Anybody?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Colin <my.old.email.sucked@gmail.com>,  53203@debbugs.gnu.org
> Date: Thu, 13 Jan 2022 08:00:18 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It seems to infloop in JIT font-lock, and the culprit seems to be this
> > part of font-lock-keywords:
> >
> >        ;; Even though pseudo-elements should be prefixed by ::, a
> >        ;; single colon is accepted for backward compatibility.
> >        "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
> >                                        css-pseudo-element-ids)
> >                                t)
> 
> Trying to understand the regexp used for scss here, I think that bit is
> somewhat innocuous -- it just matches those words.
> 
>        ;; Even though pseudo-elements should be prefixed by ::, a
>        ;; single colon is accepted for backward compatibility.
>        "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
>                                        css-pseudo-element-ids)
>                                t)
>        "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)"

I posted that because I saw this regexp in the backtrace obtained by
interrupting the infloop.

bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

[Simen Heggestøyl]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But then we get:
>
>        "\\(?:([^)]+)\\)?"
>        (if (not sassy)
>            "[^:{}()\n]*"
>          (concat "[^:{}()\n#]*\\(?:" scss--hash-re "[^:{}()\n#]*\\)*"))
>        "\\)*"
>
> Which is a whole lot of backtracking, presumably exacerbated by the
> previous ids bit of the regexp.

By the ancient scientific method of commenting out code, it seems to me
that the culprit is rather the last group of the regexp starting on
line 955:

  (concat "\\(?:" scss--hash-re
          "\\|[^@/:{}() \t\n#]\\)"
          "[^:{}()#]*\\(?:" scss--hash-re "[^:{}()#]*\\)*"))

That is, this part:

  \\(?:" scss--hash-re "[^:{}()#]*\\)*

Though I'm rather clueless on how proceed debugging/optimizing it. 😕

> But I've repressed all I once knew about the scss language -- what is
> it really trying to match here?  Anybody?

It's supposed to match selectors like the one on line 39 in
test/manual/indent/scss-mode.scss:

  p.#{$name} var

bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

[Lars Ingebrigtsen]

Simen Heggestøyl <simenheg@runbox.com> writes:

>> But I've repressed all I once knew about the scss language -- what is
>> it really trying to match here?  Anybody?
>
> It's supposed to match selectors like the one on line 39 in
> test/manual/indent/scss-mode.scss:
>
>   p.#{$name} var

Ah, I see -- it wants to match a selector followed by braces.  I.e.,
these things:

p.#{$name} var
{
}

p.#{$name}, f.#{$bar}:active, f.bar::after {
}

p.#{$name} f.#{$bar} k.var #{$bar}  #{$bar}
{
}

p.#{$name}
{
}

Hm...


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

[Lars Ingebrigtsen]

OK, I think I understand most of the regexp now, but it's a strange mix
of strictness and sloppiness.

    (,(concat
       "^[ \t]*\\("
       (if (not sassy)
           ;; We don't allow / as first char, so as not to
           ;; take a comment as the beginning of a selector.
           "[^@/:{}() \t\n][^:{}()]*"
         ;; Same as for non-sassy except we do want to allow { and }
         ;; chars in selectors in the case of #{$foo}
         ;; variable interpolation!
         (concat "\\(?:" scss--hash-re
                 "\\|[^@/:{}() \t\n#]\\)"
                 "[^:{}()#]*\\(?:" scss--hash-re "[^:{}()#]*\\)*"))
       ;; Even though pseudo-elements should be prefixed by ::, a
       ;; single colon is accepted for backward compatibility.
       "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
                                       css-pseudo-element-ids)
                               t)
       "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)"
       "\\(?:([^)]+)\\)?"

First we work really hard to see whether we have something that looks
like a selector, i.e. a line that starts with,

p
p::after
p:active

or in the scss case

p#{foo}:active


and then we allow any kind of junk for the rest of the line, like

p.a *[line noise]* p.b {
}

with this bit:

     (if (not sassy)
           "[^:{}()\n]*"
         (concat "[^:{}()\n#]*\\(?:" scss--hash-re "[^:{}()\n#]*\\)*"))
       "\\)*"
       "\\)\\(?:\n[ \t]*\\)*{")
     (1 'css-selector keep))


Anyway, I still don't grok this bit:

         (concat "\\(?:" scss--hash-re
                 "\\|[^@/:{}() \t\n#]\\)"
                 "[^:{}()#]*\\(?:" scss--hash-re "[^:{}()#]*\\)*"))

That is, we're matching an scss/css selector first, and then...
possibly another one?  Which we're doing later, anyway.

So just getting rid of that bit seems to lead to the same highlighting
(and gets rid of the original reported problem).  But like I said, I
don't understand quite what it's trying to do there.  Can anybody come
up with an scss example this breaks?

diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 1139fd1976..fd47a7b83b 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -953,14 +953,13 @@ css--font-lock-keywords
          ;; chars in selectors in the case of #{$foo}
          ;; variable interpolation!
          (concat "\\(?:" scss--hash-re
-                 "\\|[^@/:{}() \t\n#]\\)"
-                 "[^:{}()#]*\\(?:" scss--hash-re "[^:{}()#]*\\)*"))
+                 "\\|[^@/:{}() \t\n][^:{}()]\\)*"))
        ;; Even though pseudo-elements should be prefixed by ::, a
        ;; single colon is accepted for backward compatibility.
        "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
                                        css-pseudo-element-ids)
                                t)
-       "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)"
+       "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)?"
        "\\(?:([^)]+)\\)?"
        (if (not sassy)
            "[^:{}()\n]*"



-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

[Lars Ingebrigtsen]

But how about just trying to make it stricter?  That is, just allow any
number of selectors, with the separators between.  The following seems
to work fine in the test cases.

diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 1139fd1976..4db3ae82f0 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -944,29 +944,29 @@ css--font-lock-keywords
     ;; I use `keep' this should work better).  But really the part of the
     ;; selector between [...] should simply not be highlighted.
     (,(concat
-       "^[ \t]*\\("
+       "^[ \t]*\\(\\(?:"
        (if (not sassy)
            ;; We don't allow / as first char, so as not to
            ;; take a comment as the beginning of a selector.
-           "[^@/:{}() \t\n][^:{}()]*"
+           "[[:alnum:]%*#.>]+"
          ;; Same as for non-sassy except we do want to allow { and }
          ;; chars in selectors in the case of #{$foo}
          ;; variable interpolation!
          (concat "\\(?:" scss--hash-re
-                 "\\|[^@/:{}() \t\n#]\\)"
-                 "[^:{}()#]*\\(?:" scss--hash-re "[^:{}()#]*\\)*"))
+                 "\\|[[:alnum:]%*#.>]+\\)"))
        ;; Even though pseudo-elements should be prefixed by ::, a
        ;; single colon is accepted for backward compatibility.
        "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
                                        css-pseudo-element-ids)
                                t)
-       "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)"
+       "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)\\)?"
+       ;; Braces after selectors.
+       "\\(?:\\[[^]]+\\]\\)?"
+       ;; Parentheses after selectors.
        "\\(?:([^)]+)\\)?"
-       (if (not sassy)
-           "[^:{}()\n]*"
-         (concat "[^:{}()\n#]*\\(?:" scss--hash-re "[^:{}()\n#]*\\)*"))
-       "\\)*"
-       "\\)\\(?:\n[ \t]*\\)*{")
+       ;; Separators between selectors.
+       "[ \t,+~>]*"
+       "\\)+\\)\\(?:\n[ \t]*\\)*{")
      (1 'css-selector keep))
     ;; In the above rule, we allow the open-brace to be on some subsequent
     ;; line.  This will only work if we properly mark the intervening text


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But how about just trying to make it stricter?  That is, just allow any
> number of selectors, with the separators between.  The following seems
> to work fine in the test cases.

I've now done something like this in Emacs 29, and added a bunch of tests.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

[Simen Heggestøyl]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> But how about just trying to make it stricter?  That is, just allow any
>> number of selectors, with the separators between.  The following seems
>> to work fine in the test cases.
>
> I've now done something like this in Emacs 29, and added a bunch of tests.

Hm, I can't seem to find it, did you push it yet?

I tested a bit with the patch you posted 15:30 yesterday, but it seems
to have some problems:

Pasting this into a CSS mode buffer now freezes up Emacs:

  /*
   * xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
   */

Selectors with hyphens are no longer highlighted, e.g.:

  .foo-bar {

Only the last part of multi line selectors are now highlighted,
e.g. only `body` here:

  div,
  body {

bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

[Lars Ingebrigtsen]

Simen Heggestøyl <simenheg@runbox.com> writes:

> Hm, I can't seem to find it, did you push it yet?

Nope; forgot to, and it seems like a good thing.  :-)

> I tested a bit with the patch you posted 15:30 yesterday, but it seems
> to have some problems:
>
> Pasting this into a CSS mode buffer now freezes up Emacs:
>
>   /*
>    * xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>    */
>
> Selectors with hyphens are no longer highlighted, e.g.:
>
>   .foo-bar {
>
> Only the last part of multi line selectors are now highlighted,
> e.g. only `body` here:
>
>   div,
>   body {

I've now fixed these and pushed the change.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode

[Simen Heggestøyl]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Simen Heggestøyl <simenheg@runbox.com> writes:
>
>> I tested a bit with the patch you posted 15:30 yesterday, but it seems
>> to have some problems:
>>
>> Pasting this into a CSS mode buffer now freezes up Emacs:
>>
>>   /*
>>    * xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>>    */
>>
>> Selectors with hyphens are no longer highlighted, e.g.:
>>
>>   .foo-bar {
>>
>> Only the last part of multi line selectors are now highlighted,
>> e.g. only `body` here:
>>
>>   div,
>>   body {
>
> I've now fixed these and pushed the change.

Thanks! Looks good.

The new selector tests makes it much less scary to tweak the regexp.