Bug 46427 – Crash below CGContextShowGlyphsWithAdvances due to invalid viewBox

• Status: NEW
• Component: SVG

[Alexey Proskuryakov]

Created attachment 68614 [details]
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.

[Alexey Proskuryakov]

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.

(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?

[Alexey Proskuryakov]

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.

[Nikolas Zimmermann]

(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?

[Alexey Proskuryakov]

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

[Nikolas Zimmermann]

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

[Alexey Proskuryakov]

Created attachment 68685 [details]
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.

[Eric Seidel (no email)]

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?

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

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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>

[Eric Seidel (no email)]

In fact, the reduction in comment 12 causes DRT to just hang and keep spewing that message. :)

[Eric Seidel (no email)]

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?

[Alexey Proskuryakov]

Firefox ignores, and logs an error to debug console.

[Eric Seidel (no email)]

I wonder what FF does if you try and animate the viewBox to an illegal value.

[Lucas Forschler]

<rdar://problem/8752858>

[Ryosuke Niwa]

This security bug has been open since September 23rd, 2010 :(

[Abhishek Arya]

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

[Eric Seidel (no email)]

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

[Philip Rogers]

(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!

[Ryosuke Niwa]

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

[Philip Rogers]

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

[Eric Seidel (no email)]

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?

[Philip Rogers]

(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?

[Eric Seidel (no email)]

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.