679112 - crash: SpiderMonkey typed array APIs seem to lose type safety

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Dahl :ddahl]

Discovered in bug 440046 - it seems this crash is happening in debug builds.

via Brendan: "The SpiderMonkey typed array APIs seem to lose type safety by indirecting via
JSObject*, and they don't assert correct class to make up for that at runtime
in debug builds."

Comment 1

[Brendan Eich [:brendan]]

Nikhil, could you take this one? Cc'ing helpers.

/be

Comment 2

[David Dahl :ddahl]

Attachment: v 0.1 Brendan's typedarray patch split from bug 440046

Split this bit that brendan wrote out from the crypto.gerRandomValues patch in bug 440046

Comment 3

[David Dahl :ddahl]

I should have asked for this to land on the js branch days ago...

in rebasing against the latest m-c i get this error:

In the directory  /home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/js/src
The following command failed to execute properly:
c++ -o jstypedarray.o -c -I./../../dist/system_wrappers_js -include /home/ddahl/code/moz/mozilla-central/js/src/config/gcc_hidden.h -DOSTYPE="Linux2.6" -DOSARCH=Linux -DEXPORT_JS_API -DIMPL_MFBT -DJS_HAS_CTYPES -DDLL_PREFIX="lib" -DDLL_SUFFIX=".so" -Ictypes/libffi/include -I. -I/home/ddahl/code/moz/mozilla-central/js/src -I. -I./../../dist/include -I./../../dist/include/nsprpub -I/home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/dist/include/nspr -I/home/ddahl/code/moz/mozilla-central/js/src -I/home/ddahl/code/moz/mozilla-central/js/src/assembler -I/home/ddahl/code/moz/mozilla-central/js/src/yarr -fPIC -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -pedantic -Wno-long-long -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DENABLE_JIT=1 -DMOZILLA_CLIENT -include ./js-confdefs.h -MD -MF .deps/jstypedarray.pp /home/ddahl/code/moz/mozilla-central/js/src/jstypedarray.cpp
make[5]: *** [jstypedarray.o] Error 1
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [libs_tier_js] Error 2
make[3]: *** [tier_js] Error 2
make[2]: *** [default] Error 2
make[1]: *** [realbuild] Error 2
make: *** [build] Error 2

I'll attach the rebased patch

Comment 4

[David Dahl :ddahl]

Attachment: rebased against m-c

Comment 5

[Luke Wagner [:luke]]

Why are the JSUint32 types in the header being turned into uint32 types in the definitions?

Comment 6

[David Dahl :ddahl]

(In reply to Luke Wagner [:luke] from comment #5)
> Why are the JSUint32 types in the header being turned into uint32 types in
> the definitions?

I just fixed what was in my .rej file:

http://pastebin.mozilla.org/1319624

Comment 7

[David Dahl :ddahl]

(In reply to Luke Wagner [:luke] from comment #5)
> Why are the JSUint32 types in the header being turned into uint32 types in
> the definitions?

Actually, the header changes were mine, I was experimenting. I have reverted them and still this patch is failing with no apparent reason:

"The following command failed to execute properly:
c++ -o jstypedarray.o -c -I"...

Comment 8

[David Dahl :ddahl]

ah. nothing to see here. Looks like my bad typing skillz are the problem... sorry.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 557855 [details] [diff] [review]
rebased against m-c

Ah.  Well, IIUC, r+ on this patch minus the s/JSUint32/uint32/ changes.

Comment 10

[David Dahl :ddahl]

changed owner to brendan as I did not write this patch

Comment 11

[Ed Morley [:emorley]]

Unless I'm misunderstanding, the r+ was conditional on removing the s/JSUint32/uint32/ changes? Removing checkin-needed until that's resolved either way. Also please could a commit message + author be added.

Thanks! :-)

Comment 12

[David Dahl :ddahl]

Attachment: For checkin, updated user and checkin comment

Updated patch's user and message

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Still a number of jsuint->uint changes left...

Comment 14

[David Dahl :ddahl]

After hg pull --update I see all of this patch is rotten and looks kind of like it may be unneeded? 

Does 

JS_ASSERT(atype >= 0 && atype < TypedArray::TYPE_MAX);

superceed

JS_ASSERT(js_IsTypedArray(obj));

Comment 15

[Brendan Eich [:brendan]]

Not mine at this point, found it rotting on my list. Steve, please close if obsolete. Thanks,

/be

Comment 16

[Steve Fink [:sfink] [:s:]]

Yep, all of those (or their replacements) now assert either directly or indirectly via asArrayBufferObject.