473914 - Crash due to too much recursion in nsCSSScanner::Next with many adjacent comments

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

I'll attach a whitespace-ignoring diff of nsCSSScanner.cpp momentarily.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, the whitespace-ignoring diff of nsCSSScanner.cpp (with less context, thanks to interdiff) is just:

--- a/layout/style/nsCSSScanner.cpp
+++ b/layout/style/nsCSSScanner.cpp
@@ -709,2 +709,3 @@
 {
+  for (;;) { // Infinite loop so we can restart after comments.
   PRInt32 ch = Read();
@@ -778,3 +779,4 @@
 #endif
-      return SkipCComment() && Next(aToken);
+        SkipCComment();
+        continue; // start again at the beginning
     }
@@ -834,2 +836,3 @@
 }
+}

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 357434 [details] [diff] [review]
patch

You need a similar change in NextURL(), right?

Also, shouldn't that look like:

  if (!SkipCComment()) {
    return PR_FALSE;
  }
  continue;

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #3)
> You need a similar change in NextURL(), right?

Er, yes.  I have NextURL massively modified in my tree, so that code isn't there anymore.

> Also, shouldn't that look like:
> 
>   if (!SkipCComment()) {
>     return PR_FALSE;
>   }
>   continue;

Good point.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: remove bogus comment handling in NextURL

This removes the bogus comment handling in NextURL that only did anything at the beginning of the URL or after whitespace, and was also broken since it recurred into Next rather than into NextURL.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: patch

Updated to comments.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, and the latter, with interdiff -w, is now:

--- a/layout/style/nsCSSScanner.cpp
+++ b/layout/style/nsCSSScanner.cpp
@@ -709,2 +709,3 @@
 {
+  for (;;) { // Infinite loop so we can restart after comments.
   PRInt32 ch = Read();
@@ -778,3 +779,6 @@
 #endif
-      return SkipCComment() && Next(aToken);
+        if (!SkipCComment()) {
+          return PR_FALSE;
+        }
+        continue; // start again at the beginning
     }
@@ -834,2 +838,3 @@
 }
+}

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fixed:
http://hg.mozilla.org/mozilla-central/rev/e2c5c612df46
http://hg.mozilla.org/mozilla-central/rev/fefdf894b7ec

Comment 10

[Mats Palmgren (inactive)]

The crash fix is worth taking on branches?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, since it's not exploitable and not something anybody is likely to hit due to anything other than artificially-constructed testcases.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

per comment 11, not blocking 3.5 release; dunno if we want to get it in a .x release or not.

Comment 13

[Carsten Book [:Tomcat]]

http://crash-stats.mozilla.com/report/index/e38b500d-d50b-468e-a77e-3bf8e2090924?p=1 for 3.5.3 - from loading http://www.hixie.ch/tests/adhoc/css/parsing/core-syntax/comments/001.html can we get this into the 1.9.1 branch ?

Comment 14

[Samuel Sidler (old account; do not CC)]

This is wanted for 1.9.1, but we won't block on it. If someone requests approval on a patch that applies, we'll likely take it.