614522 - SVGPathData::GetMarkerPositioningData reads uninitialised stack allocated memory

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Julian Seward [:jseward]]

It looks like SVGPathData::GetMarkerPositioningData can get to here

502:       segStartAngle = segEndAngle = AngleOfVector(segEnd - segStart);

without first assigning anything to segStart.

This leads to the following complaints, when running
layout/svg/crashtests/371563-1.xhtml

Observed on x86_64-linux and x86-win32.  Should be observable
on all platforms.


Conditional jump or move depends on uninitialised value(s)
   at 0x6323F94: AngleOfVector(gfxPoint) (gfxPoint.h:127)
   by 0x6327E94: mozilla::SVGPathData::GetMarkerPositioningData
                 (SVGPathData.cpp:491)
   by 0x62FAB43: nsSVGPathElement::GetMarkPoints (nsSVGPathElement.cpp:418)
   by 0x62894B5: nsSVGPathGeometryFrame::GetCoveredRegion
                 (nsSVGPathGeometryFrame.cpp:217)
   by 0x6286401: nsSVGOuterSVGFrame::UpdateAndInvalidateCoveredRegion
                 (nsSVGOuterSVGFrame.cpp:659)
   by 0x6292C3C: nsSVGUtils::UpdateGraphic (nsSVGUtils.cpp:692)
   by 0x628833E: nsSVGPathGeometryFrame::NotifyRedrawUnsuspended 
                 (nsSVGPathGeometryFrame.cpp:338)
   by 0x62855B8: nsSVGOuterSVGFrame::UnsuspendRedraw
                 (nsSVGOuterSVGFrame.cpp:715)
   by 0x62858E3: nsSVGOuterSVGFrame::DidReflow
                 (nsSVGOuterSVGFrame.cpp:403)
   by 0x59D3E07: nsLineLayout::ReflowFrame
                 (nsLineLayout.cpp:971)
   by 0x5968FBB: nsBlockFrame::ReflowInlineFrame
                 (nsBlockFrame.cpp:3794)
   by 0x5969AAA: nsBlockFrame::DoReflowInlineFrames
                 (nsBlockFrame.cpp:3590)
 Uninitialised value was created by a stack allocation
   at 0x6326AC6: mozilla::SVGPathData::GetMarkerPositioningData
                 (SVGPathData.cpp:453)

followed by a whole bunch of clones inside __ieee754_atan2, since
AngleOfVector passes the uninitialised values onwards there.

Comment 1

[Julian Seward [:jseward]]

ftr (so it doesn't get forgotten):
[01:29] <jwatt> sewardj: note that pathStartPoint is dead too

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 3

[Daniel Holbert [:dholbert]]

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

Looks good! Just some cleanup-ish nits -- feel free to ignore them if you see fit.

>+  // info on current [sub]path (reset every M command):
>+  gfxPoint pathStart(0, 0);

s/0/0.0/ (gfxPoint constructor takes a gfxFloat aka double)

>+  gfxPoint prevSegEnd(0, 0);

Here too.

>+  float prevSegEndAngle = 0;

0.0f

>+    // info on current segment:
>+    PRUint16 segType =
>+      SVGPathSegUtils::DecodeType(mData[i++]); // advances i to args
>+    gfxPoint &segStart = prevSegEnd, segEnd;

Nit: This would be easier to read if setEnd got its own separate declaration.

>-      cp1 = SVGPathSegUtils::IsCubicType(prevSegType) ? segStart * 2 - prevCP : segStart;
>+    {
>+      gfxPoint cp1 = SVGPathSegUtils::IsCubicType(prevSegType) ? segStart * 2 - prevCP : segStart;

More than 80 chars -- add some line-wrapping sauce. (maybe newline after "=" or "?")

>     case nsIDOMSVGPathSeg::PATHSEG_CURVETO_QUADRATIC_SMOOTH_REL:
>-      cp1 = SVGPathSegUtils::IsQuadraticType(prevSegType) ? segStart * 2 - prevCP : segStart;
>+    {
>+      gfxPoint cp1 = SVGPathSegUtils::IsQuadraticType(prevSegType) ? segStart * 2 - prevCP : segStart;

Here too.

Comment 4

[Jonathan Watt [:jwatt]]

http://hg.mozilla.org/mozilla-central/rev/998a8a41310b