Crash [@ TypedArrayTemplate<unsigned char>::class_constructor(JSContext*, JSObject*, unsigned int, long*, long*) ]

RESOLVED FIXED

Status

()

Core
Canvas: 2D
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: humph, Assigned: vlad)

Tracking

(4 keywords)

Trunk
crash, testcase, verified1.9.1, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.0.x -
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 needed, status1.9.2 .4-fixed, blocking1.9.1 .10+, status1.9.1 .10-fixed)

Details

(Whiteboard: [sg:critical], crash signature, URL)

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
Also a problem in 3.6.
(Reporter)

Comment 2

7 years ago
See also bug 553948.
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).
Group: core-security
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.2: --- → ?
Blocks: 553948
Blocks: 532774
Assignee: nobody → vladimir
This is fixed on tracemonkey, by bug 550351; I'm not sure what the 3.6 issue is, though.
Depends on: 550351
Found the 1.9.2 issue -- CoerceArrayToCanvasImageData wasn't checking capacity, just length, so it had the same problem.  Testing a patch now.
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.
Attachment #434340 - Flags: review?(jorendorff)
Attachment #434340 - Flags: review?(bzbarsky)
Adding sg:? -- this should probably go into the next 3.6 dot release.
Whiteboard: [sg:?]
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+
Attachment #434340 - Flags: review?(jorendorff) → review+
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?
(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:?]
Will be back in a jiffy to approve the patch. Does this affect 1.9.1?
blocking1.9.2: ? → needed
status1.9.1: --- → ?
status1.9.2: ? → wanted
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+
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...
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]
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?
(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... :)
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.
status1.9.2: wanted → .4-fixed

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite?
Keywords: testcase
Resolution: --- → FIXED
(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: --- → ?
status1.9.1: ? → wanted
Do we have a 1.9.1 patch?
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+
js_CoerceArrayToCanvasImageData doesn't exist in 1.9.0 -- FF3.0.x is fine.
Flags: wanted1.9.0.x-
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+
blocking1.9.1: ? → ---
status1.9.1: wanted → unaffected
I'm tired. This affects 1.9.1, but it's an issue with a different function.
blocking1.9.1: --- → ?
status1.9.1: unaffected → ?
Created attachment 442220 [details] [diff] [review]
possible fix for 1.9.1
Attachment #442220 - Flags: superreview?(bzbarsky)
Attachment #442220 - Flags: review?(vladimir)
Attachment #442220 - Flags: review?(jorendorff)
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.
status1.9.1: ? → wanted

Updated

7 years ago
blocking1.9.1: ? → .10+
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+
Attachment #442220 - Flags: superreview?(bzbarsky) → superreview+
Attachment #442220 - Flags: review?(vladimir) → review+
OS: Mac OS X → All
Hardware: x86 → All
Attachment #442220 - Flags: approval1.9.1.10?
Attachment #442220 - Flags: approval1.9.1.10? → approval1.9.1.10+
Comment on attachment 442220 [details] [diff] [review]
possible fix for 1.9.1

a=beltzner for 1.9.1.10
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e28579a43585
status1.9.1: wanted → .10-fixed
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
Group: core-security
blocking2.0: ? → final+

Comment 31

7 years ago
Test: http://hg.mozilla.org/mozilla-central/rev/970453a7c6b9
Flags: in-testsuite? → in-testsuite+
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.