576716 - Crash [@ TypedArrayTemplate<int>::init] or [@ TypedArrayTemplate<int>::create]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, critical)

Description

[Jesse Ruderman]

Int32Array(ArrayBuffer.prototype)


Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000008
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   TypedArrayTemplate<int>::init(JSContext*, JSObject*, int, int) + 372
1   TypedArrayTemplate<int>::create(JSContext*, JSObject*, unsigned int, long*, long*) + 704
2   TypedArrayTemplate<int>::class_constructor(JSContext*, JSObject*, unsigned int, long*, long*) + 144
3   js_Invoke + 2771
4   js_Interpret + 93623
5   js_Execute + 1664
6   JS_ExecuteScript + 59
7   Process(JSContext*, JSObject*, char*, int) + 1360 (js.cpp:515)
8   ProcessArgs(JSContext*, JSObject*, char**, int) + 2026 (js.cpp:837)
9   shell(JSContext*, int, char**, char**) + 223 (js.cpp:5025)
10  main + 220 (js.cpp:5114)
11  _start + 224
12  start + 33

Comment 1

[Gary Kwong [:gkw] [:nth10sd]]

Seems to be a null deref:

(gdb) x/i $eip
0x14cc9e <_ZN18TypedArrayTemplateIiE4initEP9JSContextP8JSObjectii+416>: mov    0x4(%eax),%eax
(gdb) x/b $eax
0x0:    Cannot access memory at address 0x0

Comment 2

[Gary Kwong [:gkw] [:nth10sd]]

Opt output from gdb:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000004
0x000ffbe4 in TypedArrayTemplate<int>::create ()
(gdb) bt
#0  0x000ffbe4 in TypedArrayTemplate<int>::create ()
#1  0x000fff6d in TypedArrayTemplate<int>::class_constructor ()
#2  0x00066cac in Invoke<int (*)(JSContext*, JSObject*, unsigned int, long*, long*)> ()
#3  0x000676b8 in js_Invoke ()
#4  0x0005783e in js_Interpret ()
#5  0x000672f6 in js_Execute ()
#6  0x00013648 in JS_ExecuteScript ()
#7  0x00004ef6 in Process ()
#8  0x00008436 in shell ()
#9  0x00008947 in main ()
(gdb) x/i $eip
0xffbe4 <_ZN18TypedArrayTemplateIiE6createEP9JSContextP8JSObjectjPlS5_+3156>:   mov    0x4(%edx),%ecx
(gdb) x/b $edx
0x0:    Cannot access memory at address 0x0

Comment 3

[Gary Kwong [:gkw] [:nth10sd]]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   36982:165a48c9ea89
user:        Vladimir Vukicevic
date:        Sat Jan 09 13:01:37 2010 -0800
summary:     b=532774; add native typed arrays to JavaScript; r=brendan,jorendorff

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix

Missing null check, this should fix it.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 465813 [details] [diff] [review]
fix

jorendorff out, freeze looming -- Waldo has helped here before.

/be

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 465813 [details] [diff] [review]
fix

>diff --git a/js/src/jstypedarray.cpp b/js/src/jstypedarray.cpp

>@@ -1031,6 +1031,13 @@ class TypedArrayTemplate
>         } else if (js_IsTypedArray(other)) {
>             TypedArray *tarray = TypedArray::fromJSObject(other);
> 
>+            if (!tarray) {
>+                // the arg isn't a real typedarray
>+                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                     JSMSG_TYPED_ARRAY_BAD_ARGS);
>+                return false;
>+            }
>+
>             //printf ("SizeAndCount: %d %d\n", sizeof(NativeType), tarray->length);
> 
>             if (!createBufferWithSizeAndCount(cx, sizeof(NativeType), tarray->length))

Is this necessary?  js_IsTypedArray means it's a fast array.  If it's fast, doesn't that mean it has a TypedArray* private?  None of the non-buffer testcases fail.  The slow arrays fall through to the else-bad-args case.  (Maybe they shouldn't, in which case the null-check here would make sense, but that's not what we have now.)

A full audit of callers (I hope/think exhaustive, although I might have missed something) reveals another place that needs to be fixed:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#5790

This testcase (must be accessed online due to vagaries of worker URL-loading requirements) demonstrates the problem:

http://web.mit.edu/jwalden/www/worker.html [CRASHES BROWSER]

r- until the necessity question is answered and the other case is fixed, with a testcase in the bug.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix, v2

Ok, updated; the first part I added as a precaution, but it should be an assert.  I fixed the content case, but I'm cc'ing bent to figure out a test case for it.  I'm not sure where the tests are for this stuff in works.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ben, can you turn Waldo's test into a testcase?  Not sure where the arraybuffer workers tests are.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 465947 [details] [diff] [review]
fix, v2

Assert rather than check sounds good to me.  I'm not worried about having a test immediately, so long as one does happen shortly.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/7b9c03096aa1

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Test for Workers

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/be7c2f122492

Comment 13

[:ehsan akhgari]

Backed out because of mochitest-3 orange:

http://hg.mozilla.org/mozilla-central/rev/7f27cd7d8e59

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

uhh.. I think you have the wrong bug?

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(nvm, it went in as a ridealong and got taken out as a ridealong -- but this bug should stay closed since the main issue is fixed)

Comment 16

[:ehsan akhgari]

(In reply to comment #15)
> (nvm, it went in as a ridealong and got taken out as a ridealong -- but this
> bug should stay closed since the main issue is fixed)

Even though the patch is no longer on mozilla-central?

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #16)
> Even though the patch is no longer on mozilla-central?

Only the test got backed out.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Test for Workers [checked in]

Now with hg header.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in test
http://hg.mozilla.org/mozilla-central/rev/73899b33ca4b