Bug 105275 – REGRESSION (r118044): CSSParser crashes, when no context is available, and the value is a valid keyword

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Renata Hodovan]

My SVGFuzzer crashes on this svg test:

<svg xmlns="http://www.w3.org/2000/svg">
    <circle>
        <animate attributeName="display" to="bevel"></animate>
    </circle>
</svg>


The crash comes from the CSSParser.
StylePropertySet::setProperty was called with 0 (what is 0 by default too) contextStyleSheet from SVGAnimateElement. This zero was propageted to CSSParser::isValidKeywordPropertyAndValue() where parserContext.isCSSGridLayoutEnabled was NULL too and caused a segfault.


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff3d05012 in isValidKeywordPropertyAndValue (propertyId=WebCore::CSSPropertyDisplay, valueID=724, parserContext=...)
    at /home/reni/WebKit-git/Source/WebCore/css/CSSParser.cpp:632
632	        if (parserContext.isCSSGridLayoutEnabled && (valueID == CSSValueWebkitGrid || valueID == CSSValueWebkitInlineGrid))
(gdb) bt 8 
#0  0x00007ffff3d05012 in isValidKeywordPropertyAndValue (propertyId=WebCore::CSSPropertyDisplay, valueID=724, parserContext=...)
    at /home/reni/WebKit-git/Source/WebCore/css/CSSParser.cpp:632
#1  0x00007ffff3d06119 in parseKeywordValue (declaration=0x9926f0, propertyId=WebCore::CSSPropertyDisplay, string=..., important=false, parserContext=...)
    at /home/reni/WebKit-git/Source/WebCore/css/CSSParser.cpp:1119
#2  0x00007ffff3d0690a in WebCore::CSSParser::parseValue (declaration=0x9926f0, propertyID=WebCore::CSSPropertyDisplay, string=..., important=false, 
    cssParserMode=WebCore::SVGAttributeMode, contextStyleSheet=0x0) at /home/reni/WebKit-git/Source/WebCore/css/CSSParser.cpp:1232
#3  0x00007ffff3e01d11 in WebCore::StylePropertySet::setProperty (this=0x9926f0, propertyID=WebCore::CSSPropertyDisplay, value=..., important=false, 
    contextStyleSheet=0x0) at /home/reni/WebKit-git/Source/WebCore/css/StylePropertySet.cpp:661
#4  0x00007ffff4ac098d in applyCSSPropertyToTarget (targetElement=0x98c0c0, id=WebCore::CSSPropertyDisplay, value=...)
    at /home/reni/WebKit-git/Source/WebCore/svg/SVGAnimateElement.cpp:242
#5  0x00007ffff4ac0b4a in applyCSSPropertyToTargetAndInstances (targetElement=0x98c0c0, attributeName=..., valueAsString=...)
    at /home/reni/WebKit-git/Source/WebCore/svg/SVGAnimateElement.cpp:264
#6  0x00007ffff4ac129f in WebCore::SVGAnimateElement::applyResultsToTarget (this=0x9985b0) at /home/reni/WebKit-git/Source/WebCore/svg/SVGAnimateElement.cpp:359
#7  0x00007ffff4a5e516 in WebCore::SMILTimeContainer::updateAnimations (this=0x96a280, elapsed=..., seekToTime=false)
    at /home/reni/WebKit-git/Source/WebCore/svg/animation/SMILTimeContainer.cpp:319
(More stack frames follow...)

[Renata Hodovan]

Maybe we could simply extend it to

if(contextSytleSheet && parseKeyWordValue()) {}

in parseValue(). But I'm not really familiar with CSS. Ideas?

[Renata Hodovan]

We get the same crash with <set> svg tag too:


<svg xmlns="http://www.w3.org/2000/svg">
    <rect>
        <set attributeName="display" to="round"></set>
    </rect>
</svg>

[Renata Hodovan]

Created attachment 179965 [details]
Speculative fix

Maybe I should add a new test case too (?).

[Alexey Proskuryakov]

Regressed in <http://trac.webkit.org/changeset/118044>.

[Ryosuke Niwa]

Comment on attachment 179965 [details]
Speculative fix

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

> Source/WebCore/ChangeLog:10
> +        No new tests (OOPS!).

Please add a test.

[Tony Chang]

Comment on attachment 179965 [details]
Speculative fix

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

> Source/WebCore/css/CSSParser.cpp:1232
> -    if (parseKeywordValue(declaration, propertyID, string, important, contextStyleSheet->parserContext()))
> +    if (contextStyleSheet && parseKeywordValue(declaration, propertyID, string, important, contextStyleSheet->parserContext()))

I don't think this is correct.  A few lines below, we create a CSSParserContext or use contextStyleSheet->parserContext() if it's available.  We should try moving that code block here.  This might have a perf impact.  Try running run-perf-tests Dromaeo/jslib-style-prototype to check.

[Renata Hodovan]

Created attachment 180052 [details]
Proposed patch

[Renata Hodovan]

(In reply to comment #6)
> (From update of attachment 179965 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179965&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:1232
> > -    if (parseKeywordValue(declaration, propertyID, string, important, contextStyleSheet->parserContext()))
> > +    if (contextStyleSheet && parseKeywordValue(declaration, propertyID, string, important, contextStyleSheet->parserContext()))
> 
> I don't think this is correct.  A few lines below, we create a CSSParserContext or use contextStyleSheet->parserContext() if it's available.  We should try moving that code block here.  This might have a perf impact.  Try running run-perf-tests Dromaeo/jslib-style-prototype to check.

I've runned both versions a few times and it's seems that there is no measureable difference between them (+/-1 %). This way I choosed you idea.

[Tony Chang]

Comment on attachment 180052 [details]
Proposed patch

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

> LayoutTests/fast/css/invalid-parsercontext-valid-keyword-crash-expected.txt:1
> +Excellent - did not crash. See bug https://bugs.webkit.org/show_bug.cgi?id=105275

Nit: Normally we write something like "This test passes if it does not crash." or just PASS, but this is also OK.

> LayoutTests/fast/css/invalid-parsercontext-valid-keyword-crash.svg:10
> +        testRunner.dumpAsText();

Please indent the dumpAsText() line 4 more spaces.

[Renata Hodovan]

Committed r138141: <http://trac.webkit.org/changeset/138141>