Bug 105275

Summary:

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

Product:

WebKit

Reporter:

Renata Hodovan <rhodovan.u-szeged>

Component:

CSS

Assignee:

Renata Hodovan <rhodovan.u-szeged>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarcelo, koivisto, macpherson, menard, ojan.autocc, tony, webkit.review.bot, zimmermann

Priority:

Keywords:

Regression

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

116980

Attachments:

Description	Flags
Speculative fix	rniwa: review-, rniwa: commit-queue-
Proposed patch	tony: review+, tony: commit-queue-

Renata Hodovan

Reported 2012-12-18 03:42:23 PST

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

Attachments
Speculative fix (1.80 KB, patch) 2012-12-18 09:41 PST, Renata Hodovan	rniwa: review- rniwa: commit-queue-	Details Formatted Diff Diff
Proposed patch (4.46 KB, patch) 2012-12-18 16:26 PST, Renata Hodovan	tony: review+ tony: commit-queue-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Renata Hodovan

Comment 1 2012-12-18 03:47:25 PST

Maybe we could simply extend it to if(contextSytleSheet && parseKeyWordValue()) {} in parseValue(). But I'm not really familiar with CSS. Ideas?

Renata Hodovan

Comment 2 2012-12-18 03:49:31 PST

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

Comment 3 2012-12-18 09:41:20 PST

Created attachment 179965 [details] Speculative fix Maybe I should add a new test case too (?).

Alexey Proskuryakov

Comment 4 2012-12-18 09:49:18 PST

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

Ryosuke Niwa

Comment 5 2012-12-18 09:51:39 PST

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 6 2012-12-18 10:57:43 PST

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

Comment 7 2012-12-18 16:26:06 PST

Created attachment 180052 [details] Proposed patch

Renata Hodovan

Comment 8 2012-12-18 16:28:50 PST

(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 9 2012-12-18 16:32:26 PST

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

Comment 10 2012-12-19 03:57:38 PST

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

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