Closed
Bug 779669
Opened 12 years ago
Closed 12 years ago
nsCanvasRenderingContext2DAzure::GetMozDash does not set the error result
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla17
Tracking | Status | |
---|---|---|
firefox14 | --- | unaffected |
firefox15 | --- | unaffected |
firefox16 | --- | fixed |
firefox17 | --- | fixed |
firefox-esr10 | --- | unaffected |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
(Keywords: csectype-uninitialized, regression, sec-moderate, Whiteboard: [uwisc-analysis][advisory-tracking-][qa-])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mccr8
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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 <crubio@cs.wisc.edu>'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.
Assignee | ||
Updated•12 years ago
|
status-firefox16:
--- → affected
status-firefox17:
--- → affected
Assignee | ||
Comment 1•12 years ago
|
||
The fix is trivial, so I can patch it if peterv or bz would please give it a security rating, if needed.
Assignee: nobody → continuation
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
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.
Assignee | ||
Comment 4•12 years ago
|
||
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
Attachment #648158 -
Flags: review?(peterv)
Assignee | ||
Updated•12 years ago
|
status-firefox-esr10:
--- → unaffected
status-firefox14:
--- → unaffected
status-firefox15:
--- → unaffected
Comment 5•12 years ago
|
||
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);
Attachment #648158 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 6•12 years ago
|
||
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
Attachment #648158 -
Attachment is obsolete: true
Attachment #649277 -
Flags: review+
Comment 7•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 8•12 years ago
|
||
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 <crubio@cs.wisc.edu>'s error propagation analysis. I'd add it here myself, but I don't have that power. Thanks!
Assignee | ||
Updated•12 years ago
|
Whiteboard: [uwisc-analysis]
Assignee | ||
Comment 9•12 years ago
|
||
I don't think it will really be possible to write a test case for this.
Flags: in-testsuite? → in-testsuite-
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 10•12 years ago
|
||
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
Attachment #649277 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Attachment #649277 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 11•12 years ago
|
||
Comment 12•12 years ago
|
||
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.
Updated•12 years ago
|
Whiteboard: [uwisc-analysis] → [uwisc-analysis][advisory-tracking-]
Whiteboard: [uwisc-analysis][advisory-tracking-] → [uwisc-analysis][advisory-tracking-][qa-]
Updated•12 years ago
|
Group: core-security
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•