Stack overflow in CSS parser caused by recursive stylesheet import

• Component: CSS
• Status: RESOLVED FIXED

[David Barr]

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.

[David Barr]

Created attachment 136396 [details]
Patch

[Alexander Pavlov (apavlov)]

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.

[Alexey Proskuryakov]

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?

[David Barr]

Created attachment 136581 [details]
Patch

[Darin Adler]

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.

[David Barr]

Created attachment 136585 [details]
Patch

[David Barr]

(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?

[Adam Barth]

I'm not sure.

[David Barr]

Given the lack of assuredness that this change addresses the FIXME, let's leave it there.

[Eric Seidel (no email)]

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?

[David Barr]

(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.

[Ryosuke Niwa]

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?

[David Barr]

Created attachment 137255 [details]
Patch

[David Barr]

Comment on attachment 137255 [details]
Patch

Addressed comment about quirks mode in test case.

[WebKit Review Bot]

Comment on attachment 137255 [details]
Patch

Attachment 137255 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12409697

[Build Bot]

Comment on attachment 137255 [details]
Patch

Attachment 137255 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12405915

[David Barr]

Created attachment 137257 [details]
Patch

[David Barr]

Comment on attachment 137257 [details]
Patch

Accounted for interface change in StyleSheetInternal, href() => originalURL().

[David Barr]

Created attachment 137260 [details]
Patch

[David Barr]

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.

[David Barr]

Created attachment 137268 [details]
Patch for landing

[David Barr]

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.

[noel gordon]

Seems unrelated, but xmlhttprequest-unsafe-redirect.html has no history of flake.  It does flake in each of the cr-linux results above.

Running chromium try jobs with your patch to check that flake.

  http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/292
  http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/271
  http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/303
  http://build.chromium.org/p/tryserver.chromium/builders/linux_layout_rel/builds/222
  http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/238
  http://build.chromium.org/p/tryserver.chromium/builders/win_layout_rel/builds/239

[noel gordon]

Debugging sucks, testing is better.  Your new test passes in those trys, as does xmlhttprequest-unsafe-redirect.html.

[WebKit Review Bot]

Comment on attachment 137268 [details]
Patch for landing

Clearing flags on attachment: 137268

Committed r114350: <http://trac.webkit.org/changeset/114350>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.