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.
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
(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?
(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.
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.
(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.
<rdar://problem/11549186>
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.
Created attachment 144630 [details] patch
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?
(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.
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.
(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.
Created attachment 144877 [details] Patch-Updated Patch updated as per comment#12
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.
(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.
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.
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
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
Comment on attachment 145417 [details] patch Clearing the review flag as it breaks many testcases on Chromium. Does not look like a viable solution.
Created attachment 160996 [details] Patch
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.
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
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
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.
Created attachment 161044 [details] Patch
Comment on attachment 161044 [details] Patch New patch that avoids moving anything into the DOM or altering the size of Element.
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.
Created attachment 161057 [details] Patch
Comment on attachment 161057 [details] Patch Updated Changelog to correct formatting issues.
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
Ping render tree experts. Is the approach in John's patch right?
Created attachment 161290 [details] Patch
Comment on attachment 161290 [details] Patch Fix more format issues in Changelog
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.
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...
Created attachment 161497 [details] Patch
Comment on attachment 161497 [details] Patch Simplified test html file
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.
Still awaiting review...
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).
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?
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.
(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.
Comment on attachment 161497 [details] Patch This no longer reproduces in ToT.