Closed
Bug 479487
Opened 15 years ago
Closed 15 years ago
TM: js_Array_dense_setelem can call arbitrary JS code
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: igor, Assigned: gal)
References
Details
(Keywords: testcase, verified1.9.1, Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
1.48 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
Currently js_Array_dense_setelem delegates to array_setProperty difficult cases of assigning array properties. But that function can turn the array into slow one and call arbitrary JS code through a setter on the prototype as the following example demonstrates: @watson~/m/tm/js/src> cat ~/s/y.js Array.prototype[1] = 2; Array.prototype.__defineSetter__(32, function() { print("Hello from arbitrary JS");}); Array.prototype.__defineGetter__(32, function() { return 11; }); function f() { var a = []; for (var i = 0; i != 10; ++i) { a[1 << i] = 9999; } return a; } f(); @watson~/m/tm/js/src> schroot -- ~/build/js32.tm.dbg/js -j ~/s/y.js I: [i386 chroot] Running command: “/home/igor/build/js32.tm.dbg/js -j /home/igor/s/y.js” Assertion failure: cx->bailExit, at /home/igor/m/tm/js/src/jstracer.cpp:4640 E: Child terminated by signal ‘Trace/breakpoint trap’ @ This is a regression introduced in the changeset 980c23ed651e, http://hg.mozilla.org/mozilla-central/rev/980c23ed651e/ (no bug number).
Assignee | ||
Comment 1•15 years ago
|
||
We should grow on-trace. Taking. Thanks for the testcase!
Assignee: general → gal
Assignee | ||
Comment 2•15 years ago
|
||
(I mean grow dense on trace, as long not OOM, but never go slow)
Assignee | ||
Comment 3•15 years ago
|
||
Attachment #363357 -
Flags: review?(jorendorff)
Reporter | ||
Comment 4•15 years ago
|
||
Comment on attachment 363357 [details] [diff] [review] patch >diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp >--- a/js/src/jsarray.cpp >+++ b/js/src/jsarray.cpp >@@ -800,21 +800,31 @@ > { > JS_ASSERT(OBJ_IS_DENSE_ARRAY(cx, obj)); > Could the fast array guard can also be moved into js_Array_dense_setelem for smaller traced code?
Assignee | ||
Comment 5•15 years ago
|
||
The opposite. Shaver will try to pull the fast path out of the builtin and only call to realloc.
Reporter | ||
Comment 6•15 years ago
|
||
(In reply to comment #5) > The opposite. Oh, a call is that expensive? I guess FASTCALL means SLIGHTLY_FASTER_THEN_YOUR_AVERAGE_SLOW_CALL ;)
Comment 7•15 years ago
|
||
The call's probably not too horrible, but its need to spill ecx and edx for the first two arguments is probably the killer, combined with likely-dumb codegen that starts with the usual function prologue.
Comment 8•15 years ago
|
||
Comment on attachment 363357 [details] [diff] [review] patch Very nice.
Attachment #363357 -
Flags: review?(jorendorff) → review+
Comment 9•15 years ago
|
||
Comment on attachment 363357 [details] [diff] [review] patch >+ /* >+ * Let the interpreter worry about negative array indexes. >+ */ >+ if (i < 0) >+ return JS_FALSE; >+ >+ /* >+ * If needed, grow the array as long it remains dense, otherwise fall off >+ * trace. >+ */ >+ jsuint u = i; >+ jsuint length = ARRAY_DENSE_LENGTH(obj); >+ if (((jsuint)u >= length) && (INDEX_TOO_SPARSE(obj, u) || !EnsureLength(cx, obj, u + 1))) Drive-by nits (I didn't check for trailing whitespace :-P): u is a jsuint, no need to cast. Don't overparenthesize relationals and equality ops against &&. >+ return JS_FALSE; >+ >+ if (obj->dslots[u] == JSVAL_HOLE) { >+ if (cx->runtime->anyArrayProtoHasElement) >+ return JS_FALSE; >+ if (u >= jsuint(obj->fslots[JSSLOT_ARRAY_LENGTH])) C++-style constructor cast looks good here. Just noticing unnecessary (jsuint) cast earlier was C-style. Prefer C++-style now outside of jsapi.h and its includes. /be
Reporter | ||
Comment 10•15 years ago
|
||
(In reply to comment #9) > C++-style constructor cast looks good here. Just noticing unnecessary (jsuint) > cast earlier was C-style. Prefer C++-style now outside of jsapi.h and its > includes. Do you mean here just the constructor calls and not (static|const|reinterpret>_cast operators?
Comment 11•15 years ago
|
||
(In reply to comment #10) > (In reply to comment #9) > > C++-style constructor cast looks good here. Just noticing unnecessary (jsuint) > > cast earlier was C-style. Prefer C++-style now outside of jsapi.h and its > > includes. > > Do you mean here just the constructor calls and not > (static|const|reinterpret>_cast operators? We're still throwing C-style casts around instead of reinterpret_cast<T>(v) casts, I know. We should use the long_winded but type-safer variants, and Waldo at least has started. It'll be a while yet, or a post-3.1 flag day, before we get rid of all the legacy C-style pointer-type casts. /be
Assignee | ||
Comment 12•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/2f7313f0696f
Flags: blocking1.9.1?
Priority: -- → P2
Whiteboard: fixed-in-tracemonkey
Updated•15 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Comment 13•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/2f7313f0696f
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 14•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9b11f1989bd5
Keywords: fixed1.9.1
Depends on: 481977
Comment 15•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/2013ac7156d9 /cvsroot/mozilla/js/tests/js1_5/extensions/regress-479487.js,v <-- regress-479487.js initial revision: 1.1
Flags: in-testsuite+
Comment 16•15 years ago
|
||
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•