TM: js_Array_dense_setelem can call arbitrary JS code

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: gal)

Tracking

({testcase, verified1.9.1})

Trunk
testcase, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
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

10 years ago
We should grow on-trace. Taking. Thanks for the testcase!
Assignee: general → gal
(Assignee)

Comment 2

10 years ago
(I mean grow dense on trace, as long not OOM, but never go slow)
(Assignee)

Comment 3

10 years ago
Created attachment 363357 [details] [diff] [review]
patch
Attachment #363357 - Flags: review?(jorendorff)
(Reporter)

Comment 4

10 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

10 years ago
The opposite. Shaver will try to pull the fast path out of the builtin and only call to realloc.
(Reporter)

Comment 6

10 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

10 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 on attachment 363357 [details] [diff] [review]
patch

Very nice.
Attachment #363357 - Flags: review?(jorendorff) → review+
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

10 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?
(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

10 years ago
http://hg.mozilla.org/tracemonkey/rev/2f7313f0696f
Flags: blocking1.9.1?
Priority: -- → P2
Whiteboard: fixed-in-tracemonkey

Updated

10 years ago
Flags: blocking1.9.1? → blocking1.9.1+

Comment 13

10 years ago
http://hg.mozilla.org/mozilla-central/rev/2f7313f0696f
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Keywords: testcase
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+
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.