As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 553938 - Crash [@ TypedArrayTemplate<unsigned char>::class_constructor(JSContext*, JSObject*, unsigned int, long*, long*) ]
: Crash [@ TypedArrayTemplate<unsigned char>::class_constructor(JSContext*, JSO...
Status: RESOLVED FIXED
[sg:critical]
: crash, testcase, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Vladimir Vukicevic [:vlad] [:vladv]
:
: Milan Sreckovic [:milan]
Mentors:
http://crash-stats.mozilla.com/report...
Depends on: 550351
Blocks: 532774 553948
  Show dependency treegraph
 
Reported: 2010-03-21 14:02 PDT by David Humphrey (:humph)
Modified: 2011-06-13 10:01 PDT (History)
13 users (show)
dveditz: wanted1.9.0.x-
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
needed
.4-fixed
.10+
.10-fixed


Attachments
Test Case (will crash your browser if you click "Crash Me") (596 bytes, text/html)
2010-03-21 14:02 PDT, David Humphrey (:humph)
no flags Details
fix and crashtest (1.9.2) (1.75 KB, patch)
2010-03-23 13:59 PDT, Vladimir Vukicevic [:vlad] [:vladv]
jorendorff: review+
bzbarsky: review+
mbeltzner: approval1.9.2.4+
Details | Diff | Splinter Review
possible fix for 1.9.1 (5.75 KB, patch)
2010-04-28 16:03 PDT, Reed Loden [:reed] (use needinfo?)
jorendorff: review+
vladimir: review+
bzbarsky: superreview+
mbeltzner: approval1.9.1.10+
Details | Diff | Splinter Review

Description User image David Humphrey (:humph) 2010-03-21 14:02:37 PDT
Created attachment 433833 [details]
Test Case (will crash your browser if you click "Crash Me")

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
Comment 1 User image David Humphrey (:humph) 2010-03-21 14:14:22 PDT
Also a problem in 3.6.
Comment 2 User image David Humphrey (:humph) 2010-03-21 14:56:51 PDT
See also bug 553948.
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2010-03-22 11:57:43 PDT
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).
Comment 4 User image Vladimir Vukicevic [:vlad] [:vladv] 2010-03-23 10:58:01 PDT
This is fixed on tracemonkey, by bug 550351; I'm not sure what the 3.6 issue is, though.
Comment 5 User image Vladimir Vukicevic [:vlad] [:vladv] 2010-03-23 12:02:56 PDT
Found the 1.9.2 issue -- CoerceArrayToCanvasImageData wasn't checking capacity, just length, so it had the same problem.  Testing a patch now.
Comment 6 User image Vladimir Vukicevic [:vlad] [:vladv] 2010-03-23 13:59:04 PDT
Created attachment 434340 [details] [diff] [review]
fix and crashtest (1.9.2)

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.
Comment 7 User image Vladimir Vukicevic [:vlad] [:vladv] 2010-03-23 14:02:08 PDT
Adding sg:? -- this should probably go into the next 3.6 dot release.
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2010-03-23 14:10:02 PDT
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.
Comment 9 User image Vladimir Vukicevic [:vlad] [:vladv] 2010-03-23 14:26:55 PDT
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.
Comment 10 User image Reed Loden [:reed] (use needinfo?) 2010-03-23 19:56:13 PDT
(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.
Comment 11 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-03-24 12:47:42 PDT
Will be back in a jiffy to approve the patch. Does this affect 1.9.1?
Comment 12 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-03-24 12:49:14 PDT
Comment on attachment 434340 [details] [diff] [review]
fix and crashtest (1.9.2)

a=beltzner for 1.9.2.3
Comment 13 User image Daniel Veditz [:dveditz] 2010-03-24 12:49:27 PDT
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...
Comment 14 User image Vladimir Vukicevic [:vlad] [:vladv] 2010-03-24 17:07:53 PDT
Is just marking it security sensitive enough to put it into the "to be looked at" bucket?  I wasn't clear on that.
Comment 15 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2010-03-31 14:00:48 PDT
Let's call it critical ... it doesn't really matter since we'll have the fix everywhere soon anyway.
Comment 16 User image Vladimir Vukicevic [:vlad] [:vladv] 2010-04-07 13:13:11 PDT
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 User image Reed Loden [:reed] (use needinfo?) 2010-04-07 13:17:39 PDT
(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... :)
Comment 18 User image Vladimir Vukicevic [:vlad] [:vladv] 2010-04-07 15:40:58 PDT
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.
Comment 19 User image Daniel Veditz [:dveditz] 2010-04-28 15:24:11 PDT
(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
Comment 20 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-04-28 15:33:34 PDT
Do we have a 1.9.1 patch?
Comment 21 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-04-28 15:42:50 PDT
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
Comment 22 User image Daniel Veditz [:dveditz] 2010-04-28 15:46:15 PDT
js_CoerceArrayToCanvasImageData doesn't exist in 1.9.0 -- FF3.0.x is fine.
Comment 23 User image Reed Loden [:reed] (use needinfo?) 2010-04-28 15:50:28 PDT
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.
Comment 24 User image Reed Loden [:reed] (use needinfo?) 2010-04-28 15:57:18 PDT
I'm tired. This affects 1.9.1, but it's an issue with a different function.
Comment 25 User image Reed Loden [:reed] (use needinfo?) 2010-04-28 16:03:43 PDT
Created attachment 442220 [details] [diff] [review]
possible fix for 1.9.1
Comment 26 User image Daniel Veditz [:dveditz] 2010-04-28 17:17:25 PDT
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.
Comment 27 User image Jason Orendorff [:jorendorff] 2010-04-29 13:09:39 PDT
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.
Comment 28 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-04-29 14:35:47 PDT
Comment on attachment 442220 [details] [diff] [review]
possible fix for 1.9.1

a=beltzner for 1.9.1.10
Comment 29 User image Reed Loden [:reed] (use needinfo?) 2010-04-29 14:40:41 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e28579a43585
Comment 30 User image Al Billings [:abillings] 2010-05-07 11:17:30 PDT
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).
Comment 31 User image Jesse Ruderman 2010-09-27 19:56:15 PDT
Test: http://hg.mozilla.org/mozilla-central/rev/970453a7c6b9

Note You need to log in before you can comment on or make changes to this bug.