46427 2010-09-23 16:21:07 -0700 Crash below CGContextShowGlyphsWithAdvances due to invalid viewBox 2026-05-11 07:13:25 -0700 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) Mac (Intel) OS X 10.6 RESOLVED CONFIGURATION CHANGED InRadar P2 Normal --- 42959 1 ap webkit-unassigned eric inferno karlcow krit mitz pdr rniwa schenney zimmermann oldest_to_newest 284069 0 68614 ap 2010-09-23 16:21:07 -0700 Created attachment 68614 serialized DOM I've seen a cross_fuzz crash happening in Safari 5.0.2 due to an invalid view box of root SVG element: (gdb) p viewBox() $51 = { m_location = { m_x = 0, m_y = 0 }, m_size = { m_width = inf, m_height = nan(0x400000) } } I couldn't find a way to reproduce it, since parsing of the viewBox attribute fails if there are infs or nans. I'm attaching a serialization of what the DOM looks like at the moment of crash, but there probably needs to be some dynamic manipulation performed to create such a broken viewBox. This might or might have not been fixed in ToT. This is not really a security bug, but I'm filing it as such because of fuzzer discussion. Also, even non-security crashes that happen in fuzzers complicate running them. 284126 1 ap 2010-09-23 17:15:14 -0700 Actually, this seems fairly reproducible for me. Please tell me if there's any debug output I can add that would help - I don't know much about SVG. 284143 2 mitz 2010-09-23 17:34:00 -0700 (In reply to comment #1) > Actually, this seems fairly reproducible for me. Please tell me if there's any debug output I can add that would help - I don't know much about SVG. Can you set a breakpoint to see when the viewBox value changes to that? 284200 3 ap 2010-09-23 20:38:20 -0700 As you are aware, it's all defined in macros, and viewBox isn't a plain variable. That said, if we can't get any SVG insight, I will of course have to resort to direct debugging techniques like this one. 284282 4 zimmermann 2010-09-24 00:44:15 -0700 (In reply to comment #3) > As you are aware, it's all defined in macros, and viewBox isn't a plain variable. That said, if we can't get any SVG insight, I will of course have to resort to direct debugging techniques like this one. SVGFitToViewBox::parseMappedAttribute contains the viewBox parsing code. It's invoked from SVGSVGElement::parseMappedAttribute. The bug is probably in SVGFitToViewBox::parseViewBox. Hope that helps? 284288 5 ap 2010-09-24 01:08:58 -0700 I don't see how parsing a viewBox string can produce inf or nan, which is why I've been suspecting some kind of direct manipulation (or animation?). 284293 6 zimmermann 2010-09-24 01:24:46 -0700 (In reply to comment #5) > I don't see how parsing a viewBox string can produce inf or nan, which is why I've been suspecting some kind of direct manipulation (or animation?). Aha, I didn't know that any DOM manipulations are involved. (never heard of cross_fuzz). Ok, let's see, SVGSVGElement.idl inherits from SVGFitToViewBox.idl, which exposes: readonly attribute SVGAnimatedRect viewBox; Internally we just store FloatRects to in the ANIMATED_PROPERTY macros. Whenever you call mySVGSVGElement.viewBox, an internal JSSVGAnimatedRect wrapper object is created, which lets you access the "baseVal" which is of type JSSVGRect. You can manipuate the x/y/width/height directly there. Snippet from JSSVGRect.cpp: void setJSSVGRectX(ExecState* exec, JSObject* thisObject, JSValue value) { JSSVGRect* castedThis = static_cast<JSSVGRect*>(thisObject); JSSVGPODTypeWrapper<FloatRect> * imp = static_cast<JSSVGPODTypeWrapper<FloatRect> *>(castedThis->impl()); FloatRect podImp(*imp); podImp.setX(value.toFloat(exec)); imp->commitChange(podImp, castedThis); } When calling mySVGSVGElement.viewBox.baseVal.x = 100; above code is executed, which modifes the x coordinate of the existing viewBox, and calls "commitChange" on the JSSVGPODTypeWrapper (which is used to expose POD types like FloatRect, FloatPoint, etc. to the corresponding SVG DOM types in the bindings). JSSVGDynamicPODTypeWrapper contains: virtual void commitChange(PODType type, DOMObject* wrapper) { (m_creator.get()->*m_setter)(type); JSSVGContextCache::propagateSVGDOMChange(wrapper, m_creator->associatedAttributeName()); } The second call just notifies the SVGSVGElement using svgAttributeChanged(SVGNames::viewBoxAttr), that the viewBox has been changed from SVG DOM, it's irrelevant here. The first call is more interessting. It notifies the creator of the JSSVGRect, that it has been changed. The creator here is the JSSVGAnimatedRect, where we called the baseVal function to get a SVGRect object: JSValue jsSVGAnimatedRectBaseVal(ExecState* exec, JSValue slotBase, const Identifier&) { JSSVGAnimatedRect* castedThis = static_cast<JSSVGAnimatedRect*>(asObject(slotBase)); UNUSED_PARAM(exec); SVGAnimatedRect* imp = static_cast<SVGAnimatedRect*>(castedThis->impl()); JSValue result = toJS(exec, castedThis->globalObject(), JSSVGDynamicPODTypeWrapperCache<FloatRect, SVGAnimatedRect>::lookupOrCreateWrapper(imp, &SVGAnimatedRect::baseVal, &SVGAnimatedRect::setBaseVal).get(), JSSVGContextCache::svgContextForDOMObject(castedThis));; return result; } The commitChange(..) call from JSSVGRect, causes a call to SVGAnimatedRect::setBaseVal, replacing the current FloatRect with the new one. The setBaseVal call is hidden in the SVGAnimatedTemplate/SVGAnimatedProperty macro hackery, but in the end it's just that the existing viewBox FloatRect is replaced by the new one. There's no check done at all wheter the viewBox is still correct. So it's possible to set the viewBox to any arbitary value, no one is checking it. Note this affects all properties like SVGRect/Number, etc.. very good catch! I might need to think a bit more how to intercept this, to perform some sanity checks first. 284400 7 68685 ap 2010-09-24 08:40:56 -0700 Created attachment 68685 test case (will crash) > You can manipuate the x/y/width/height directly there. Snippet from JSSVGRect.cpp: Ah! What I tried before was creating an SVGRect and assigning it to s.viewBox.baseVal - which of course didn't work, since the attribute is readonly. With your explanation, I could make a reduced test case. 286450 8 eric 2010-09-28 12:33:49 -0700 Agreed this is not a security bug. We could file a separate bug to track this with no mention of cross_fuzz if that's desired. I'm not sure I understand why CG is aborting here? I caught this in the debugger, but I don't understand what part of the viewbox being invalid causes the crash? 286452 9 mitz 2010-09-28 12:35:08 -0700 (In reply to comment #8) > Agreed this is not a security bug. We could file a separate bug to track this with no mention of cross_fuzz if that's desired. > > I'm not sure I understand why CG is aborting here? I caught this in the debugger, but I don't understand what part of the viewbox being invalid causes the crash? Looks like the CGContext has a singular CTM because of the invalid viewbox. 286454 10 eric 2010-09-28 12:36:39 -0700 Only SVG has this problem, right? Because HTML uses CGLayer to do all the transforms, if you ended up with a singular CTM the layer would simply not render, instead of CG crashing? 286468 11 eric 2010-09-28 12:51:08 -0700 Assertion failed: ((min.y == p[0].y && max.y == p[order].y) || (min.y == p[order].y && max.y == p[0].y)), function crossing_count, file Paths/path-crossing.c, line 176. Is the CG assert we're hitting. 286471 12 eric 2010-09-28 12:52:04 -0700 In playing with the reduction, I'm seeing this message too: Tue Sep 28 12:49:38 eseidel.local DumpRenderTree[91952] <Error>: crossing_count: warning: assertion failed: 0 is not in the range (nan, nan) or (nan, nan); assuming the latter. Please report this bug. From: <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 1 1"> <g stroke="red" stroke-width="4"> <text x="50" y="60">Target</text> </g> <script> document.documentElement.viewBox.baseVal.width = Infinity; </script> </svg> 286473 13 eric 2010-09-28 12:52:39 -0700 In fact, the reduction in comment 12 causes DRT to just hang and keep spewing that message. :) 286474 14 eric 2010-09-28 12:54:05 -0700 To fully fix this, we should figure out what other browsers do with this value. Should the SVG JS bindings be throwing an exception when trying to set this? Ignoring the value? Respecting it, but then the graphics layer should be ignoring it? 286511 15 ap 2010-09-28 13:57:21 -0700 Firefox ignores, and logs an error to debug console. 286584 16 eric 2010-09-28 15:34:42 -0700 I wonder what FF does if you try and animate the viewBox to an illegal value. 319807 17 lforschler 2010-12-09 17:13:58 -0800 <rdar://problem/8752858> 610876 18 rniwa 2012-04-26 13:12:38 -0700 This security bug has been open since September 23rd, 2010 :( 610886 19 inferno 2012-04-26 13:20:42 -0700 We are not tracking this in chromium. i reverified on asan linux, drt, it does not crash. and not on chrome mac either. do we need to tweak the repro for chromium ? ccing our new SVG experts :) 610889 20 eric 2012-04-26 13:21:37 -0700 It's not a security bug. :) And cross_fuzz is now public: http://lcamtuf.blogspot.com/2011/01/announcing-crossfuzz-potential-0-day-in.html 610892 21 pdr 2012-04-26 13:25:24 -0700 (In reply to comment #19) > We are not tracking this in chromium. i reverified on asan linux, drt, it does not crash. and not on chrome mac either. do we need to tweak the repro for chromium ? ccing our new SVG experts :) Chromium has moved from CG to Skia so Chromium won't hit this. Still, looks like a simple enough bug--let me take a look! 610911 22 rniwa 2012-04-26 13:35:35 -0700 Current stack trace: #0 0x7fff814f70b6 in __kill #1 0x7fff815979f6 in abort #2 0x7fff815849bc in __assert_rtn #3 0x7fff883718a5 in crossing_count #4 0x7fff8837150b in path_evaluate_level #5 0x7fff88371375 in path_get_expected_outside_orientation #6 0x7fff88371329 in path_fix_orientation #7 0x7fff88370daa in CGPathCreateByNormalizingGlyphPath #8 0x7fff88370ba2 in CGFontCreateGlyphPath #9 0x7fff838f7b17 in ripc_DrawGlyphs #10 0x7fff8831b967 in draw_glyphs #11 0x7fff8831b26e in CGContextShowGlyphsWithAdvances #12 0x1029ef0cc in WebCore::showGlyphsWithAdvances at FontMac.mm:120 #13 0x1029efb2a in WebCore::Font::drawGlyphs at FontMac.mm:250 #14 0x1029eb326 in WebCore::Font::drawGlyphBuffer at FontFastPath.cpp:422 #15 0x1029eb426 in WebCore::Font::drawSimpleText at FontFastPath.cpp:366 #16 0x1029d94d5 in WebCore::Font::drawText at Font.cpp:152 #17 0x10212dd05 in WebCore::SVGInlineTextBox::paintTextWithShadows at SVGInlineTextBox.cpp:649 #18 0x102131d33 in WebCore::SVGInlineTextBox::paintText at SVGInlineTextBox.cpp:683 #19 0x102132536 in WebCore::SVGInlineTextBox::paint at SVGInlineTextBox.cpp:326 #20 0x10211b253 in WebCore::SVGRootInlineBox::paint at SVGRootInlineBox.cpp:66 #21 0x10331de9a in WebCore::RenderLineBoxList::paint at RenderLineBoxList.cpp:262 #22 0x10324b1a9 in WebCore::RenderBlock::paintContents at RenderBlock.cpp:2715 #23 0x103252125 in WebCore::RenderBlock::paintObject at RenderBlock.cpp:2825 #24 0x10324c084 in WebCore::RenderBlock::paint at RenderBlock.cpp:2571 #25 0x10211c971 in WebCore::RenderSVGText::paint at RenderSVGText.cpp:336 #26 0x10211d6a3 in WebCore::RenderSVGContainer::paint at RenderSVGContainer.cpp:128 #27 0x10328c92c in WebCore::RenderBox::paint at RenderBox.cpp:935 #28 0x10211e919 in WebCore::RenderSVGRoot::paintReplaced at RenderSVGRoot.cpp:302 #29 0x1020a98f8 in WebCore::RenderReplaced::paint at RenderReplaced.cpp:153 #30 0x103303993 in WebCore::RenderLayer::paintLayerContents at RenderLayer.cpp:3102 #31 0x10330406b in WebCore::RenderLayer::paintLayerContentsAndReflection at RenderLayer.cpp:2974 #32 0x10330455d in WebCore::RenderLayer::paintLayer at RenderLayer.cpp:2955 #33 0x103304ed1 in WebCore::RenderLayer::paintList at RenderLayer.cpp:3183 #34 0x103303c03 in WebCore::RenderLayer::paintLayerContents at RenderLayer.cpp:3125 #35 0x10330406b in WebCore::RenderLayer::paintLayerContentsAndReflection at RenderLayer.cpp:2974 #36 0x10330455d in WebCore::RenderLayer::paintLayer at RenderLayer.cpp:2955 #37 0x103305076 in WebCore::RenderLayer::paint at RenderLayer.cpp:2772 #38 0x102a35026 in WebCore::FrameView::paintContents at FrameView.cpp:3102 #39 0x100d1dd87 in -[WebFrame(WebInternal) _drawRect:contentsOnly:] at WebFrame.mm:571 612677 23 pdr 2012-04-30 09:00:18 -0700 (In reply to comment #21) > (In reply to comment #19) > > We are not tracking this in chromium. i reverified on asan linux, drt, it does not crash. and not on chrome mac either. do we need to tweak the repro for chromium ? ccing our new SVG experts :) > > Chromium has moved from CG to Skia so Chromium won't hit this. > > Still, looks like a simple enough bug--let me take a look! "Simple enough", ha! nothing of the sort.. As Niko pointed out, some types (not just attributes) can be designated as read only where modification should throw a DOM modification exception. http://www.w3.org/TR/SVG/types.html#InterfaceSVGRect http://www.w3.org/TR/SVG/types.html#InterfaceSVGNumberList http://www.w3.org/TR/SVG/types.html#InterfaceSVGLength http://www.w3.org/TR/SVG/types.html#InterfaceSVGLengthList http://www.w3.org/TR/SVG/types.html#InterfaceSVGAngle We correctly handle some of these cases but not all. I think we could add a readonly flag to one of the svg/properties, and throw an exception on modification, similar to SVGListProperty::canAlterList(). Another option would be to introduce SVGReadOnlyAnimatableRect and change the IDLs, but I'm not a fan of that. 612849 24 eric 2012-04-30 12:07:53 -0700 You're suggesting that these values are not "readonly" in the JavaScript sense, but rather dynamically readonly based on how it's being used by other IDLs? 612873 25 pdr 2012-04-30 12:32:32 -0700 (In reply to comment #24) > You're suggesting that these values are not "readonly" in the JavaScript sense, but rather dynamically readonly based on how it's being used by other IDLs? I think that's correct--let me explain further: Consider the SVG spec snippet for SVGLengthList: "An SVGLengthList object can be designated as read only, which means that attempts to modify the object will result in an exception being thrown". From my understanding of the IDL spec, calling SVGLengthList::clear() would be allowed on a readonly property, but not on a readonly property designated as readonly in the SVG spec. Does that clarify things? 613416 26 eric 2012-05-01 08:26:53 -0700 It sounds like these various tear-off objects (at least their wrappers) need some sort of readonly method. Ideally one which is shared between these tearoffs. Should be easy enough to implement, and hopefully not too expensive. 2209706 27 karlcow 2026-05-11 07:13:13 -0700 Chrome: attachment.cgi?id=68685:13 Uncaught TypeError: Failed to set the 'width' property on 'SVGRect': The provided float value is non-finite. at attachment.cgi?id=68685:13:25 Firefox Navigated to https://bug-46427-attachments.webkit.org/attachment.cgi?id=68685 Uncaught TypeError: can't access property "x", s.viewBox.baseVal is null <anonymous> https://bug-46427-attachments.webkit.org/attachment.cgi?id=68685:11 attachment.cgi:11:1 <anonymous> https://bug-46427-attachments.webkit.org/attachment.cgi?id=68685:11 GET https://bug-46427-attachments.webkit.org/favicon.ico [HTTP/2 404 0ms] ​ Safari [Error] TypeError: The provided value is non-finite Global Code (attachment.cgi:13) It doesn't crash. 68614 2010-09-23 16:21:07 -0700 2010-09-24 08:40:56 -0700 serialized DOM sanitized.svg text/plain 1596 ap //48ACEARABPAEMAVABZAFAARQAgAHMAdgBnACAAUABVAEIATABJAEMAIAAiAC0ALwAvAFcAMwBD AC8ALwBEAFQARAAgAFMAVgBHACAAMQAuADEALwAvAEUATgAiACAAIgBoAHQAdABwADoALwAvAHcA dwB3AC4AdwAzAC4AbwByAGcALwBHAHIAYQBwAGgAaQBjAHMALwBTAFYARwAvADEALgAxAC8ARABU AEQALwBzAHYAZwAxADEALgBkAHQAZAAiAD4ACgA8AHMAdgBnACAAeABtAGwAbgBzAD0AIgBoAHQA dABwADoALwAvAHcAdwB3AC4AdwAzAC4AbwByAGcALwAyADAAMAAwAC8AcwB2AGcAIgAgAHcAaQBk AHQAaAA9ACIAbgBhAG4AJQAiACAAaABlAGkAZwBoAHQAPQAiADEAMAAwACUAIgAgAHYAZQByAHMA aQBvAG4APQAiADEALgAxACIAIAB5AD0AIgBuAGEAbgAiACAAdgBpAGUAdwBCAG8AeAA9ACIAMAAu ADAAMAAwADAAMAAwACAAMAAuADAAMAAwADAAMAAwACAAaQBuAGYAIABuAGEAbgAiACAAcAByAGUA cwBlAHIAdgBlAEEAcwBwAGUAYwB0AFIAYQB0AGkAbwA9ACIAeABNAGkAZABZAE0AaQBkACAAbQBl AGUAdAAiACAAYwBvAG4AdABlAG4AdABTAGMAcgBpAHAAdABUAHkAcABlAD0AIgBwAGkAbgBrACIA IAB4AD0AIgAwACIAPgAKADwAZABlAGYAcwA+AAoAPABmAGkAbAB0AGUAcgAgAGkAZAA9ACIARQBy AG8AZABlACIAIABzAHQAeQBsAGUAPQAiACIAPgBJAG4AZgBpAG4AaQB0AHkAPABmAGUATQBvAHIA cABoAG8AbABvAGcAeQAgAG8AcABlAHIAYQB0AG8AcgA9ACIAZQByAG8AZABlACIAIABpAG4APQAi AFMAbwB1AHIAYwBlAEcAcgBhAHAAaABpAGMAIgAgAHIAYQBkAGkAdQBzAD0AIgAxACIALwA+AAoA PAAvAGYAaQBsAHQAZQByAD4ACgA8AGYAaQBsAHQAZQByACAAaQBkAD0AIgBEAGkAbABhAHQAZQAi ACAAYgBhAHMAZQA9ACIASQBuAGYAaQBuAGkAdAB5ACIAIAA+AAoACQA8AGYAZQBNAG8AcgBwAGgA bwBsAG8AZwB5ACAAbwBwAGUAcgBhAHQAbwByAD0AIgBkAGkAbABhAHQAZQAiACAAaQBuAD0AIgBT AG8AdQByAGMAZQBHAHIAYQBwAGgAaQBjACIAIAByAGEAZABpAHUAcwA9ACIAMwAiAC8APgAKADwA LwBmAGkAbAB0AGUAcgA+AAoAPAAvAGQAZQBmAHMAPgAKAAoAPABnACAAZQBuAGEAYgBsAGUALQBi AGEAYwBrAGcAcgBvAHUAbgBkAD0AIgBuAGUAdwAiAD4ACgA8AGcAIABmAG8AbgB0AC0AZgBhAG0A aQBsAHkAPQAiAFYAZQByAGQAYQBuAGEAIgAgAGYAbwBuAHQALQBzAGkAegBlAD0AIgA1ADAAIgAg AHMAdAByAG8AawBlAD0AIgByAGUAZAAiACAAcwB0AHIAbwBrAGUALQB3AGkAZAB0AGgAPQAiADQA IgA+AAoACQA8AHQAZQB4AHQAIAB4AD0AIgA1ADAAIgAgAHkAPQAiADYAMAAiAD4AVABhAHIAZwBl AHQAPAAvAHQAZQB4AHQAPgAKAAkAPAB0AGUAeAB0ACAAeAA9ACIANQAwACIAIAB5AD0AIgAxADIA MAAiACAAZgBpAGwAdABlAHIAPQAiAHUAcgBsACgAIwBFAHIAbwBkAGUAKQAiAD4AVABhAHIAZwBl AHQAPAAvAHQAZQB4AHQAPgAKAAkAPAB0AGUAeAB0ACAAeAA9ACIANQAwACIAIAB5AD0AIgAxADgA MAAiACAAZgBpAGwAdABlAHIAPQAiAHUAcgBsACgAIwBEAGkAbABhAHQAZQApACIAPgBUAGEAcgBn AGUAdAA8AC8AdABlAHgAdAA+AAoAPAAvAGcAPgAKADwALwBnAD4ACgAKAAoAPAAvAHMAdgBnAD4A 68685 2010-09-24 08:40:56 -0700 2010-09-24 08:40:56 -0700 test case (will crash) viewbox-crash.svg image/svg+xml 504 ap PCFET0NUWVBFIHN2ZyBQVUJMSUMgIi0vL1czQy8vRFREIFNWRyAxLjEvL0VOIiAiaHR0cDovL3d3 dy53My5vcmcvR3JhcGhpY3MvU1ZHLzEuMS9EVEQvc3ZnMTEuZHRkIj4KPHN2ZyB4bWxucz0iaHR0 cDovL3d3dy53My5vcmcvMjAwMC9zdmciID4KCjxnIGZvbnQtZmFtaWx5PSJWZXJkYW5hIiBmb250 LXNpemU9IjUwIiBzdHJva2U9InJlZCIgc3Ryb2tlLXdpZHRoPSI0Ij4KCTx0ZXh0IHg9IjUwIiB5 PSI2MCI+VGFyZ2V0PC90ZXh0PgoJPHRleHQgeD0iNTAiIHk9IjEyMCI+VGFyZ2V0PC90ZXh0PgoJ PHRleHQgeD0iNTAiIHk9IjE4MCI+VGFyZ2V0PC90ZXh0Pgo8L2c+CjxzY3JpcHQ+CnZhciBzID0g ZG9jdW1lbnQuZG9jdW1lbnRFbGVtZW50OwpzLnZpZXdCb3guYmFzZVZhbC54ID0gMDsKcy52aWV3 Qm94LmJhc2VWYWwueSA9IDA7CnMudmlld0JveC5iYXNlVmFsLndpZHRoID0gSW5maW5pdHk7CnMu dmlld0JveC5iYXNlVmFsLmhlaWdodCA9IE5hTjsKPC9zY3JpcHQ+Cgo8L3N2Zz4K