Closed Bug 1322537 Opened 8 years ago Closed 7 years ago

SVG: hang & memory exhaustion inside of mozilla::gfx::Path::EnsureFlattenedPath(), with animateMotion

Categories

(Core :: SVG, defect, P3)

Product:
Core
Component:
SVG
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox53 --- affected
firefox56 --- fixed

People

(Reporter: tsmith, Assigned: kevin.hsieh)

References

(Blocks 2 open bugs)

Details

(Keywords: hang, perf, testcase, Whiteboard: [fuzzblocker])

Attachments

(5 files)

testcase 1 (WARNING, hangs your browser)
(deleted), text/html
Details
backtrace during the hang
(deleted), text/plain
Details
testcase 2 (using <path> & getTotalLength)
(deleted), text/html
Details
testcase 3 (reduced version of testcase 2)
(deleted), text/html
Details
Bug 1322537 - Increase tolerance when splitting Bezier curves to prevent hang.
(deleted), text/x-review-board-request
bas.schouten
: review+
Details
No description provided.
Summary: SVG with memory exhaustion with animate motion → SVG: memory exhaustion with animate motion
Keywords: hang
Summary: SVG: memory exhaustion with animate motion → SVG: hang & memory exhaustion, with animateMotion
Summary: SVG: hang & memory exhaustion, with animateMotion → SVG: hang & memory exhaustion inside of mozilla::gfx::Path::EnsureFlattenedPath(), with animateMotion
Attached file backtrace during the hang (deleted) —
Here's a backtrace from during the hang. We're inside of gfx::FlattenBezierCurveSegment's call to gfx::SplitBezier. I know we've had death-spirals in that code in the past, from trying to approximate a curve to a certain degree of precision and requiring zillions of tiny straight lines to do so...
Attachment #8817471 - Attachment description: test_case.html → testcase 1 (WARNING, hangs your browser)
Severity: major → critical
Priority: -- → P3
CC'ing Bas & Tom Klein who have worked on this Bezier-flattening code before (in bug 1134549).
Here's a testcase with no SMIL, using the getTotalLength() API to trigger the same GFX path-flattening code.
The path in testcase 2 is: <path id="myPath" d="M8,0l69,97m45,-17592186044414A71,23 46,0,0 16382,98Q50,10 48,72T21,0Z"/> -17592186044414 is a 14 digit number - is that supported/expected to work? We don't want to crash even if it isn't supported of course.
The reduced path <path id="myPath" d="M45,-17592186044414A71,23 46,0,0 16382,98"/> with just a single arc also causes a hang.
Attachment #8817572 - Attachment mime type: image/svg+xml → text/html
Oh, that's what Grizzly is: https://en.wikipedia.org/wiki/Fuzz_testing (I guess?). 14 digits makes more sense then :-)
(In reply to Tom Klein from comment #4) > The path in testcase 2 is: > > <path id="myPath" d="M8,0l69,97m45,-17592186044414A71,23 46,0,0 > 16382,98Q50,10 48,72T21,0Z"/> > > -17592186044414 is a 14 digit number - is that supported/expected to work? > We don't want to crash even if it isn't supported of course. you can also use e notation to have numbers as large as a float will store; something around 1e38. In theory we're supposed to use doubles but that would double our memory cost.
This bug is coming up quite frequently while fuzzing.
Flags: needinfo?(dholbert)
Whiteboard: [fuzzblocker]
Perhaps we could set some reasonable limit on how many splits we'll let ourselves make when flattening a bezier segment? e.g. maybe we could pick some arbitrary maximum loop count for this loop here: https://dxr.mozilla.org/mozilla-central/rev/6f8f10f48ace5692256efd91f011bd23054ee2ec/gfx/2d/Path.cpp#246,261 ...which is large enough for accuracy in common cases, but small enough to prevent us from letting a small testcase eat up all the memory.
Flags: needinfo?(dholbert) → needinfo?(bas)
Keywords: access, testcase-wanted → testcase
(In reply to Daniel Holbert [:dholbert] from comment #9) > Perhaps we could set some reasonable limit on how many splits we'll let > ourselves make when flattening a bezier segment? > > e.g. maybe we could pick some arbitrary maximum loop count for this loop > here: > > https://dxr.mozilla.org/mozilla-central/rev/ > 6f8f10f48ace5692256efd91f011bd23054ee2ec/gfx/2d/Path.cpp#246,261 > ...which is large enough for accuracy in common cases, but small enough to > prevent us from letting a small testcase eat up all the memory. That seems like a fine idea, in practice it's impossible to achieve sufficient accuracy for every conceivable curve. So this seems like a good solution.
Flags: needinfo?(bas)
Assignee: nobody → kevin.hsieh
Status: NEW → ASSIGNED
I added a little bit of code to increase the tolerance slightly (but multiplicatively) every time the loop that Daniel mentioned gets executed. Reproducing my code comment regarding this change: // Increase tolerance every iteration to prevent this loop from executing // too many times. This approximates the length of large curves more // roughly. In practice, aTolerance is the constant kFlatteningTolerance // which has value 0.0001. With this value, it takes 6,932 splits to double // currentTolerance (to 0.0002) and 23,028 splits to increase // currentTolerance by an order of magnitude (to 0.001). With this, the test cases do not produce any noticeable hang or excessive memory usage. Hopefully this addition doesn't affect non-pathological SVG's, but maybe Bas can comment on that. I also added the first and third testcases to layout/svg/crashtests as 1322537-1.html and 1322537-2.html (skipping the second testcase since it uses the same path as the first testcase and the same API entry point as the third testcase).
Comment on attachment 8890621 [details] Bug 1322537 - Increase tolerance when splitting Bezier curves to prevent hang. https://reviewboard.mozilla.org/r/161776/#review167118 ::: layout/svg/crashtests/1322537-2.html:11 (Diff revision 2) > +<svg> > + <path id="myPath" d="M45,-17592186044414A71,23 46,0,0 16382,98"/> > +</svg> > +<body onload="go()"> Drive-by nit: you should put the <svg> inside of (i.e. after) the <body> here.
Comment on attachment 8890621 [details] Bug 1322537 - Increase tolerance when splitting Bezier curves to prevent hang. https://reviewboard.mozilla.org/r/161776/#review167800 ::: gfx/2d/Path.cpp:260 (Diff revision 3) > * then be approximated by a quadratic equation for which the maximum > * difference from a linear approximation can be much more easily determined. > */ > BezierControlPoints currentCP = aControlPoints; > > - double t = 0; > + double t = 0, currentTolerance = aTolerance; nit: Put this on a separate line. ::: layout/svg/crashtests/1322537-1.html:1 (Diff revision 3) > +<svg> I don't see this being added to the manifest file so it won't actually be run afaict :-).
Attachment #8890621 - Flags: review?(bas) → review+
This looks ready to land (review nits addressed, successful-looking try run, with new crashtests definitely running in the try run [which I verified by inspecting crashtest log]) --> triggered autoland.
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6df0dd7948ad Increase tolerance when splitting Bezier curves to prevent hang. r=bas
https://hg.mozilla.org/mozilla-central/rev/6df0dd7948ad
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 963223
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: