656244 - Crash when a malformed SVG is included in a page [@ mozilla::imagelib::SVGDocumentWrapper::GetWidthOrHeight ]

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Aki Helin]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Firefox crashes when the attached HTML page is opened. The page includes one SVG file as an image.

Reproducible: Sometimes

Steps to Reproduce:
1. $ firefox test.html
2. reload the page a few times

Actual Results:  
Firefox crashes.

Expected Results:  
Firefox remains running.

I tried a few variants of this and only got a null deref. Opening just the SVG file causes either a circle being drawn or an error. Firefox seems to crash only when the image is included on another page.

Crash report @ https://crash-stats.mozilla.com/report/index/bp-ec7915a8-fe06-41bd-87d2-3cc042110511

Comment 1

[Aki Helin]

Attachment: svg file causing odd behavior

Comment 2

[Aki Helin]

Attachment: testcase (needs to be downloaded locally, along w/ SVG file, to work)

Comment 3

[Thomas Ahlblom]

0 	libxul.so 	mozilla::imagelib::SVGDocumentWrapper::GetWidthOrHeight 	modules/libpr0n/src/SVGDocumentWrapper.cpp:113
1 	libxul.so 	mozilla::imagelib::VectorImage::GetWidth 	modules/libpr0n/src/VectorImage.cpp:313
2 		@0xacb2f09b 	
3 	libxul.so 	nsImageFrame::UpdateIntrinsicSize 	layout/generic/nsImageFrame.cpp:309
4 	libxul.so 	nsImageFrame::EnsureIntrinsicSizeAndRatio 	layout/generic/nsImageFrame.cpp:718
5 	libxul.so 	nsImageFrame::ComputeSize 	layout/generic/nsImageFrame.cpp:746
6 	libxul.so 	nsHTMLReflowState::InitConstraints 	layout/generic/nsHTMLReflowState.cpp:1849
7 	libxul.so 	nsHTMLReflowState::Init 	layout/generic/nsHTMLReflowState.cpp:284
8 	libxul.so 	nsLineLayout::ReflowFrame 	jstl.h:350
9 	libxul.so 	nsBlockFrame::ReflowInlineFrame 	layout/generic/nsBlockFrame.cpp:3811
10 	libxul.so 	nsBlockFrame::DoReflowInlineFrames 	layout/generic/nsBlockFrame.cpp:3607
11 	libxul.so 	nsBlockFrame::ReflowInlineFrames 	layout/generic/nsBlockFrame.cpp:3466
12 	libxul.so 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2562
13 	libxul.so 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:1999
14 	libxul.so 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:1080
15 	libxul.so 	nsBlockReflowContext::ReflowBlock 	layout/generic/nsBlockReflowContext.cpp:297
16 	libxul.so 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3184
17 	libxul.so 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2506
18 	libxul.so 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:1999
19 	libxul.so 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:1080
20 	libxul.so 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:740
21 	libxul.so 	nsCanvasFrame::Reflow 	layout/generic/nsCanvasFrame.cpp:494
22 	libxul.so 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:740
23 	libxul.so 	nsHTMLScrollFrame::ReflowScrolledFrame 	layout/generic/nsGfxScrollFrame.cpp:535
24 	libxul.so 	nsHTMLScrollFrame::ReflowContents 	layout/generic/nsGfxScrollFrame.cpp:627
25 	libxul.so 	nsHTMLScrollFrame::Reflow 	layout/generic/nsGfxScrollFrame.cpp:868
26 	libxul.so 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:740
27 	libxul.so 	ViewportFrame::Reflow 	layout/generic/nsViewportFrame.cpp:293
28 	libxul.so 	PresShell::DoReflow 	layout/base/nsPresShell.cpp:7897
29 	libxul.so 	PresShell::ProcessReflowCommands 	layout/base/nsPresShell.cpp:8036
30 	libxul.so 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:4916
31 	libxul.so 	nsRefreshDriver::Notify 	layout/base/nsRefreshDriver.cpp:326
32 	libxul.so 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:428
33 	libxul.so 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:517
34 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:633
35 	libxul.so 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:250
36 	libxul.so 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
37 	libxul.so 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:219
38 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:202
39 	libxul.so 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:192
40 	libxul.so 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:220
41 		@0xb76877b3 	
42 	libxul.so 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3786
43 	firefox-bin 	main 	nsBrowserApp.cpp:158
44 	libc-2.11.2.so 	libc-2.11.2.so@0x16c75 	
45 	firefox-bin 	firefox-bin@0x1390 	
46 	firefox-bin 	Output 	nsBrowserApp.cpp:77
47 		@0x1

Comment 4

[Thomas Ahlblom]

Works for me so far (no crash, but also no image viewed).
Mozilla/5.0 (X11; Linux i686 on x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110510 Firefox/6.0a1
Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

I've also tried to view just the SVG image in some programs, but they all issue an error message similar to:
[13:54:07.797] mismatched tag. Expected: </animateColor>. @ file:///home/user1/Downloads/trigger.svg:3063

But of course, Firefox should not crash even with a malformed image file.

Comment 5

[Thomas Ahlblom]

How easy is it for you to reproduce the crash?

Aki, are you able to reproduced the crash when running Firefox in Safe Mode?
(The crash report shows that your profile doesn't have a lot of extensions, but anyway...)
https://support.mozilla.com/en-US/kb/Safe+Mode

How about with a new, empty profile?
https://support.mozilla.com/en-US/kb/Basic+Troubleshooting#w_8-make-a-new-profile

Comment 6

[Aki Helin]

The crash happens in safe mode and with a blank profile. This is mainly a fuzzing/testing machine so there shouldn't be any funny customizations.

The crash rarely happens on first load, but it almost always happens on first or second reload.

Comment 7

[Aki Helin]

Ah, this behaves like a race condition. The included test SVG file did not trigger this on another faster machine, but it is easily reproducible after adding 100 more of the repeating 11-line section. The other machine is similar to the one above (also Debian 6.0.1, i686, Firefox 4.0.1) but with Intel Core 2 Duo instead of an Atom.

Comment 8

[Thomas Ahlblom]

Reproduced with a new clean profile, pressing F5 until crash.
Build identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20100101 Firefox/4.0
bp-2594a28b-de9e-4d5f-b997-798602110511

This is on a laptop, weaker than the workstation I used in comment 4.

Comment 9

[Thomas Ahlblom]

(In reply to comment #7)
>..., but it is easily reproducible after adding 100 more of the repeating
>11-line section. 

Do you mean that you have created a test file with 101 lines of "<img src="trigger.svg">"?

Comment 10

[Aki Helin]

I meant editing just the SVG file and repeating the section:
   <script><![CDATA[SVGDoc=document;//]]></script>
   <defs id="DEF">
   <linearGradient id="rhue" x1="0" x2="100" y1="100" y2="0" gradientUnits="userSpaceOnUse">
      <stop offset="0" id="ro0" style="stop-color: red"/>
      <stop offset=".25" id="ro1" style="stop-color: blue"/>
      <stop offset=".75" id="ro2" style="stop-color: yellow"/>
      <stop offset="1" id="ro3" style="stop-color: green"/>
   </linearGradient></defs>
   <g transform="translate(0 50)">
      <ellipse cx="290" cy="200" rx="80" ry="80">
      <animateColor attri="http://www.w3.org/1999/xlink">

100 more times. 1000 didn't work for me, but with 100 this reproduced several times with 1 or 2 reloads. Apparently when there is trouble ahead, instead of a broken image picture there is a partially shown parsing error message or a blank yellow box (background of the error message?) when there is no crash.

Comment 11

[Thomas Ahlblom]

I eventually manged to crash Firefox on my powerful workstation as well, but I had to use stressapptest to make the computer a bit dizzy. :-)
Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
bp-aeb84886-49cc-4e55-9199-15b3c2110511

Comment 12

[Thomas Ahlblom]

(In reply to comment #10)
> Apparently when there is trouble ahead,
> instead of a broken image picture there is a partially shown parsing error
> message or a blank yellow box (background of the error message?) when there
> is no crash.

Interesting, I also saw the yellow box flash shortly before the crash, by estimate the size of the error message I get when I look at the image directly. I could not see any text in it, but it was visible a very short moment so I may have missed it.

Comment 13

[Aki Helin]

Alternative repro that might work better on desktop supercomputers:
 $ perl -e 'print "<svg xmlns=\"http://www.w3.org/2000/svg\" xmlns:xlink=\"http://www.w3.org/1999/xlink\">\n\n", "<defs> <linearGradient id=\"rhue\" x1=\"0\" x2=\"100\" y1=\"100\" y2=\"0\"> <stop style=\"stop-color: red\"/> </linearGradient></defs> <ellipse cx=\"290\" cy=\"200\" rx=\"80\" ry=\"80\"> <animateColor attri=\"http://www.w3.org/1999/xlink\">\n\n" x 800, "</lineargradient></svg>"' > foo.svg
 $ echo '<img src="foo.svg">' > test.html
 $ firefox test.html

That one works on my slowish computer (crashes within a few reloads), and lifting the "x 800" might help on faster machines.

Might be that just about any malformed SVG that takes the correct amount of time to parse would do it. The null crash, yellow box and partially shown error message would make sense for example if someone was trying to print the reason for the error while it was still being computed.

Comment 14

[Thomas Ahlblom]

Attachment: Screenshot right before crash

Reproduced with the original test.html and trigger.svg:
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110511 Firefox/6.0a1
bp-72442292-5d83-4deb-9966-ca01d2110511

I pressed reload/stop as an idiot in a semi-random pattern - and suddenly when I pressed stop the XML Parsing Error box was on the screen. I downloaded and installed GIMP, made the attached screenshoot, and when I pressed reload again Firefox crashed.

Comment 15

[Daniel Holbert [:dholbert]]

I haven't yet reproduced the crash, but when I use a local copy of the testcase, I do seem to hit the "XML Parsing Error" yellow rendering every single time, and that that in and of itself is unexpected/bad -- we're supposed to catch that and display the broken-image icon instead.  (I think both that and the crash have the same underlying problem.)

The code to handle that is VectorImage::OnStopRequest() (called when we've received all the data for the image file) -- that checks mSVGDocumentWrapper->ParsedSuccessfully(), which checks the image-document's root element.  If ParsedSuccessfully sees a non-<svg> root element in its document (e.g. the root node of the internal yellow XML error page), then it takes that as a sign that our SVG was malformed.

But when I hit the yellow rendering here, we end up getting a <svg> element at that point, which makes us behave as if everything is fine (and paint the "image").  I'm not sure why that's happening...

Comment 16

[Daniel Holbert [:dholbert]]

I believe bug 657191 fixed this -- in a current nightly, I can't hit the "XML Parsing Error" rendering that I previously could in comment 15.

Can anyone else reproduce in an up-to-date Nightly (mozilla-central) build?

Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110524 Firefox/6.0a1

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to comment #16)
> I believe bug 657191 fixed this -- in a current nightly, I can't hit the
> "XML Parsing Error" rendering that I previously could in comment 15

(that is to say -- I now always get a "broken image" icon instead, which is correct)

Comment 18

[Aki Helin]

I get the same crash with current nightly (6.0a1 2011-05-24) and the original testcase. Heading off to an offline vacation so I can't test with other versions for a week.

Crash report is at https://crash-stats.mozilla.com/report/index/bp-d385aa68-cadf-414d-bf9f-d4c962110525. Not sure why it shows 4.0.1 as version.

Comment 19

[Daniel Holbert [:dholbert]]

I still can't reproduce (now with a build from before bug 657191 landed), but I'm willing to believe that it's still broken.

However...
> I get the same crash with current nightly 
> Crash report is [...] Not sure why it shows 4.0.1 as version.

I take this to mean you *actually* crashed on 4.0.1... (Maybe you already had Firefox 4.0.1 open & forgot to specify -no-remote when starting your nightly build?)

Crash reports aren't generally wrong about the crashing version -- if they were, that'd be a very bad thing for our ability to track trends in crashes.

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to comment #15)
> If ParsedSuccessfully sees a non-<svg> root element in its
> document (e.g. the root node of the internal yellow XML error page), then it
> takes that as a sign that our SVG was malformed.
> 
> But when I hit the yellow rendering here, we end up getting a <svg> element
> at that point, which makes us behave as if everything is fine (and paint the
> "image").  I'm not sure why that's happening...

So when I load the attached (malformed) SVG file directly, I see a "normal" rendering (with a black circle) for a little bit, and *then* we switch to the XML Parsing error page.

In theory we're supposed to be flushing all of the parsing (and hitting any parsing errors) before we check "ParsedSuccessfully".  But it seems like in this case, given that we're seeing an <svg> root element, ParsedSuccessfully must still be seeing the pre-converted-to-error-page document somehow.

I still don't know exactly why this is happening, but it makes sense that this would be related to the document size, as suggested in comment 10.

Comment 21

[Daniel Holbert [:dholbert]]

Ah, ok - so using a current debug build, I _can_ still reproduce the yellow-error-page-instead-of-broken-image rendering that I saw in comment 15. (probably because debug builds are slower). Still no crashes, though.

So, definitely not fixed by bug 657191.

Comment 22

[Daniel Holbert [:dholbert]]

d'oh, we're definitely getting a call to nsParser::ContinueInterruptedParsing() *after* the ParsedSuccessfully check.

(And there's a call to nsExpatDriver::HandleError inside of that ContinueInterruptedParsing call, which is what switches to the error page.)

I think I initially assumed that SVGDocumentWrapper's call to ContinueInterruptedParsing would finish off all remaining parsing, but I guess it only does one more "batch"...  We may need to wrap it in some sort of "while(moreParsingToDo) {}" loop.

Marking this as security-sensitive, since this could can definitely have crashy implications, some of which could be scary (not sure yet).

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to comment #15)
> I haven't yet reproduced the crash, but when I use a local copy of the
> testcase, I do seem to hit the "XML Parsing Error" yellow rendering

BTW, it makes sense that this would particularly affect local-file-loads, because it happens when we're loading data fast enough that the parser "falls behind" and is left needing multiple ContinueInterruptedParsing batches even after we've received all the data from the file request.

Comment 24

[Daniel Holbert [:dholbert]]

So bug 599497 comment 6 added:
>+    if (!parser->IsComplete()) {
>+      parser->ContinueInterruptedParsing();

I think we just want "while" instead of "if" there.

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 26

[Daniel Holbert [:dholbert]]

This code hasn't changed since Firefox 4, so this bug affects us back that far.  I think we need this on all active release trains (for Firefox 5 and 6).

Comment 27

[Johnny Stenback (:jst)]

Not tracking a dos for 5.

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 535506 [details] [diff] [review]
fix v1

Switching review request to Henri. I'm not sure continuing interrupted parsing in a synchronous loop is something that the parser expects and will always be ok with. But I'm guessing Henri will know.

Comment 29

[Henri Sivonen (:hsivonen)]

Comment on attachment 535506 [details] [diff] [review]
fix v1

ContinueInterruptedParsing() is meant to be called from the event loop, so neither the code before the patch nor the code after the patch is OK. Instead of calling into the parser like this, I think the right way to proceed would be to let the parser finish at its own pace asynchronously and then to hook whatever code the SVG-in-img code needs to run last to code that runs around nsXMLContentSink::DidBuildModel().

I see the comment right above your code change though. If the assumptions of imgRequest::OnStopRequest are too hard to change, you might just get away with calling ContinueInterruptedParsing() in a loop assuming that SVG-in-img is guaranteed never to run scripts. However, I'm not sure that you can get away with in that case. Are the assumptions of imgRequest::OnStopRequest too hard to change?

Comment 30

[Daniel Holbert [:dholbert]]

(In reply to comment #29)
> I see the comment right above your code change though.

Just for clarity, that comment is:
>298     // A few levels up the stack, imgRequest::OnStopRequest is about to tell
>299     // all of its observers that we know our size and are ready to paint.  That
>300     // might not be true at this point, though -- so here, we synchronously
>301     // finish parsing & layout in our helper-document to make sure we can hold
>302     // up to this promise.


> If the assumptions of
> imgRequest::OnStopRequest are too hard to change, you might just get away
> with calling ContinueInterruptedParsing() in a loop assuming that SVG-in-img
> is guaranteed never to run scripts.

That's my thinking as well.  And we do indeed have that guarantee.

> Are the assumptions of imgRequest::OnStopRequest too
> hard to change?

IIRC from discussion with Joe and/or Bobby, that would require some nontrivial refactoring of imagelib code, to separate out the "received all the data" and "have image size available" conditions.  I think I remember bholley saying that this refactoring is wanted eventually, but it's non-trivial.

Henri: would you agree that the addition of the loop here is at least an improvement over what we have now? :)

Comment 31

[Daniel Veditz [:dveditz]]

Attachment: Bug Bounty non-qual

Comment 33

[LegNeato]

Not version-specific so not tracking for Firefox 6.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 535506 [details] [diff] [review]
fix v1

comment 29 looks like an r-, setting flag so no one is misled by the bug state

Comment 35

[Daniel Veditz [:dveditz]]

If it's a DoS bug should we unhide it?

Comment 36

[Henri Sivonen (:hsivonen)]

Comment on attachment 535506 [details] [diff] [review]
fix v1

(In reply to comment #30)
> Henri: would you agree that the addition of the loop here is at least an
> improvement over what we have now? :)

Yes. It's an improvement. Sorry about the delay, I thought I had already responded. Marking r+ in the "it's an improvement" sense. I can't promise that the solution still wouldn't be broken in some way that I just fail to see right now.

Comment 37

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bdd4d79f9836

Comment 38

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/bdd4d79f9836

Comment 39

[Vlad [QA]]

I was able to view the image for about one-two seconds in the tab and after that the error appeared: "XML Parsing Error: mismatched tag. Expected: </animateColor>.
Location: https://bug656244.bugzilla.mozilla.org/attachment.cgi?id=531579
Line Number 3063, Column 3:"

I've tried this on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 1.
So the issue is still reproducing for me. I don't get a Firefox crash but that error page.

Comment 40

[Robert Longson [:longsonr]]

(In reply to Vlad from comment #39)
> I was able to view the image for about one-two seconds in the tab and after
> that the error appeared: "XML Parsing Error: mismatched tag. Expected:
> </animateColor>.
> Location: https://bug656244.bugzilla.mozilla.org/attachment.cgi?id=531579
> Line Number 3063, Column 3:"

That's the expected result. The previous behaviour was a crash which was fixed.

Comment 41

[Vlad [QA]]

Considering comment39 and comment40, setting resolution to Verified Fixed on  Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0

Comment 42

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #30)
> > Are the assumptions of imgRequest::OnStopRequest too
> > hard to change?
> 
> IIRC from discussion with Joe and/or Bobby, that would require some
> nontrivial refactoring of imagelib code, to separate out the "received all
> the data" and "have image size available" conditions.

I just filed bug 704059 on doing this refactoring, BTW, to be sure it's being tracked somewhere. (Not sure if there was already a bug filed on that; if so, mine can be duped to the original.)