Bug 42806 - Crash when CSS selector is very long.
Summary: Crash when CSS selector is very long.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 43783 43784
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-21 23:08 PDT by Hayato Ito
Modified: 2010-10-21 19:26 PDT (History)
3 users (show)

See Also:


Attachments
That would cause stack overflow (799 bytes, text/html)
2010-07-21 23:10 PDT, Hayato Ito
no flags Details
make-checkSelector-iterative (21.27 KB, patch)
2010-08-10 01:42 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2010-07-21 23:08:23 PDT
Take a look at the attached html. That would cause stack overflow like:


#0  0x00007ffff5b204a2 in WebCore::CSSSelector::hasTag (this=Cannot access memory at address 0x7fffff5aeff8
) at ../../../../webkit3/WebKit/WebCore/css/CSSSelector.h:238
#1  0x00007ffff5b29613 in WebCore::CSSStyleSelector::SelectorChecker::checkOneSelector (this=0xdc03f0, sel=0x2045f50, e=0x3ee6cf0, selectorAttrs=0xdc04a0, dynamicPseudo=@0xdc03e8, isSubSelector=false, elementStyle=0x0, elementParentStyle=0x0) at ../../../../webkit3/WebKit/WebCore/css/CSSStyleSelector.cpp:2078
#2  0x00007ffff5b28acb in WebCore::CSSStyleSelector::SelectorChecker::checkSelector (this=0xdc03f0, sel=0x2045f50, e=0x3ee6cf0, selectorAttrs=0xdc04a0, dynamicPseudo=@0xdc03e8, isSubSelector=false, encounteredLink=false, elementStyle=0x0, elementParentStyle=0x0) at ../../../../webkit3/WebKit/WebCore/css/CSSStyleSelector.cpp:1911
#3  0x00007ffff5b28e6a in WebCore::CSSStyleSelector::SelectorChecker::checkSelector (this=0xdc03f0, sel=0x2045f50, e=0x3ee6cf0, selectorAttrs=0xdc04a0, dynamicPseudo=@0xdc03e8, isSubSelector=false, encounteredLink=false, elementStyle=0x0, elementParentStyle=0x0) at ../../../../webkit3/WebKit/WebCore/css/CSSStyleSelector.cpp:1972
#4  0x00007ffff5b28e6a in WebCore::CSSStyleSelector::SelectorChecker::checkSelector (this=0xdc03f0, sel=0x2045fc0, e=0x3ee6f30, selectorAttrs=0xdc04a0, dynamicPseudo=@0xdc03e8, isSubSelector=false, encounteredLink=false, elementStyle=0x0, elementParentStyle=0x0) at ../../../../webkit3/WebKit/WebCore/css/CSSStyleSelector.cpp:1972
Comment 1 Hayato Ito 2010-07-21 23:10:03 PDT
Created attachment 62265 [details]
That would cause stack overflow
Comment 2 Hayato Ito 2010-07-21 23:12:37 PDT
https://bugs.webkit.org/show_bug.cgi?id=41129 is a similar bug which was already fixed.

We have to use an iterative approach instead of recursive one in order to avoid stack overflow.

I found the following functions use recursion:

- CSSSelector::specifity()
- CSSStyleSelector::SelectorChecker::checkSelector()

We need to investige further. There might be other functions which use recursion.
Comment 3 Mark Rowe (bdash) 2010-07-21 23:46:01 PDT
<rdar://problem/8220988>
Comment 4 Hayato Ito 2010-08-04 01:50:42 PDT
Is there anyone working on fix this? If no one is working, I'll investigate the issue further.
Comment 5 Hayato Ito 2010-08-10 01:42:09 PDT
Created attachment 63988 [details]
make-checkSelector-iterative
Comment 6 Hayato Ito 2010-08-10 02:00:26 PDT
I've made CSSStyleSelector::SelectorChecker::checkSelector() iterative to avoid stack overflow.
To achieve it, I have to maintain CallStack manually.

Hi reviewers,

I am afraid the code became unintuitive. But I guess it is inevitable. If you have any ideas to make it clean and intuitive, please let me know.

I also have a concern about performance. Is there any standard way to make sure a performance degradation won't happen by this change?


I'll fix CSSSelector:specifity() in another patch later. That might be an pretty easy task.
Comment 7 WebKit Review Bot 2010-08-10 02:02:37 PDT
Attachment 63988 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSStyleSelector.cpp:1999:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
WebCore/css/CSSStyleSelector.cpp:2092:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Hayato Ito 2010-08-10 04:11:35 PDT
I've canceled the review request.
It might be better to make a couple of bugs to fix them separately step by step.

I'd like to use this bug as a master bug to manage them.
Comment 10 Hayato Ito 2010-10-19 19:32:36 PDT
I'll close this bug because all sub bugs are now fixed.
If you find any other cases which causes a crash, please reopen this.
Comment 11 Hayato Ito 2010-10-21 19:25:11 PDT
https://bugs.webkit.org/show_bug.cgi?id=43783 is reopened. So this master bug should be also reopened.