779669 - nsCanvasRenderingContext2DAzure::GetMozDash does not set the error result

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

DashArrayToJSVal can fail, but we just drop the error instead of passing it along to |error|.  This is bad because when DashArrayToJSVal fails, mozDash will remain uninitialized.

3570 JS::Value
3571 nsCanvasRenderingContext2DAzure::GetMozDash(JSContext* cx, ErrorResult& error)
3572 {
3573   JS::Value mozDash;
3574   DashArrayToJSVal(CurrentState().dash, cx, &mozDash);
3575   return mozDash;
3576 }

This bug was found by Cindy Rubio González <[e-mail]>'s error propagation analysis.  I marked it s-s because it I'm not sure what sort of badness can happen from uninitialized variables.

This is a regression from bug 762652, which introduced this function.

Comment 1

[Andrew McCreight [:mccr8]]

The fix is trivial, so I can patch it if peterv or bz would please give it a security rating, if needed.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: catch the error, untested

Comment 3

[Andrew McCreight [:mccr8]]

Looking over the code, I'm not sure if there's any way for DashArrayToJSVal to actually fail, outside of an OOM, so maybe this isn't that bad.  We're just exporting previously sanitized data.  Of course, if there's a mismatch between the checking done when putting into the array and when taking out then you could hit errors.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 648158 [details] [diff] [review]
catch the error, untested

This had a clean try run on Windows.
https://tbpl.mozilla.org/?tree=Try&rev=80c4ccae4a31

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 648158 [details] [diff] [review]
catch the error, untested

Review of attachment 648158 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +3573,5 @@
>    JS::Value mozDash;
> +  nsresult rv = DashArrayToJSVal(CurrentState().dash, cx, &mozDash);
> +  if (NS_FAILED(rv)) {
> +    error.Throw(rv);
> +  }

Just do:

  error = DashArrayToJSVal(CurrentState().dash, cx, &mozDash);

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: catch the error

Thanks!  There was a comment that said this was for backwards compatibility, which is why I avoided it, but it is a lot nicer this way.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc18da2fd18c

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc18da2fd18c

Comment 8

[Ben Liblit]

Would someone please add "[uwisc-analysis]" to the Whiteboard list for this bug?  We've been using that elsewhere to tag bugs discovered by Cindy Rubio González <[e-mail]>'s error propagation analysis.  I'd add it here myself, but I don't have that power.  Thanks!

Comment 9

[Andrew McCreight [:mccr8]]

I don't think it will really be possible to write a test case for this.

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 649277 [details] [diff] [review]
catch the error

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 762652
User impact if declined: possible security problems
Testing completed (on m-c, etc.): it has been on m-c for a few days
Risk to taking this patch (and alternatives if risky): should be low, it only propagates an error, and there are likely to be few errors
String or UUID changes made by this patch: none

Comment 11

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b7b13e997cfd

Comment 12

[Boris Zbarsky [:bzbarsky]]

For future reference, the operator=(nsresult) is in fact for backwards compat.  The right way to do new things is probably to make DashArrayToJSVal take an ErrorResult out param.