crash: SpiderMonkey typed array APIs seem to lose type safety

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ddahl, Assigned: sfink)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: js-triage-needed)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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."
Whiteboard: js-triage-needed
Nikhil, could you take this one? Cc'ing helpers.

/be
(Reporter)

Comment 2

6 years ago
Created attachment 553779 [details] [diff] [review]
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
(Reporter)

Updated

6 years ago
Blocks: 440046
(Reporter)

Updated

6 years ago
Attachment #553779 - Flags: review?(luke)

Updated

6 years ago
Attachment #553779 - Flags: review?(luke) → review+
(Reporter)

Comment 3

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

Comment 4

6 years ago
Created attachment 557855 [details] [diff] [review]
rebased against m-c
Attachment #557855 - Flags: review?
(Reporter)

Updated

6 years ago
Attachment #557855 - Flags: review? → review?(luke)

Comment 5

6 years ago
Why are the JSUint32 types in the header being turned into uint32 types in the definitions?
(Reporter)

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

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

Ah.  Well, IIUC, r+ on this patch minus the s/JSUint32/uint32/ changes.
Attachment #557855 - Flags: review?(luke) → review+
(Reporter)

Updated

6 years ago
Attachment #553779 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Assignee: general → ddahl
(Reporter)

Comment 10

6 years ago
changed owner to brendan as I did not write this patch
Assignee: ddahl → brendan
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! :-)
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Reporter)

Comment 12

6 years ago
Created attachment 567290 [details] [diff] [review]
For checkin, updated user and checkin comment

Updated patch's user and message
Attachment #557855 - Attachment is obsolete: true
Attachment #567290 - Flags: review+
Still a number of jsuint->uint changes left...
(Reporter)

Comment 14

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

Updated

6 years ago
No longer blocks: 440046
Not mine at this point, found it rotting on my list. Steve, please close if obsolete. Thanks,

/be
Assignee: brendan → sphink
(Assignee)

Comment 16

5 years ago
Yep, all of those (or their replacements) now assert either directly or indirectly via asArrayBufferObject.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.