Closed Bug 333634 Opened 18 years ago Closed 18 years ago

Stack overflow involving CSS selectors

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Version:
1.8 Branch
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: security, Assigned: dbaron)

References

(
URL
)

Details

(Keywords: crash, fixed1.8.1, Whiteboard: [sg:dos])

Attachments

(3 files, 1 obsolete file)

winxp trunk stack
7.39 KB, text/plain
Details
get rid of recursive operations on CSS selector structures
23.02 KB, patch
Details | Diff | Splinter Review
fix leak in previous patch
8.35 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
bzbarsky
: approval-branch-1.8.1+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

I have found a stack overflow in the latest version of Mozilla Firefox that seems to be caused by loading a large font array inside of a stylesheet element. The issue is not present on Mac OSX Intel, but does crash FF on every windows platform I have tested.

Reproducible: Always

Steps to Reproduce:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML>
<HEAD>
<TITLE> New Document </TITLE>
</HEAD>

<BODY>


<script>

var buffer = 'a,';

for (i=0; i < 85750; i++)
{
	buffer += 'g,';
}

document.write('<style>.'+buffer+' { font-family: '+buffer+'; }</style>');

</script>



</BODY>
</HTML>

Actual Results:  
The example above will crash the browser
"seems to be caused by loading a large font array inside of a stylesheet element"

After looking at this issue for more than a few minutes I realize the problem is with the large number of "tags" defined in the style sheet element and has nothing to do with the font array. But, I am sure you already see that :)
Attached file winxp trunk stack — 
looks like Bug 279678
> winxp trunk stack
> 
> looks like Bug 279678

Really? I get a stack overflow (as described) with a repeating bunch of nsCSSSelectorList routines on the stack. Quite unlike the js-engine stuff in 279678
Assignee: nobody → dbaron
Status: UNCONFIRMED → NEW
Component: Security → Style System (CSS)
Ever confirmed: true
Product: Firefox → Core
Whiteboard: [sg:dos]
Version: unspecified → 1.8 Branch
There seems to be a few unusual things going on here. It seems like different results can be achieved by specifying different formats of selectors and selector classes references. For example, in my tests the following script will not crash the browser immediately, but will upon refreshing the page.

<script language="JavaScript" type="text/javascript">

var buffer = 'a';

for (i=0; i < 100000; i++)
{
	buffer += ',a';
}

document.write('<style>'+buffer+' {}</style>');

</script>

This kind of behavior almost makes me think that there are possibly two different, but very similar bugs present? Has anyone found anything yet in the source tree that seems to be the cause?
Also Confirmed in the latest version of Mozilla Thunderbird. Of course the large selector would have to be written out, and not generated with Javascript.
This also gets rid of a good bit of unused code.
Attachment #219718 - Flags: superreview?(bzbarsky)
Attachment #219718 - Flags: review?(bzbarsky)
Attachment #219718 - Flags: approval-branch-1.8.1?
Comment on attachment 219718 [details] [diff] [review]
get rid of recursive operations on CSS selector structures

>Index: nsICSSStyleRule.h
> struct nsAtomList {
>+  /**
>+   * Do a deep clone.  Should be used only on the first in the linked list.
>+   *
>+   * aDeep == PR_FALSE is for internal use only.
>+   */
>+  nsAtomList* Clone(PRBool aDeep = PR_TRUE) const;

I think I'd prefer something like:

  public:
    nsAtomList* Clone() const {
      return Clone(PR_TRUE);
    }
  protected:
   nsAtomList* Clone(PRBool aDeep) const;

instead of relying purely on comments for the "internal use only" thing, if possible.  Same elsewhere.

r+sr=bzbarsky with that.
Attachment #219718 - Flags: superreview?(bzbarsky)
Attachment #219718 - Flags: superreview+
Attachment #219718 - Flags: review?(bzbarsky)
Attachment #219718 - Flags: review+
Attachment #219718 - Flags: approval-branch-1.8.1?
Attachment #219718 - Flags: approval-branch-1.8.1+
Patch addressing bz's comments.
Attachment #219718 - Attachment is obsolete: true
Checked in to trunk and MOZILLA_1_8_BRANCH.
Status: NEW → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
This patch leaks with chains of negations because moving the cloning of negations outside of the aDeep check means they get double-cloned.

        Attachment #220142 -
        Flags: superreview?(bzbarsky)

        Attachment #220142 -
        Flags: review?(bzbarsky)

        Attachment #220142 -
        Flags: approval-branch-1.8.1?(bzbarsky)

    
    

    
  
Comment on attachment 220142 [details] [diff] [review]
fix leak in previous patch

>Index: nsICSSStyleRule.h
>   // No need to worry about multiple levels of recursion (or about copying
>   // mNegations->mNext) since an mNegations can't have an mNext.

Is this comment still relevant?

r+sr+a=bzbarsky

        Attachment #220142 -
        Flags: superreview?(bzbarsky)

        Attachment #220142 -
        Flags: superreview+

        Attachment #220142 -
        Flags: review?(bzbarsky)

        Attachment #220142 -
        Flags: review+

        Attachment #220142 -
        Flags: approval-branch-1.8.1?(bzbarsky)

        Attachment #220142 -
        Flags: approval-branch-1.8.1+

    
    

    
  
(In reply to comment #12)
> Is this comment still relevant?

Yes, except for the parenthetical.

    
    

    
  
Leak fix checked in to trunk and MOZILLA_1_8_BRANCH.

    
    

    
  
Should this remain security-sensitive despite just being a crash?  (e.g. because it affects Thunderbird?)
Severity: normal → critical
Keywords: crash

    
  
Summary: Firefox Stack Overflow → Stack overflow involving CSS selectors
Flags: blocking1.8.0.8?
Group: security
Flags: blocking1.8.0.8? → blocking1.8.0.8-

          You need to log in
          before you can comment on or make changes to this bug.