Closed Bug 627302 Opened 9 years ago Closed 9 years ago

UMR [@ nsSVGMarkerElement::SetOrientToAngle] with InstallTrigger


(Core :: SVG, defect)

Not set





(Reporter: jruderman, Assigned: mrbkap)


(Blocks 2 open bugs)


(Keywords: testcase, valgrind, Whiteboard: [sg:low] fixed-in-tracemonkey)


(3 files, 1 obsolete file)

Attached file testcase (obsolete) —
document.createElementNS("", "marker")

triggers a UMR in nsSVGMarkerElement::SetOrientToAngle.

I suspect this is XPConnect's fault for letting something that isn't (and doesn't even claim to be!?) an nsIDOMSVGAngle into nsSVGMarkerElement::SetOrientToAngle (cf bug 503926).

For comparison,
  .setOrientToAngle({})             -->  NS_ERROR_DOM_SVG_WRONG_TYPE_ERR
  .setOrientToAngle(document)       -->  NS_ERROR_XPC_BAD_CONVERT_JS
  .setOrientToAngle(InstallTrigger) -->  goes through, causes trouble
Attached file valgrind's complaint
Summary: UMR [@ nsSVGMarkerElement::SetOrientToAngle] → UMR [@ nsSVGMarkerElement::SetOrientToAngle] with InstallTrigger
Attached file testcase 2
InstallTrigger isn't special because it's a chrome object.  It's special because it has a list of exposed properties, causing other gets to throw (which becomes a non-NS_OK rv).

Here's a saner testcase.
Attachment #505363 - Attachment is obsolete: true
The trick is that InstallTrigger throws when you try to access value on it. |{ get value() { throw 42 } }| would also cause the UMR.

The fix is to check the return value of GetValue() and not access the out parameter if we throw. Also note bug 543613 which would allow us to assume more about the implementation of the nsIDOMSVGAngle that's being passed in to us.
Assignee: nobody → mrbkap
Component: XPConnect → SVG
QA Contact: xpconnect → general
Attachment #505606 - Flags: review? → review+
Opening this bug up as this is not a security bug, we're simply just using an uninitialized float as the value for the angle here, which can cause wrong rendering, but nothing worse than that.
Group: core-security
Comment on attachment 505606 [details] [diff] [review]
Proposed fix

And we might as well take this fix, there's really not much of a risk involved here, and we have a patch as a result of investigating the severity here...
Attachment #505606 - Flags: approval2.0+
I think the uninitialized value can make be seen by script, so it's potential information disclosure, slightly worse than just wrong rendering.
Whiteboard: [sg:low]
Whiteboard: [sg:low] → [sg:low] fixed-in-tracemonkey
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.