Summary: | SVGRootInlineBox::buildTextChunks can do an invalid downcast | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Haas <myrdred> | ||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | ||||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Jonathan Haas
2008-05-02 17:19:55 PDT
Created attachment 20937 [details]
patch
proposed patch
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.
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 on attachment 20969 [details]
improved patch
Assuming you ran the tests
and no new regressions/crashes occur, r=me
|