991400 - SVGEllipseElement.cpp instantiates gfxPath (which is refcounted) on the stack

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGEllipseElement.cpp#101

96 SVGEllipseElement::ConstructPath(gfxContext *aCtx)
97 {
98   if (!aCtx->IsCairo()) {
99     RefPtr<Path> path = BuildPath();
100     if (path) {
101       gfxPath gfxpath(path);
102       aCtx->SetPath(&gfxpath);

That "gfxPath" instantiation is bogus and potentially dangerous, depending on what the SetPath() call does with it. See bug 991398 for more.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch to give gfxPath private destructor, which breaks the build due to this bug

Comment 2

[Daniel Holbert [:dholbert]]

jwatt, do you have cycles to take this?

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 4

[Jonathan Watt [:jwatt]]

This was caused by http://hg.mozilla.org/mozilla-central/rev/53ddb97448dc (bug 944704, part 3).

Comment 5

[Daniel Holbert [:dholbert]]

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

Thanks! r=me

Comment 6

[Daniel Holbert [:dholbert]]

Also: from skimming the gfxContext::SetPath() impl...
  http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxContext.cpp?rev=3c1fc4eed0f5#303
...it looks like that function doesn't retain any pointers to the passed-in stack-allocated gfxPath, fortunately.

In the Moz2D case, it may retain a refcounted pointer to a refcounted *member-var* on the gfxPath, but that should be fine.

So fortunately, I don't think this was dangerous.

 --> Setting sec-low, for "Missing best practice security controls" [from the keyword description page]

Comment 7

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31dbf8a76158

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Jonathan, could you fill the uplift request today ? (Monday) Beta 8 is going to build today. Thanks

Comment 9

[Jonathan Watt [:jwatt]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 944704
User impact if declined: sg bug
Testing completed (on m-c, etc.): been on m-c a day or so
Risk to taking this patch (and alternatives if risky): very, very low risk
String or IDL/UUID changes made by this patch: none

Comment 10

[Jonathan Watt [:jwatt]]

http://hg.mozilla.org/releases/mozilla-beta/rev/679aa869f39f
https://hg.mozilla.org/releases/mozilla-aurora/rev/e5d8af7e112d

Comment 11

[Carsten Book [:Tomcat]]

landed on central as https://hg.mozilla.org/mozilla-central/rev/31dbf8a76158

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-] due to lack of test case. Please feel free to provide one if you'd like verification, thanks.