Bug 18859 – SVGRootInlineBox::buildTextChunks can do an invalid downcast

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Jonathan Haas]

rendering/SVGRootInlineBox.cpp:1382:

            Node* node = text->element()->parent();
            if (node && node->isSVGElement())
                textContent = static_cast<SVGTextContentElement*>(node);
            ASSERT(textContent);

The problem is that the parent of the element node may not be an SVGTextContentElement. For example, in this SVG:

<svg xmlns="http://www.w3.org/2000/svg">
  <text>
    <a>Oh snap!</a>
  </text>
</svg>

...the parent node is an SVGAElement, which doesn't inherit SVGTextContentElement. To see this more clearly, replace the code above with:

            Node* node = text->element()->parent();
            if (node && node->isSVGElement()) {
                ASSERT(static_cast<SVGElement*>(node)->isTextContent());
                textContent = static_cast<SVGTextContentElement*>(node);
            }
            ASSERT(textContent);

Build, run Safari, load above SVG, earth-shattering kaboom.

[Jonathan Haas]

Created attachment 20937 [details]
patch

proposed patch

[Rob Buis]

Comment on attachment 20937 [details]
patch

The general idea seems good to me. The braces in the if/else are not needed since the blocks 
are both single line.

Please include a testcase in the patch that would crash in the old situation and work
fine with the patch.

r- because of the missing testcase.

[Jonathan Haas]

Created attachment 20969 [details]
improved patch

Removed extraneous braces. I assume the braces around the body of the while loop can stay?

There is no good test case for the original, unpatched code. The behavior of an invalid downcast is undefined and implementation-dependent. In the case of MSVC 8, the return value from a call to textContent->textLength() on the invalid pointer ends up pointing to the m_systemLanguage of SVGAElement::SVGTests. This usually produces innocuous if bogus values. I suppose I might be able to contrive a case where it forced an assert to trigger, but again, the behavior is undefined and there's no guarantee that the same behavior would result in an Xcode compilatior, or a gcc compilation, or even a different version of MSVC.

[Rob Buis]

Comment on attachment 20969 [details]
improved patch

Assuming you ran the tests
and no new regressions/crashes occur, r=me

[Rob Buis]

Landed in r32907.