Bug 83545 - Stack overflow in CSS parser caused by recursive stylesheet import
Summary: Stack overflow in CSS parser caused by recursive stylesheet import
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Barr
URL: http://www.image-in-nation.com/templa...
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-09 20:59 PDT by David Barr
Modified: 2012-04-16 23:27 PDT (History)
12 users (show)

See Also:


Attachments
Patch (1.47 KB, patch)
2012-04-09 21:57 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch (3.55 KB, patch)
2012-04-10 17:27 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch (3.49 KB, patch)
2012-04-10 17:42 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch (3.56 KB, patch)
2012-04-15 17:25 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch (3.57 KB, patch)
2012-04-15 17:42 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch (3.56 KB, patch)
2012-04-15 18:23 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch for landing (3.54 KB, patch)
2012-04-15 20:21 PDT, David Barr
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Barr 2012-04-09 20:59:04 PDT
This stack overflow was identified by cluster-fuzz, minimal repro below:

  <base href="http://www.image-in-nation.com/"><link href="templates/Imagination.css" rel="stylesheet">

Note that the first line of that style-sheet is:

  @import url("/template/Imagination.css");

and that

  http://www.image-in-nation.com/template/Imagination.css

redirects to

  http://www.image-in-nation.com/templates/Imagination.css

See http://crbug.com/122606 for the initial report.
Comment 1 David Barr 2012-04-09 21:57:19 PDT
Created attachment 136396 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2012-04-10 09:27:44 PDT
Comment on attachment 136396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136396&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests - checkpointing a simple fix.

It would be a good idea to add a new test (which should be easy thanks to a few crashing cases we have at hand), especially given that the correct behavior was regressed some time ago.
Comment 3 Alexey Proskuryakov 2012-04-10 13:02:40 PDT
Comment on attachment 136396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136396&action=review

Not sure what "checkpointing" means in this context, but this is clearly the kind of fix that needs regression tests.

> Source/WebCore/css/CSSImportRule.cpp:141
>          // FIXME: This is wrong if the finalURL was updated via document::updateBaseURL.

Is this FIXME still accurate?
Comment 4 David Barr 2012-04-10 17:27:54 PDT
Created attachment 136581 [details]
Patch
Comment 5 Darin Adler 2012-04-10 17:40:16 PDT
Comment on attachment 136581 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136581&action=review

> Source/WebCore/css/CSSImportRule.cpp:-141
> -        // FIXME: This is wrong if the finalURL was updated via document::updateBaseURL.

This FIXME doesn’t seem to be for the bug you’re fixing, so please don’t remove it.
Comment 6 David Barr 2012-04-10 17:42:53 PDT
Created attachment 136585 [details]
Patch
Comment 7 David Barr 2012-04-10 18:30:59 PDT
(In reply to comment #5)
> > Source/WebCore/css/CSSImportRule.cpp:-141
> > -        // FIXME: This is wrong if the finalURL was updated via document::updateBaseURL.
> 
> This FIXME doesn’t seem to be for the bug you’re fixing, so please don’t remove it.

The FIXME was added in http://trac.webkit.org/changeset/53607
I thought it might have been addressed as a side-effect of the change.
I refrained from removing it in the updated patch.
Adam, does the comment still apply after this change?
Comment 8 Adam Barth 2012-04-10 19:44:43 PDT
I'm not sure.
Comment 9 David Barr 2012-04-10 20:00:40 PDT
Given the lack of assuredness that this change addresses the FIXME, let's leave it there.
Comment 10 Eric Seidel (no email) 2012-04-11 00:34:14 PDT
Comment on attachment 136585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136585&action=review

> Source/WebCore/css/CSSImportRule.cpp:141
>          // FIXME: This is wrong if the finalURL was updated via document::updateBaseURL.

Do we need to test this FIXME As well?  With a <base> tag?

> Source/WebCore/css/CSSImportRule.cpp:142
> +        if (absHref == sheet->finalURL().string() || absHref == sheet->href())

Is href() always absolute?

    // Note that href is the URL that started the redirect chain that led to
    // this style sheet. This property probably isn't useful for much except
    // the JavaScript binding (which needs to use this value for security).
    const String& href() const { return m_originalURL; }

How do we do other redirect recursion checks?  Do we keep redirect chains cached somewhere at the network layer?
Comment 11 David Barr 2012-04-11 21:13:30 PDT
(In reply to comment #10)
> (From update of attachment 136585 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136585&action=review
> 
> > Source/WebCore/css/CSSImportRule.cpp:141
> >          // FIXME: This is wrong if the finalURL was updated via document::updateBaseURL.
> 
> Do we need to test this FIXME As well?  With a <base> tag?

Sure, but if so, that's another bug.

> > Source/WebCore/css/CSSImportRule.cpp:142
> > +        if (absHref == sheet->finalURL().string() || absHref == sheet->href())
> 
> Is href() always absolute?

By my reading of the code, yes.

Prior to the introduction of putativeBaseURL(), the comparison used to be just against href().
Now, putativeBaseURL() is known as finalURL().

>     // Note that href is the URL that started the redirect chain that led to
>     // this style sheet. This property probably isn't useful for much except
>     // the JavaScript binding (which needs to use this value for security).
>     const String& href() const { return m_originalURL; }
> 
> How do we do other redirect recursion checks?  Do we keep redirect chains cached somewhere at the network layer?

Strictly speaking, this is not a redirect recursion bug. The problem is that redirects break our stylesheet-import cycle detection.
Comment 12 Ryosuke Niwa 2012-04-15 17:08:25 PDT
Comment on attachment 136585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136585&action=review

I think this patch is a progression and landable as is although there are cases where this patch won't adequately address.

e.g. say we have <link href="redirection1.css"> then redirection1.css would redirect us to redirection2.css then again to actual.css.
we can then encounter <link href="redirection2.css">, which then redirect again to actual.css. But fixing this bug in general requires making http requests, etc... so it should probably be fixed in a separate patch.

> LayoutTests/http/tests/css/css-imports-redirect-cycle.html:1
> +<script>

Do we really want to use quirks mode?
Comment 13 David Barr 2012-04-15 17:25:27 PDT
Created attachment 137255 [details]
Patch
Comment 14 David Barr 2012-04-15 17:27:18 PDT
Comment on attachment 137255 [details]
Patch

Addressed comment about quirks mode in test case.
Comment 15 WebKit Review Bot 2012-04-15 17:30:13 PDT
Comment on attachment 137255 [details]
Patch

Attachment 137255 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12409697
Comment 16 Build Bot 2012-04-15 17:41:32 PDT
Comment on attachment 137255 [details]
Patch

Attachment 137255 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12405915
Comment 17 David Barr 2012-04-15 17:42:55 PDT
Created attachment 137257 [details]
Patch
Comment 18 David Barr 2012-04-15 17:43:58 PDT
Comment on attachment 137257 [details]
Patch

Accounted for interface change in StyleSheetInternal, href() => originalURL().
Comment 19 David Barr 2012-04-15 18:23:55 PDT
Created attachment 137260 [details]
Patch
Comment 20 David Barr 2012-04-15 18:26:47 PDT
Comment on attachment 137260 [details]
Patch

Discovered that cr-linux can be flaky with tests that are missing blank pixel results. Amended patch to exclude pixel results.
Comment 21 David Barr 2012-04-15 20:21:24 PDT
Created attachment 137268 [details]
Patch for landing
Comment 22 David Barr 2012-04-16 19:07:44 PDT
The flakiness in xmlhttprequest-unsafe-redirect.html seems unrelated to my patch, StyleRuleImport:: requestStyleSheet() shouldn't be on the code path at all for that test, AFAICT.
Comment 24 noel gordon 2012-04-16 23:19:23 PDT
Debugging sucks, testing is better.  Your new test passes in those trys, as does xmlhttprequest-unsafe-redirect.html.
Comment 25 WebKit Review Bot 2012-04-16 23:27:08 PDT
Comment on attachment 137268 [details]
Patch for landing

Clearing flags on attachment: 137268

Committed r114350: <http://trac.webkit.org/changeset/114350>
Comment 26 WebKit Review Bot 2012-04-16 23:27:15 PDT
All reviewed patches have been landed.  Closing bug.