Bug 18859 - SVGRootInlineBox::buildTextChunks can do an invalid downcast
Summary: SVGRootInlineBox::buildTextChunks can do an invalid downcast
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2008-05-02 17:19 PDT by Jonathan Haas
Modified: 2008-05-06 10:38 PDT (History)
0 users

See Also:

patch (1.53 KB, patch)
2008-05-02 17:22 PDT, Jonathan Haas
rwlbuis: review-
Details | Formatted Diff | Diff
improved patch (1.51 KB, patch)
2008-05-05 09:20 PDT, Jonathan Haas
rwlbuis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Haas 2008-05-02 17:19:55 PDT

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

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">
    <a>Oh snap!</a>

...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()) {
                textContent = static_cast<SVGTextContentElement*>(node);

Build, run Safari, load above SVG, earth-shattering kaboom.
Comment 1 Jonathan Haas 2008-05-02 17:22:42 PDT
Created attachment 20937 [details]

proposed patch
Comment 2 Rob Buis 2008-05-03 13:39:01 PDT
Comment on attachment 20937 [details]

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.
Comment 3 Jonathan Haas 2008-05-05 09:20:45 PDT
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.
Comment 4 Rob Buis 2008-05-06 00:23:55 PDT
Comment on attachment 20969 [details]
improved patch

Assuming you ran the tests
and no new regressions/crashes occur, r=me
Comment 5 Rob Buis 2008-05-06 10:38:40 PDT
Landed in r32907.