627302 - UMR [@ nsSVGMarkerElement::SetOrientToAngle] with InstallTrigger

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

document.createElementNS("http://www.w3.org/2000/svg", "marker")
  .setOrientToAngle(InstallTrigger)

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

Comment 1

[Jesse Ruderman]

Attachment: valgrind's complaint

Comment 2

[Jesse Ruderman]

Attachment: 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.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

Comment 5

[Johnny Stenback (:jst)]

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.

Comment 6

[Johnny Stenback (:jst)]

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...

Comment 7

[Jesse Ruderman]

I think the uninitialized value can make be seen by script, so it's potential information disclosure, slightly worse than just wrong rendering.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/68931522981d

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/68931522981d