579356 - Stack overflow crash [@ CalcBezLength] with SVG animateMotion or getPathSegAtLength() DOM method

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Bjoern Hoehrmann]

User-Agent:       Opera/9.80 (Windows NT 5.2; U; en) Presto/2.5.22 Version/10.51
Build Identifier: Mozilla/5.0 (Windows; Windows NT 5.2; en-US; rv:2.0b2pre) Gecko/20100715 Minefield/4.0b2pre

Firefox crashes when loading the attached SVG document. It does not crash when the animateMotion elements are removed.

Reproducible: Always

Comment 1

[Bjoern Hoehrmann]

Attachment: test case

Comment 2

[Daniel Holbert [:dholbert]]

Confirmed. I get an immediate crash, from infinite recursion in CalcBezLength inside of nsSVGPathSeg.cpp.  No crash reporter dialog, too.

That function (CalcBezLength) makes two recursive calls here:
137   if (length - dist > PATH_SEG_LENGTH_TOLERANCE) {
138       split(curve, left, right);
139       return CalcBezLength(left, numPts, split) + 
140              CalcBezLength(right, numPts, split);
141   }

PATH_SEG_LENGTH_TOLERANCE is 0.0000001f, and in this case 'split' is a function pointer to SplitCubicBezier.

Comment 3

[Daniel Holbert [:dholbert]]

Mozilla/5.0 (X11; Linux i686; en-US; rv:2.0b2pre) Gecko/20100714 Minefield/4.0b2pre

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1

This much-reduced testcase still reproduces the crash for me.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1

oops, last testcase was over-reduced & reproduced the problem in an opt build but not a debug build.

This version reproduces in both.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: backtrace when we've just entered the recursion stack

We crash inside of our second run of "nsSVGPathSegCurvetoCubicAbs::GetLength".  Here's the backtrace when we've just dipped our toes into the infinite recurision at that point.

Comment 7

[Bjoern Hoehrmann]

Attachment: further reduced test case

You can then further reduce it to just

  <svg xmlns="http://www.w3.org/2000/svg">
    <animateMotion dur="1" begin="0" path="
      M 0 0
      C 0,0 0,0
      501.208526, 390.4
      501.208543, 390.4
      0,0 0,0
    "/>
  </svg>

which still crashes for me.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Why is this security-sensitive?  Looks like a non-exploitable crash to me...

Comment 9

[Daniel Holbert [:dholbert]]

I think Bjoern was being on the safe side when filing. (thanks for that, Bjoern!)

However, given the analysis so far, I agree that this looks non-exploitable -- un-hiding.

Comment 10

[Martijn Wargers (dead)]

The crash reporter is working fine for this crash on windows.

http://crash-stats.mozilla.com/report/index/67c60d29-cd3c-4c7c-9de3-8eb5e2100716
0  	xul.dll  	CalcBezLength  	 content/svg/content/src/nsSVGPathSeg.cpp:133
1 	xul.dll 	CalcBezLength 	content/svg/content/src/nsSVGPathSeg.cpp:140
2 	xul.dll 	CalcBezLength 	content/svg/content/src/nsSVGPathSeg.cpp:140
3 	xul.dll 	CalcBezLength 	content/svg/content/src/nsSVGPathSeg.cpp:140
4 	xul.dll 	CalcBezLength 	content/svg/content/src/nsSVGPathSeg.cpp:140
5 	xul.dll 	CalcBezLength 	content/svg/content/src/nsSVGPathSeg.cpp:140
etc..
...
8543  	xul.dll  	CalcBezLength  	 content/svg/content/src/nsSVGPathSeg.cpp:140
8544 	xul.dll 	nsSVGPathSegCurvetoCubicAbs::GetLength 	content/svg/content/src/nsSVGPathSeg.cpp:692
8545 	xul.dll 	mozilla::SVGMotionSMILAnimationFunction::SetPathVerticesFromPathString 	content/svg/content/src/SVGMotionSMILAnimationFunction.cpp:323
8546 	xul.dll 	mozilla::SVGMotionSMILAnimationFunction::RebuildPathAndVerticesFromPathAttr 	content/svg/content/src/SVGMotionSMILAnimationFunction.cpp:286
8547 	xul.dll 	mozilla::SVGMotionSMILAnimationFunction::RebuildPathAndVertices 	content/svg/content/src/SVGMotionSMILAnimationFunction.cpp:355
8548 	xul.dll 	mozilla::SVGMotionSMILAnimationFunction::GetValues 	content/svg/content/src/SVGMotionSMILAnimationFunction.cpp:395
8549 	xul.dll 	nsSMILAnimationFunction::ComposeResult 	content/smil/nsSMILAnimationFunction.cpp:233
8550 	xul.dll 	nsSMILCompositor::ComposeAttribute 	content/smil/nsSMILCompositor.cpp:125
8551 	xul.dll 	DoComposeAttribute 	content/smil/nsSMILAnimationController.cpp:311
8552 	xul.dll 	nsTHashtable<nsSMILCompositor>::s_EnumStub 	obj-firefox/dist/include/nsTHashtable.h:420

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: no-animation testcase, using path.getPathSegAtLength() JS method

This isn't a bug in animateMotion specifically -- it can actually be triggered with the normal SVG path API, too, without any animation.

In particular, the "getPathSegAtDist()" DOM method can trigger the same stack, as demonstrated in this testcase.  (That method ends up calling nsSVGPathSegCurvetoCubicAbs::GetLength on the path-segment and hits the same infinite recursion described above.)

Comment 12

[Daniel Holbert [:dholbert]]

Using the latest testcase, I've confirmed that this bug goes back affects builds at least as far back as 3.5.x:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.10) Gecko/20100504 Firefox/3.5.10

So, not a recent regression -- just a new way of triggering it (animateMotion).

Comment 13

[Robert Longson [:longsonr]]

Attachment: patch

We get pretty close but never quite beat PATH_SEG_LENGTH_TOLERANCE. 

Patch just introduces a recursion limit.

Comment 14

[Robert Longson [:longsonr]]

I guess MAX_STEPS should be PRUint32 really. I can fix that on check in.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 458176 [details] [diff] [review]
patch

I'd prefer it if we didn't require the callers to specify "0 steps so far" when they invoke this method -- that makes the code harder to read.

That's easily fixed, though -- could you rename your new version of CalcBezLength to "CalcBezLengthHelper" or something, and then create a new inline function "CalcBezLength" that just calls "CalcBezLengthHelper" with steps=0?  That way the callers don't need to know about the extra argument at all.

r=dholbert with that (& with comment 14)

Comment 16

[Robert Longson [:longsonr]]

Attachment: address review comment

Comment 17

[Daniel Holbert [:dholbert]]

Looks great!

Also, I forgot to mention before -- we should also include the latest two testcases (bjoern's further-reduced one and my no-animation one) as crashtests for this bug.

Comment 18

[Daniel Holbert [:dholbert]]

Once the tree reopens, I'd be happy to land this (& watch the tree) as part of a push with a few other patches I been waiting to check in.

(Robert, let me know if you'd prefer to push it yourself.)

Comment 19

[Robert Longson [:longsonr]]

Feel free to land it Daniel, thanks.

Comment 20

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/2d187773aef4

Comment 21

[Daniel Holbert [:dholbert]]

Landed tests as: http://hg.mozilla.org/mozilla-central/rev/bc6cf1553ec7