457580 - TM: Crash [@ js_ValueToStringId] with /x/[-4]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

$ cat a.js
for (let i=0;i<3;++i) /x/[-4];
$ ~/tracemonkey/js/src/Darwin_DBG.OBJ/js -j a.js
Crash [@ js_ValueToStringId]

Crashes with -4, -8, -12, etc. but not with other numbers I tried.

Crashes dereferencing 0xfffffff8, so filing as security-sensitive.

Comment 1

[Jesse Ruderman]

With some trial-and-error wrapping, I got it to crash in the interactive shell:

$ ~/tracemonkey/js/src/Darwin_DBG.OBJ/js -j
js> eval("(function(){for (let i=0;i<3;++i) /x/[-4];})()")
Crash [@ js_ValueToStringId]

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

This is a regression from bug 455748. In particular, that caused us to call js_IndexToId on a jsval that isn't actually an index. Further confusing things is the fact that we store the bogo-index back into the interpreter stack, causing us to crash in the interpreter and not on trace.

Comment 3

[Andreas Gal :gal]

Attachment: Catch negative indexes at recording time. At runtime the builtins already check for us. Also guard for shape and setters/getters for non-dense integer index setelem case.

Comment 4

[Andreas Gal :gal]

Comment on attachment 340836 [details] [diff] [review]
Catch negative indexes at recording time. At runtime the builtins already check for us. Also guard for shape and setters/getters for non-dense integer index setelem case.

Whoever gets to it first please review.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 340836 [details] [diff] [review]
Catch negative indexes at recording time. At runtime the builtins already check for us. Also guard for shape and setters/getters for non-dense integer index setelem case.

Looks good.

Comment 6

[Andreas Gal :gal]

Not user reported so closing bug directly.

http://hg.mozilla.org/tracemonkey/rev/358a6b0a757c

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 340836 [details] [diff] [review]
Catch negative indexes at recording time. At runtime the builtins already check for us. Also guard for shape and setters/getters for non-dense integer index setelem case.

I should have caught this -- I was looking for guardElemOp but thought one was there, saw net-0 change to count of calls to that method, gave up the chase too soon. Sorry,

/be

Comment 8

[Andreas Gal :gal]

Follow-up fix (was breaking jquery).

http://hg.mozilla.org/tracemonkey/rev/82841707d495

Comment 9

[Bob Clary [:bc] (inactive)]

test also included in js1_8_1/trace/trace-test.js

Comment 10

[Bob Clary [:bc] (inactive)]

verified fixed mozilla-central, tracemonkey.

Comment 11

[Bob Clary [:bc] (inactive)]

v 1.9.1