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]
:
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 | 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 | Review

Description 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 David Humphrey (:humph) 2010-03-21 14:14:22 PDT
Also a problem in 3.6.
Comment 2 David Humphrey (:humph) 2010-03-21 14:56:51 PDT
See also bug 553948.
Comment 3 Boris Zbarsky [:bz] 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 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 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 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 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 Boris Zbarsky [:bz] 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 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 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 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 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 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 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 Robert O'Callahan (:roc) (Exited; 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 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 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 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 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 Mike Beltzner [:beltzner, not reading bugmail] 2010-04-28 15:33:34 PDT
Do we have a 1.9.1 patch?
Comment 21 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 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 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 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 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 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 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 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 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 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 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.