Bug 87533

Summary:

Crash in styleForFirstLetter (pseudoStyle is null)

Product:

WebKit

Reporter:

Yong Li <yong.li.webkit>

Component:

Layout and Rendering

Assignee:

John Griggs <jgriggs>

Status:

RESOLVED WORKSFORME

Severity:

Normal

CC:

andersca, ap, bdakin, cmarcelo, dbates, dglazkov, eric, esprehn, igor.oliveira, inferno, jchaffraix, jgriggs, jkjiang, joethomas, kenrb, kling, koivisto, macpherson, menard, simon.fraser, webkit-bug-importer, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch	none
Patch-Updated	none
patch	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-04	none
Patch	none
Archive of layout-test-results from gce-cr-linux-03	none
Patch	none
Patch	none
Patch	none
Patch	none

Yong Li

Reported 2012-05-25 13:45:55 PDT

The minimal test I can get is: <style id="m"></style> <script>var g=":nth-child(2n+1) :first-letter{ list-style-image: url(http://www.w3.org/1999/xhtml); }"; var me; function test(){ me=document.getElementById("m"); me.textContent=g; } window.addEventListener("load", test, false); </script> <body> <div><span></span></div> <span></span> <span><span><span><span></span></span></span></span> <table> <tr><input></input></tr> <tr><table></table></tr> <a></a> <table> <tbody></tbody> <th><td></td></th><tr><td>???</td></tr></table> </body> It appears as null pointer dereferencing, but I feel it is a critical issue in layout engine or CSS.

Attachments
patch (4.85 KB, patch) 2012-05-29 15:06 PDT, Joe Thomas	no flags	Details Formatted Diff Diff
Patch-Updated (4.46 KB, patch) 2012-05-30 11:24 PDT, Joe Thomas	no flags	Details Formatted Diff Diff
patch (5.53 KB, text/plain) 2012-06-01 18:28 PDT, Joe Thomas	webkit.review.bot: commit-queue-	Details
Archive of layout-test-results from ec2-cr-linux-04 (788.80 KB, application/zip) 2012-06-01 23:47 PDT, WebKit Review Bot	no flags	Details
Patch (10.66 KB, patch) 2012-08-28 09:24 PDT, John Griggs	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-03 (394.33 KB, application/zip) 2012-08-28 10:25 PDT, WebKit Review Bot	no flags	Details
Patch (5.78 KB, patch) 2012-08-28 13:02 PDT, John Griggs	no flags	Details Formatted Diff Diff
Patch (5.00 KB, patch) 2012-08-28 14:07 PDT, John Griggs	no flags	Details Formatted Diff Diff
Patch (4.02 KB, patch) 2012-08-29 12:47 PDT, John Griggs	no flags	Details Formatted Diff Diff
Patch (3.94 KB, patch) 2012-08-30 09:46 PDT, John Griggs	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Yong Li

Comment 1 2012-05-25 14:02:47 PDT

Call stack is like this: #0 WebCore::RenderStyle::isFloating (this=0x0) #1 WebCore::styleForFirstLetter #2 WebCore::RenderBlock::updateFirstLetter #3 WebCore::RenderBlock::computePreferredLogicalWidths #4 WebCore::RenderTableCell::computePreferredLogicalWidths #5 WebCore::AutoTableLayout::recalcColumn #6 WebCore::AutoTableLayout::fullRecalc #7 WebCore::AutoTableLayout::computePreferredLogicalWidths #8 WebCore::RenderTable::computePreferredLogicalWidths #9 WebCore::RenderBox::maxPreferredLogicalWidth #10 WebCore::RenderTable::computeLogicalWidth #11 WebCore::RenderTable::layout #12 WebCore::RenderBlock::layoutBlockChild #13 WebCore::RenderBlock::layoutBlockChildren #14 WebCore::RenderBlock::layoutBlock #15 WebCore::RenderBlock::layout #16 WebCore::RenderBlock::layoutBlockChild #17 WebCore::RenderBlock::layoutBlockChildren #18 WebCore::RenderBlock::layoutBlock #19 WebCore::RenderBlock::layout #20 WebCore::RenderBlock::layoutBlockChild #21 WebCore::RenderBlock::layoutBlockChildren #22 WebCore::RenderBlock::layoutBlock #23 WebCore::RenderBlock::layout #24 WebCore::RenderView::layout #25 WebCore::FrameView::layou

Ken Buchanan

Comment 2 2012-05-25 15:12:24 PDT

(In reply to comment #0) > > It appears as null pointer dereferencing, but I feel it is a critical issue in layout engine or CSS. We don't normally consider NULL pointer dereferences to be security issues. Is there a reason you think it should be treated as such?

Yong Li

Comment 3 2012-05-28 07:15:21 PDT

(In reply to comment #2) > (In reply to comment #0) > > > > It appears as null pointer dereferencing, but I feel it is a critical issue in layout engine or CSS. > > We don't normally consider NULL pointer dereferences to be security issues. Is there a reason you think it should be treated as such? If it is purely a null pointer dereference, adding some null-checks should be good enough in most cases. But in this case, it seems to me RenderBlock::updateFirstLetter doesn't think the pseudoStyle can ever be null, which means this assumption is broken. Usually, such problems in the layout engine are security issues. That being said, I'm not certain, though.

Ken Buchanan

Comment 4 2012-05-28 08:31:07 PDT

We can't assume that any bug that breaks a code assumption in rendering is a security bug, because that's a pretty broad brush and we wouldn't be able to deal with the volume. Obviously this is a bug, but it's not practical label it a security bug without more direct evidence of it enabling exploitable memory corruption.

Yong Li

Comment 5 2012-05-28 08:39:55 PDT

(In reply to comment #4) > We can't assume that any bug that breaks a code assumption in rendering is a security bug, because that's a pretty broad brush and we wouldn't be able to deal with the volume. Obviously this is a bug, but it's not practical label it a security bug without more direct evidence of it enabling exploitable memory corruption. I was just afraid making it publicly visible could be a risk. But if the policy is "don't mark it as security issue until evidence is found", I'll make it a non-security one.

Radar WebKit Bug Importer

Comment 6 2012-05-29 10:51:36 PDT

<rdar://problem/11549186>

Joe Thomas

Comment 7 2012-05-29 12:58:07 PDT

pseudoStyle is null as there are no matchedProperties in StyleResolver::pseudoStyleForElement. Looks like null check here will solve the crash. I will create a patch for it.

Joe Thomas

Comment 8 2012-05-29 15:06:39 PDT

Created attachment 144630 [details] patch

Eric Seidel (no email)

Comment 9 2012-05-29 16:34:20 PDT

Comment on attachment 144630 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=144630&action=review Interesting. I would have assumed that getCachedPsuedoStyle might have returned the normal style when there is no pseudo. > Source/WebCore/rendering/RenderBlock.cpp:6024 > + if (!pseudoStyle) > + return; What if the style changed to detach? or to no longer be a first letter? Is this early-return safe?

Joe Thomas

Comment 10 2012-05-29 18:55:03 PDT

(In reply to comment #9) > (From update of attachment 144630 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144630&action=review > > Interesting. I would have assumed that getCachedPsuedoStyle might have returned the normal style when there is no pseudo. When there are no matching properties, StyleResolver::pseudoStyleForElement returns null from the time this function is introduced(http://trac.webkit.org/changeset/5234). Apart from the above case, getCachedPseudoStyle can return null from various other places. And currently whoever calls this function also handles the null case except the place where we are getting the crash now. > > > Source/WebCore/rendering/RenderBlock.cpp:6024 > > + if (!pseudoStyle) > > + return; > > What if the style changed to detach? or to no longer be a first letter? Is this early-return safe? With my limited knowledge in rendering, early return looks fine to me as we cannot execute these functions further without Pseudo RenderStyle.

Julien Chaffraix

Comment 11 2012-05-30 08:21:37 PDT

Comment on attachment 144630 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=144630&action=review >>> Source/WebCore/rendering/RenderBlock.cpp:6024 >>> + return; >> >> What if the style changed to detach? or to no longer be a first letter? Is this early-return safe? > > With my limited knowledge in rendering, early return looks fine to me as we cannot execute these functions further without Pseudo RenderStyle. I am also concerned about the already returns. If you have no pseudo-style, it looks like you either shouldn't be calling the updateFirstLetter* function or should be detaching your first letter renderers as it means the first letter selector don't apply anymore.

Joe Thomas

Comment 12 2012-05-30 11:12:24 PDT

(In reply to comment #11) > (From update of attachment 144630 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144630&action=review > > >>> Source/WebCore/rendering/RenderBlock.cpp:6024 > >>> + return; > >> > >> What if the style changed to detach? or to no longer be a first letter? Is this early-return safe? > > > > With my limited knowledge in rendering, early return looks fine to me as we cannot execute these functions further without Pseudo RenderStyle. > > I am also concerned about the already returns. If you have no pseudo-style, it looks like you either shouldn't be calling the updateFirstLetter* function or should be detaching your first letter renderers as it means the first letter selector don't apply anymore. After some more debugging, it looks like null check is required only in RenderBlock::createFirstLetterRenderer and not needed in RenderBlock::updateFirstLetterStyle. Renderer for FirstLetter pseudo element will not be created in createFirstLetterRenderer() if there is no pseudoStyle because of the newly introduced null check. And before calling updateFirstLetterStyle(), we check whether FirstLetter renderer is there.

Joe Thomas

Comment 13 2012-05-30 11:24:49 PDT

Created attachment 144877 [details] Patch-Updated Patch updated as per comment#12

Julien Chaffraix

Comment 14 2012-05-30 11:33:39 PDT

Comment on attachment 144877 [details] Patch-Updated View in context: https://bugs.webkit.org/attachment.cgi?id=144877&action=review > Source/WebCore/ChangeLog:9 > + Adding null check for Pseudo element's RenderStyle as getCachedPseudoStyle returns null in different scenarios. > + In this case, null is returned from StyleResolver::pseudoStyleForElement when the matchedProperties list is empty. Please update your ChangeLog, it mentions matchedProperties which bears little (if any) relation to the fix. > Source/WebCore/rendering/RenderBlock.cpp:6074 > + if (!pseudoStyle) > + return; Why are we calling createFirstLetterRenderer if we don't have a first letter in this case? You haven't answered this question and without that, we can't assess if your patch is right.

Joe Thomas

Comment 15 2012-05-30 16:39:07 PDT

(In reply to comment #14) > (From update of attachment 144877 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144877&action=review > > > Source/WebCore/ChangeLog:9 > > + Adding null check for Pseudo element's RenderStyle as getCachedPseudoStyle returns null in different scenarios. > > + In this case, null is returned from StyleResolver::pseudoStyleForElement when the matchedProperties list is empty. > > Please update your ChangeLog, it mentions matchedProperties which bears little (if any) relation to the fix. I mentioned matchedProperties to give more details about the place from which null is returned. getCachedPseudoStyle() calls StyleResolver::pseudoStyleForElement and this returns null when matchedProperties is empty. > > > Source/WebCore/rendering/RenderBlock.cpp:6074 > > + if (!pseudoStyle) > > + return; > > Why are we calling createFirstLetterRenderer if we don't have a first letter in this case? You haven't answered this question and without that, we can't assess if your patch is right. There is a firstLetterBlock and calling createFirstLetterRenderer() is right. The problems looks to be in StyleResolver::pseudoStyleForElement, where it returns NULL if there no matchedProperties after trying to match UA, User and Author rules. Another approach to solve this crash would be to return the style inherited from parent if there are no matchedProperties in pseudoStyleForElement() instead of returning NULL. Eric has mentioned about it in comment#9. Similar functions like StyleResolver::styleForElement does not return NULL if there are no matchedProperties. Please let me know your thoughts on this.

Joe Thomas

Comment 16 2012-06-01 18:28:02 PDT

Created attachment 145417 [details] patch Return the normal style (style inherited from parent) if there are no matchedProperties in StyleResolver:pseudoStyleForElement() instead of returning NULL as mentioned in Comment#15. Looks to me a better approach to solve the crash than the null check in the previous patch.

WebKit Review Bot

Comment 17 2012-06-01 23:47:08 PDT

Comment on attachment 145417 [details] patch Attachment 145417 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12859905 New failing tests: compositing/reflections/nested-reflection-on-overflow.html fast/css/resize-corner-tracking-transformed.html scrollbars/custom-scrollbar-with-incomplete-style.html fast/css/resize-corner-tracking.html fast/repaint/scroll-inside-table-cell.html fast/overflow/hit-test-overflow-controls.html fast/replaced/width100percent-textarea.html compositing/scrollbar-painting.html fast/frames/iframe-scaling-with-scroll.html fast/frames/transparent-scrollbar.html fast/repaint/scroll-relative-table-inside-table-cell.html compositing/overflow/clip-content-under-overflow-controls.html scrollbars/scrollbars-on-positioned-content.html compositing/iframes/iframe-in-composited-layer.html compositing/overflow/content-loses-scrollbars.html platform/chromium/scrollbars/rtl-resizer-position.html compositing/overflow/scrollbar-painting.html

WebKit Review Bot

Comment 18 2012-06-01 23:47:14 PDT

Created attachment 145433 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Joe Thomas

Comment 19 2012-06-04 10:39:32 PDT

Comment on attachment 145417 [details] patch Clearing the review flag as it breaks many testcases on Chromium. Does not look like a viable solution.

John Griggs

Comment 20 2012-08-28 09:24:20 PDT

Created attachment 160996 [details] Patch

John Griggs

Comment 21 2012-08-28 09:27:52 PDT

The root cause of the crash turns out to be an issue with the nth-child(2n+1) selector in the test case. In some cases, style sharing causes the childIndex to be stored incorrectly in SelectorChecker::checkOneSelector. The patch moves childIndex from RenderStyle to Element, which allows the continued use of childIndex as a performance enhancement.

WebKit Review Bot

Comment 22 2012-08-28 10:25:48 PDT

Comment on attachment 160996 [details] Patch Attachment 160996 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13652449 New failing tests: fast/multicol/progression-reverse.html fast/dom/nthchild-firstletter-crash.html

WebKit Review Bot

Comment 23 2012-08-28 10:25:52 PDT

Created attachment 161007 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Eric Seidel (no email)

Comment 24 2012-08-28 10:59:11 PDT

Comment on attachment 160996 [details] Patch I'm surrprised we don't have a COMPILE_ASSERT For Element size. I would expect this to be a memory regression.

John Griggs

Comment 25 2012-08-28 13:02:42 PDT

Created attachment 161044 [details] Patch

John Griggs

Comment 26 2012-08-28 13:03:47 PDT

Comment on attachment 161044 [details] Patch New patch that avoids moving anything into the DOM or altering the size of Element.

Yong Li

Comment 27 2012-08-28 13:41:13 PDT

Comment on attachment 161044 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161044&action=review > Source/WebCore/ChangeLog:27 > +2012-08-28 John Griggs <jgriggs@rim.com> > + > + Source/WebCore: > + > + 2012-08-28 John Griggs <jgriggs@rim.com> > + > + Crash in styleForFirstLetter (pseudoStyle is null) https://bugs.webkit.org/show_bug.cgi?id=87533 > + Don't write childIndex into potentially shared RenderStyle objects when processing :nth-child selector. > + > + Reviewed by NOBODY (OOPS!). > + > + Test: fast/dom/nthchild-firstletter-crash.html > + > + * css/SelectorChecker.cpp: > + (WebCore::SelectorChecker::checkOneSelector): > + > + LayoutTests: > + > + 2012-08-28 John Griggs <jgriggs@rim.com> > + > + Crash in styleForFirstLetter (pseudoStyle is null) https://bugs.webkit.org/show_bug.cgi?id=87533 > + Don't write childIndex into potentially shared RenderStyle objects when processing :nth-child selector. > + > + Reviewed by NOBODY (OOPS!). > + > + * fast/dom/nthchild-firstletter-crash-expected.txt: Added. > + * fast/dom/nthchild-firstletter-crash.html: Added. should remove this nested log. > Source/WebCore/ChangeLog:35 > + Reviewed by NOBODY (OOPS!). > + > + Test: fast/dom/nthchild-firstletter-crash.html > + Missing description between line 33 and 34.

John Griggs

Comment 28 2012-08-28 14:07:46 PDT

Created attachment 161057 [details] Patch

John Griggs

Comment 29 2012-08-28 14:08:33 PDT

Comment on attachment 161057 [details] Patch Updated Changelog to correct formatting issues.

Yong Li

Comment 30 2012-08-29 12:15:34 PDT

Comment on attachment 161057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161057&action=review > LayoutTests/ChangeLog:14 > +2012-08-28 John Griggs <jgriggs@rim.com> > + > + Source/WebCore: > + > + 2012-08-28 John Griggs <jgriggs@rim.com> > + > + Crash in styleForFirstLetter (pseudoStyle is null) https://bugs.webkit.org/show_bug.cgi?id=87533 > + Don't write childIndex into potentially shared RenderStyle objects when processing :nth-child selector. > + > + Reviewed by NOBODY (OOPS!). > + > + Test: fast/dom/nthchild-firstletter-crash.html > + > + * css/SelectorChecker.cpp: Still nested here > LayoutTests/ChangeLog:34 > + Reviewed by NOBODY (OOPS!). > + > + * fast/dom/nthchild-firstletter-crash-expected.txt: Added. missing description between 32-34

Yong Li

Comment 31 2012-08-29 12:16:25 PDT

Ping render tree experts. Is the approach in John's patch right?

John Griggs

Comment 32 2012-08-29 12:47:16 PDT

Created attachment 161290 [details] Patch

John Griggs

Comment 33 2012-08-29 12:48:05 PDT

Comment on attachment 161290 [details] Patch Fix more format issues in Changelog

Elliott Sprehn

Comment 34 2012-08-29 15:06:55 PDT

Comment on attachment 161290 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161290&action=review > Source/WebCore/css/SelectorChecker.cpp:922 > + if (context.elementStyle) This means we never write to element->renderStyle(), why was that wrong before? > LayoutTests/fast/dom/nthchild-firstletter-crash.html:12 > +window.addEventListener("load", test, false); Can you reduce this test case? It seems like you should be able to reproduce this without the var me variables, and with a lot fewer elements.

John Griggs

Comment 35 2012-08-30 06:24:12 PDT

Sometimes element->renderStyle() returns a style that is shared and childIndex is set incorrectly. Then, when the code to layout the :first-letter pseudo-element runs, it cannot retrieve the style information and crashes when dereferencing a NULL pointer to the missing style. Avoiding writing to element->renderStyle() prevents writing to a shared RenderStyle. Previous attempts to patch this attempted to detect the NULL pointer and avoid dereferencing it (there was some concern that short-circuiting after detecting the NULL would be an issue and that this patch did not address the root cause) or to return an inherited style when the :first-letter stuff could not be selected (this patch broke several tests.) My own previous patch moved the childIndex from the Render tree into the dom (in the Element class), but this was rejected since it increases the size of Element. I based the test on the test case from this bug - I will attempt to simplify it...

John Griggs

Comment 36 2012-08-30 09:46:49 PDT

Created attachment 161497 [details] Patch

John Griggs

Comment 37 2012-08-30 09:47:48 PDT

Comment on attachment 161497 [details] Patch Simplified test html file

John Griggs

Comment 38 2012-09-18 11:02:19 PDT

Ping - Any chance of getting a review (or a comment explaining why not)? I believe I have responded to the most recent review comments and simplified the test case, as requested.

John Griggs

Comment 39 2012-10-01 09:38:31 PDT

Still awaiting review...

Eric Seidel (no email)

Comment 40 2012-10-01 10:13:18 PDT

Comment on attachment 161497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161497&action=review > Source/WebCore/css/SelectorChecker.cpp:-921 > - RenderStyle* childStyle = context.elementStyle ? context.elementStyle : element->renderStyle(); I'm confused. Element* const & element = context.element; is at the very top of this (long) function. It's unclear what this line even did, let alone why it's wrong. Could you save me the debugging by explaining (ideally in teh ChangeLog for all to see).

Eric Seidel (no email)

Comment 41 2012-10-01 10:14:32 PDT

Comment on attachment 161497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161497&action=review >> Source/WebCore/css/SelectorChecker.cpp:-921 >> - RenderStyle* childStyle = context.elementStyle ? context.elementStyle : element->renderStyle(); > > I'm confused. Element* const & element = context.element; > is at the very top of this (long) function. It's unclear what this line even did, let alone why it's wrong. Could you save me the debugging by explaining (ideally in teh ChangeLog for all to see). are all uses of element->renderStyle() wrong in this function?

Eric Seidel (no email)

Comment 42 2012-10-01 10:15:45 PDT

Anttik and Kling are really your ideal reviewers here. They've been in this code most recently. You should be able to find them both in #webkit, they're often there.

Andreas Kling

Comment 43 2012-10-01 10:20:44 PDT

(In reply to comment #40) > (From update of attachment 161497 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161497&action=review > > > Source/WebCore/css/SelectorChecker.cpp:-921 > > - RenderStyle* childStyle = context.elementStyle ? context.elementStyle : element->renderStyle(); > > I'm confused. Element* const & element = context.element; > is at the very top of this (long) function. It's unclear what this line even did, let alone why it's wrong. Could you save me the debugging by explaining (ideally in teh ChangeLog for all to see). +1 to this. Needs more explanation.

Anders Carlsson

Comment 44 2014-02-05 10:43:10 PST

Comment on attachment 161497 [details] Patch This no longer reproduces in ToT.

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