Bug 44610 – Malformed SVG causes crash in updateContainerOffset

• Component: SVG
• Status: RESOLVED FIXED

Created attachment 65416 [details]
Viewing the attached SVG crashes Safari/Chrome

Opening the attached SVG file causes crashes both in Safari and Google Chrome. Quick stack trace from OS X 10.6.5 (10H529), Safari Version 5.0.1 (6533.17.8) follows:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
0x00007fff8714d312 in WebCore::updateContainerOffset ()
(gdb) bt
#0  0x00007fff8714d312 in WebCore::updateContainerOffset ()
#1  0x00007fff8714d2d8 in WebCore::updateContainerOffset ()
#2  0x00007fff8714d2d8 in WebCore::updateContainerOffset ()
#3  0x00007fff8714d2d8 in WebCore::updateContainerOffset ()
#4  0x00007fff8714d2d8 in WebCore::updateContainerOffset ()
#5  0x00007fff8714d2d8 in WebCore::updateContainerOffset ()
#6  0x00007fff8714d2d8 in WebCore::updateContainerOffset ()
#7  0x00007fff8714d2d8 in WebCore::updateContainerOffset ()
#8  0x00007fff8714d0f0 in WebCore::SVGUseElement::updateContainerOffsets ()
#9  0x00007fff8714bf3a in WebCore::SVGUseElement::buildShadowAndInstanceTree ()
#10 0x00007fff8714b709 in WebCore::RenderSVGShadowTreeRootContainer::updateFromElement ()
#11 0x00007fff86bf07d1 in WebCore::ContainerNode::dispatchPostAttachCallbacks ()
#12 0x00007fff86ae25ff in WebCore::ContainerNode::resumePostAttachCallbacks ()
#13 0x00007fff86ae85ed in WebCore::Element::attach ()
#14 0x00007fff86c31be4 in WebCore::ContainerNode::appendChild ()
#15 0x00007fff86d62732 in WebCore::XMLTokenizer::insertErrorMessageBlock ()
#16 0x00007fff86bbdb68 in WebCore::XMLTokenizer::end ()
#17 0x00007fff86ad05f2 in WebCore::DocumentWriter::endIfNotLoadingMainResource ()
#18 0x00007fff86b899ab in WebCore::FrameLoader::finishedLoading ()
#19 0x00007fff86b898f0 in WebCore::MainResourceLoader::didFinishLoading ()
#20 0x00007fff8249938c in _NSURLConnectionDidFinishLoading ()
#21 0x00007fff80206646 in URLConnectionClient::_clientDidFinishLoading ()
#22 0x00007fff8026bd16 in URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload ()
#23 0x00007fff8026bf82 in URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload ()
#24 0x00007fff801f2cbf in URLConnectionClient::processEvents ()
#25 0x00007fff801f2a9c in MultiplexerSource::perform ()
#26 0x00007fff8179c5f1 in __CFRunLoopDoSources0 ()
#27 0x00007fff8179a7e9 in __CFRunLoopRun ()
#28 0x00007fff81799faf in CFRunLoopRunSpecific ()
#29 0x00007fff8503991a in RunCurrentEventLoopInMode ()
#30 0x00007fff8503971f in ReceiveNextEventCommon ()
#31 0x00007fff850395d8 in BlockUntilNextEventMatchingListInMode ()
#32 0x00007fff83ff3e64 in _DPSNextEvent ()
#33 0x00007fff83ff37a9 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#34 0x0000000100015938 in ?? ()
#35 0x00007fff83fb948b in -[NSApplication run] ()
#36 0x00007fff83fb21a8 in NSApplicationMain ()
#37 0x0000000100009804 in ?? ()

Does not appear to be exploitable.

[Leo Yang]

Created attachment 77486 [details]
Patch

[Dirk Schulze]

Comment on attachment 77486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77486&action=review

LGTM. But reviewing the patch gives following error: Can't expand. Unable to apply patch to tip of tree.
Can you check if it is still possible to apply the patch to tot?

And a short snippet left

> LayoutTests/svg/dom/recursive-use-expected.txt:1
> +Survived the crash!

Shouldn't it be. "Did not crash!"? IIRC we don't want a crash at all ;-)

[Nikolas Zimmermann]

Comment on attachment 77486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77486&action=review

Good approach, r- for a few nitpicks:

> LayoutTests/ChangeLog:7
> +        Test case for recursive svg <use>.
> +        This test case passes if no crash occurs.
> +        https://bugs.webkit.org/show_bug.cgi?id=44610

This text should go below the link to the bug report, instead the bug title should be above the link:

<bug title>
https://bugs.webkit.org/...

<bug description>

> LayoutTests/ChangeLog:10
> +        * svg/dom/recursive-use-expected.txt: Added.
> +        * svg/dom/recursive-use.svg: Added.

I think the testcase should rather go into svg/custom/, we only use svg/dom/ for tests that access any DOM attribute/props.

> WebCore/ChangeLog:9
> +        We should check recursive <use> at the begining of
> +        WebCore::SVGUseElement::buildInstanceTree instead
> +        of at the end of it because the target element's
> +        children may cause infinite recursive <use>.
> +        https://bugs.webkit.org/show_bug.cgi?id=44610

This text should go below the link to the bug report, instead the bug title should be above the link:

<bug title>
https://bugs.webkit.org/...

<bug description>

[Leo Yang]

Created attachment 77531 [details]
Revised patch

[Eric Seidel (no email)]

Comment on attachment 77531 [details]
Revised patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77531&action=review

> LayoutTests/svg/custom/recursive-use.svg:10
> +<text>Don't crash!</text>

PASS would be better text here.

[Dirk Schulze]

(In reply to comment #5)
> (From update of attachment 77531 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77531&action=review
> 
> > LayoutTests/svg/custom/recursive-use.svg:10
> > +<text>Don't crash!</text>
> 
> PASS would be better text here.

Hm. Indeed. Maybe "PASS. Test didn't crash." Leo can you upload a new patch please, since you do not have commit rights yet?

[Leo Yang]

Created attachment 77862 [details]
Revised patch version 2

[Dirk Schulze]

Comment on attachment 77862 [details]
Revised patch version 2

LGTM. r=me

[WebKit Commit Bot]

Comment on attachment 77862 [details]
Revised patch version 2

Clearing flags on attachment: 77862

Committed r74960: <http://trac.webkit.org/changeset/74960>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74960 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
svg/custom/use-nested-children.svg

[Leo Yang]

(In reply to comment #11)
> http://trac.webkit.org/changeset/74960 might have broken GTK Linux 64-bit Debug
> The following tests are not passing:
> svg/custom/use-nested-children.svg

This case passes. Reverified based on 74962.

[Leo Yang]

(In reply to comment #12)
> (In reply to comment #11)
> > http://trac.webkit.org/changeset/74960 might have broken GTK Linux 64-bit Debug
> > The following tests are not passing:
> > svg/custom/use-nested-children.svg
> 
> This case passes. Reverified based on 74962.

It seems hitting an assertion. Testing with debug build.(In reply to comment #12)
> (In reply to comment #11)
> > http://trac.webkit.org/changeset/74960 might have broken GTK Linux 64-bit Debug
> > The following tests are not passing:
> > svg/custom/use-nested-children.svg
> 
> This case passes. Reverified based on 74962.

It seems hitting an assertion. Testing with debug build.

[Leo Yang]

Created attachment 77978 [details]
Revised patch version 3

[Dirk Schulze]

Comment on attachment 77978 [details]
Revised patch version 3

View in context: https://bugs.webkit.org/attachment.cgi?id=77978&action=review

just a snippet

> WebCore/svg/SVGUseElement.cpp:726
> -        handleDeepUseReferencing(static_cast<SVGUseElement*>(target), targetInstance, foundProblem);
> +    if (targetHasUseTag && newTarget) {
> +        RefPtr<SVGElementInstance> newInstance = SVGElementInstance::create(this, newTarget);
> +        SVGElementInstance* newInstancePtr = newInstance.get();
> +        targetInstance->appendChild(newInstance.release());
> +        buildInstanceTree(newTarget, newInstancePtr, foundProblem);
> +    }

should be
if (!targetHasUseTag || !newTarget)
    return;

[Dirk Schulze]

reopen bug

[Leo Yang]

Created attachment 77983 [details]
Revised patch version 4

[Dirk Schulze]

Comment on attachment 77983 [details]
Revised patch version 4

r=me

[WebKit Commit Bot]

Comment on attachment 77983 [details]
Revised patch version 4

Clearing flags on attachment: 77983

Committed r75059: <http://trac.webkit.org/changeset/75059>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/75059 might have broken Leopard Intel Debug (Tests)