Closed
Bug 553938
Opened 15 years ago
Closed 15 years ago
Crash [@ TypedArrayTemplate<unsigned char>::class_constructor(JSContext*, JSObject*, unsigned int, long*, long*) ]
Categories
(Core :: Graphics: Canvas2D, defect)
Core
Graphics: Canvas2D
Tracking
()
People
(Reporter: humph, Assigned: vlad)
References
()
Details
(4 keywords, Whiteboard: [sg:critical])
Crash Data
Attachments
(3 files)
|
596 bytes,
text/html
|
Details | |
|
1.75 KB,
patch
|
jorendorff
:
review+
bzbarsky
:
review+
beltzner
:
approval1.9.2.4+
|
Details | Diff | Splinter Review |
|
5.75 KB,
patch
|
jorendorff
:
review+
vlad
:
review+
bzbarsky
:
superreview+
beltzner
:
approval1.9.1.10+
|
Details | Diff | Splinter Review |
While writing some performance tests, I made a typo in my code and forgot to write .data when updating a pixel colour value. This stepped on the ImageData object itself, which then causes us to crash in putImageData:
var canvas = document.getElementById('c');
var ctx = canvas.getContext('2d');
var imgData = {data: new Array(10*10*4), width: 10, height: 10};
//
imgData[0] = 0;
ctx.putImageData(imgData, 0, 0);
I've attached a test case with the above code (click "Crash Me" to crash). See also:
http://crash-stats.mozilla.com/report/index/f69de639-dbd4-4537-8ebd-b7efa2100321
| Reporter | ||
Comment 1•15 years ago
|
||
Also a problem in 3.6.
| Reporter | ||
Comment 2•15 years ago
|
||
See also bug 553948.
Comment 3•15 years ago
|
||
The problem is that js_CreateTypedArrayWithArray is called with an Array that has length 400 but has no actual entries, so dslots is null. Since len is 400, we end up trying to dereference null when doing *src++ in TypedArrayTemplate<unsigned char>::copyFrom and crash.
If imgData.data[0] = 0 had happened, then dslots would instead be length 1, but we'd still try to read 400 entries from it and read random memory (and probably not crash). This needs to get fixed ASAP, I think. Looks like a regression from bug 532774 (or at least the JS_GetArrayLength call is blamed on that).
Updated•15 years ago
|
Assignee: nobody → vladimir
| Assignee | ||
Comment 4•15 years ago
|
||
This is fixed on tracemonkey, by bug 550351; I'm not sure what the 3.6 issue is, though.
Depends on: 550351
| Assignee | ||
Comment 5•15 years ago
|
||
Found the 1.9.2 issue -- CoerceArrayToCanvasImageData wasn't checking capacity, just length, so it had the same problem. Testing a patch now.
| Assignee | ||
Comment 6•15 years ago
|
||
Fix. This function wasn't checking dense array capacity, just length. Also added crashtests -- the patch isn't the same as on trunk, but the crashtest is valid on trunk, but needs a tracemonkey merge for it to stop crashing.
Attachment #434340 -
Flags: review?(jorendorff)
Attachment #434340 -
Flags: review?(bzbarsky)
| Assignee | ||
Comment 7•15 years ago
|
||
Adding sg:? -- this should probably go into the next 3.6 dot release.
Whiteboard: [sg:?]
Comment 8•15 years ago
|
||
Comment on attachment 434340 [details] [diff] [review]
fix and crashtest (1.9.2)
r=bzbarsky if the "bogus" test is moved to before the "some holes" test.
Attachment #434340 -
Flags: review?(bzbarsky) → review+
Updated•15 years ago
|
Attachment #434340 -
Flags: review?(jorendorff) → review+
| Assignee | ||
Comment 9•15 years ago
|
||
Comment on attachment 434340 [details] [diff] [review]
fix and crashtest (1.9.2)
Asking 1.9.2.3 approval -- 1.9.2 patch only, fixes potential crash and other issues.
Removed unnecessary part of crashtest, after discussion with bz on irc.
Attachment #434340 -
Attachment description: fix and crashtest → fix and crashtest (1.9.2)
Attachment #434340 -
Flags: approval1.9.2.3?
Comment 10•15 years ago
|
||
(In reply to comment #7)
> Adding sg:? -- this should probably go into the next 3.6 dot release.
Not a valid sg: whiteboard. Better to either assign it something or leave it blank.
Whiteboard: [sg:?]
Comment 11•15 years ago
|
||
Will be back in a jiffy to approve the patch. Does this affect 1.9.1?
Comment 12•15 years ago
|
||
Comment on attachment 434340 [details] [diff] [review]
fix and crashtest (1.9.2)
a=beltzner for 1.9.2.3
Attachment #434340 -
Flags: approval1.9.2.3? → approval1.9.2.3+
Comment 13•15 years ago
|
||
Ditto Reed -- [sg:?] buries the bug, taking it out of the "to be looked at" pile but not into an actionable bucket. Eventually we sweep unknown marks, but until then...
| Assignee | ||
Comment 14•15 years ago
|
||
Is just marking it security sensitive enough to put it into the "to be looked at" bucket? I wasn't clear on that.
Let's call it critical ... it doesn't really matter since we'll have the fix everywhere soon anyway.
Whiteboard: [sg:critical]
| Assignee | ||
Comment 16•15 years ago
|
||
This is basically ready to land, though do I want to land it right now, or wait until closer to 1.9.2.4 is ready to go out?
Comment 17•15 years ago
|
||
(In reply to comment #16)
> This is basically ready to land, though do I want to land it right now, or wait
> until closer to 1.9.2.4 is ready to go out?
Just make sure not to land the testcase until after 1.9.2.4 has shipped... :)
| Assignee | ||
Comment 18•15 years ago
|
||
landed without crashtest on 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c71e0fe28e2a
will land crashtest once 1.9.2.4 ships.
| Assignee | ||
Updated•15 years ago
|
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Keywords: testcase
Resolution: --- → FIXED
Comment 19•15 years ago
|
||
(In reply to comment #11)
> Will be back in a jiffy to approve the patch. Does this affect 1.9.1?
To answer beltzner's question from a month ago, yes, 1.9.1 crashes also.
bp-6b55a7f6-0d94-4ac1-805c-64d242100428
blocking1.9.1: --- → ?
Comment 20•15 years ago
|
||
Do we have a 1.9.1 patch?
Comment 21•15 years ago
|
||
Comment on attachment 434340 [details] [diff] [review]
fix and crashtest (1.9.2)
a=beltzner for 1.9.1.10, where reed tells me this applies cleanly
Attachment #434340 -
Flags: approval1.9.1.10+
Comment 22•15 years ago
|
||
js_CoerceArrayToCanvasImageData doesn't exist in 1.9.0 -- FF3.0.x is fine.
Updated•15 years ago
|
Flags: wanted1.9.0.x-
Comment 23•15 years ago
|
||
Comment on attachment 434340 [details] [diff] [review]
fix and crashtest (1.9.2)
It applies with fuzz 2, but that's only because patch is dumb. This bug doesn't apply to 1.9.1. The function doesn't even exist on 1.9.1.
Attachment #434340 -
Flags: approval1.9.1.10+
Updated•15 years ago
|
blocking1.9.1: ? → ---
Comment 24•15 years ago
|
||
I'm tired. This affects 1.9.1, but it's an issue with a different function.
blocking1.9.1: --- → ?
Comment 25•15 years ago
|
||
Updated•15 years ago
|
Attachment #442220 -
Flags: superreview?(bzbarsky)
Attachment #442220 -
Flags: review?(vladimir)
Attachment #442220 -
Flags: review?(jorendorff)
Comment 26•15 years ago
|
||
re comment 22: 3.0 is fine not because that named function doesn't exist but because the testcase does not crash. The call to putImageData() throws an exception.
Updated•15 years ago
|
Comment 27•15 years ago
|
||
Comment on attachment 442220 [details] [diff] [review]
possible fix for 1.9.1
Nice catch.
It seems like if (offset + count) overflows, this code will leave the dest buffer unchanged and return true. (This is true both with and without the patch.) That could be bad, depending on what the caller is using this for.
r=me regardless; checking capacity is an improvement over not checking it, for sure.
Attachment #442220 -
Flags: review?(jorendorff) → review+
Updated•15 years ago
|
Attachment #442220 -
Flags: superreview?(bzbarsky) → superreview+
| Assignee | ||
Updated•15 years ago
|
Attachment #442220 -
Flags: review?(vladimir) → review+
Updated•15 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Updated•15 years ago
|
Attachment #442220 -
Flags: approval1.9.1.10?
Updated•15 years ago
|
Attachment #442220 -
Flags: approval1.9.1.10? → approval1.9.1.10+
Comment 28•15 years ago
|
||
Comment on attachment 442220 [details] [diff] [review]
possible fix for 1.9.1
a=beltzner for 1.9.1.10
Comment 29•15 years ago
|
||
Comment 30•15 years ago
|
||
Verified crashing in 1.9.1.9 and not crashing with 1.9.1.10 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.10) Gecko/20100504 Firefox/3.5.10 (.NET CLR 3.5.30729) using testcase.
The same with 1.9.2.3 and 1.9.2.4 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.4) Gecko/20100503 Firefox/3.6.4 (.NET CLR 3.5.30729).
Keywords: verified1.9.1,
verified1.9.2
Updated•15 years ago
|
Group: core-security
Updated•15 years ago
|
blocking2.0: ? → final+
Comment 31•15 years ago
|
||
Flags: in-testsuite? → in-testsuite+
Updated•14 years ago
|
Crash Signature: [@ TypedArrayTemplate<unsigned char>::class_constructor(JSContext*, JSObject*, unsigned int, long*, long*) ]
You need to log in
before you can comment on or make changes to this bug.
Description
•