Bug 44610 - Malformed SVG causes crash in updateContainerOffset
Summary: Malformed SVG causes crash in updateContainerOffset
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P1 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 51868
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-25 07:31 PDT by brondaire
Modified: 2011-01-05 05:15 PST (History)
7 users (show)

See Also:


Attachments
Viewing the attached SVG crashes Safari/Chrome (1.03 KB, image/svg+xml)
2010-08-25 07:31 PDT, brondaire
no flags Details
Patch (5.17 KB, patch)
2010-12-27 01:55 PST, Leo Yang
krit: review-
Details | Formatted Diff | Diff
Revised patch (5.32 KB, patch)
2010-12-27 18:28 PST, Leo Yang
no flags Details | Formatted Diff | Diff
Revised patch version 2 (5.35 KB, patch)
2011-01-03 18:20 PST, Leo Yang
no flags Details | Formatted Diff | Diff
Revised patch version 3 (8.80 KB, patch)
2011-01-05 00:16 PST, Leo Yang
krit: review-
krit: commit-queue-
Details | Formatted Diff | Diff
Revised patch version 4 (8.80 KB, patch)
2011-01-05 01:38 PST, Leo Yang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description brondaire 2010-08-25 07:31:48 PDT
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.
Comment 1 Leo Yang 2010-12-27 01:55:19 PST
Created attachment 77486 [details]
Patch
Comment 2 Dirk Schulze 2010-12-27 04:15:45 PST
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 ;-)
Comment 3 Nikolas Zimmermann 2010-12-27 04:43:36 PST
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>
Comment 4 Leo Yang 2010-12-27 18:28:31 PST
Created attachment 77531 [details]
Revised patch
Comment 5 Eric Seidel (no email) 2010-12-28 14:07:30 PST
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.
Comment 6 Dirk Schulze 2010-12-28 14:47:03 PST
(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?
Comment 7 Leo Yang 2011-01-03 18:20:50 PST
Created attachment 77862 [details]
Revised patch version 2
Comment 8 Dirk Schulze 2011-01-04 00:51:24 PST
Comment on attachment 77862 [details]
Revised patch version 2

LGTM. r=me
Comment 9 WebKit Commit Bot 2011-01-04 01:11:43 PST
Comment on attachment 77862 [details]
Revised patch version 2

Clearing flags on attachment: 77862

Committed r74960: <http://trac.webkit.org/changeset/74960>
Comment 10 WebKit Commit Bot 2011-01-04 01:11:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 WebKit Review Bot 2011-01-04 01:35:23 PST
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
Comment 12 Leo Yang 2011-01-04 02:03:29 PST
(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.
Comment 13 Leo Yang 2011-01-04 02:40:43 PST
(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.
Comment 14 Leo Yang 2011-01-05 00:16:55 PST
Created attachment 77978 [details]
Revised patch version 3
Comment 15 Dirk Schulze 2011-01-05 00:43:05 PST
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;
Comment 16 Dirk Schulze 2011-01-05 00:44:46 PST
reopen bug
Comment 17 Leo Yang 2011-01-05 01:38:54 PST
Created attachment 77983 [details]
Revised patch version 4
Comment 18 Dirk Schulze 2011-01-05 01:54:21 PST
Comment on attachment 77983 [details]
Revised patch version 4

r=me
Comment 19 WebKit Commit Bot 2011-01-05 04:11:34 PST
Comment on attachment 77983 [details]
Revised patch version 4

Clearing flags on attachment: 77983

Committed r75059: <http://trac.webkit.org/changeset/75059>
Comment 20 WebKit Commit Bot 2011-01-05 04:11:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 WebKit Review Bot 2011-01-05 05:15:00 PST
http://trac.webkit.org/changeset/75059 might have broken Leopard Intel Debug (Tests)