427196 - "Assertion failure: OBJ_SCOPE(pobj)->object == pobj" with __proto__, __defineSetter__

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Pasting the following script into a debug js shell triggers
Assertion failure: OBJ_SCOPE(pobj)->object == pobj, at jsinterp.c:370


function boom()
{
  var b = {};
  Array.__proto__ = b;
  b.__proto__ = {};
  var c = {};
  c.__proto__ = [];
  try { c.__defineSetter__("t", undefined); } catch(e) { }
  c.__proto__ = Array;
  try { c.__defineSetter__("v", undefined); } catch(e) { }
}

boom();

Comment 1

[Tommi Komulainen]

Worked for me with 'changeset:   24950:86c57e08cfe7' on Tiger.

Comment 2

[Jesse Ruderman]

Still happens for me on Leopard (TM branch, rev c8a855f68fd4+).

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This should be a regression of bug 418139.

Seems to work as expected with cvs js shell checkout at 2008-03-01 20:05 PST

Asserts with cvs js shell checkout at 2008-03-01 20:07 PST with:
Assertion failure: obj == pobj, at jsinterp.c:149

and later morphed to: (by around the 2008-03-05 00:00 GMT timeframe)
Assertion failure: OBJ_SCOPE(pobj)->object == pobj, at jsinterp.c:365

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-03-01+20%3A05%3A00&maxdate=2008-03-01+20%3A07%3A00&cvsroot=%2Fcvsroot

Bonsai message:

Fix property cache fill to recompute protoIndex to handle XBL and other JS_SetPrototype users (418139, r/a=shaver).

Comment 4

[Igor Bukanov]

I cannot reproduce the assert on Linux with TN or CVS tips. Is it Mac-only?

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #4)
> I cannot reproduce the assert on Linux with TN or CVS tips. Is it Mac-only?

I definitely see this on TM tip without -j on 32-bit Ubuntu and Mac Leopard.

Debug console output in Ubuntu 32-bit: (Mac console spew is similar)

[Thread debugging using libthread_db enabled]
js> function boom()
{
  var b = {};
  Array.__proto__ = b;
  b.__proto__ = {};
  var c = {};
  c.__proto__ = [];
  try { c.__defineSetter__("t", undefined); } catch(e) { }
  c.__proto__ = Array;
  try { c.__defineSetter__("v", undefined); } catch(e) { }
}
js> 
js> boom();
Assertion failure: OBJ_SCOPE(pobj)->object == pobj, at ../jsinterp.cpp:397
[New Thread 0xb7dc86c0 (LWP 12367)]

Program received signal SIGTRAP, Trace/breakpoint trap.
[Switching to Thread 0xb7dc86c0 (LWP 12367)]
JS_Assert (s=0x81e4a30 "OBJ_SCOPE(pobj)->object == pobj", file=0x81e3f61 "../jsinterp.cpp", ln=397) at ../jsutil.cpp:68
68	    abort();
(gdb) bt
#0  JS_Assert (s=0x81e4a30 "OBJ_SCOPE(pobj)->object == pobj", file=0x81e3f61 "../jsinterp.cpp", ln=397) at ../jsutil.cpp:68
#1  0x080b0599 in js_FullTestPropertyCache (cx=0x87b0d88, pc=0x87bc931 "�", objp=0xbf993168, pobjp=0xbf993368, entryp=0xbf9930a0) at ../jsinterp.cpp:397
#2  0x081c18d2 in js_Interpret (cx=0x87b0d88) at ../jsinterp.cpp:4380
#3  0x080ae95e in js_Execute (cx=0x87b0d88, chain=0x87b4000, script=0x87bab58, down=0x0, flags=0, result=0xbf993524) at ../jsinterp.cpp:1589
#4  0x08058547 in JS_ExecuteScript (cx=0x87b0d88, obj=0x87b4000, script=0x87bab58, rval=0xbf993524) at ../jsapi.cpp:5038
#5  0x08051b26 in Process (cx=0x87b0d88, obj=0x87b4000, filename=0x0, forceTTY=0) at ../../shell/js.cpp:485
#6  0x08052330 in ProcessArgs (cx=0x87b0d88, obj=0x87b4000, argv=0xbf993698, argc=0) at ../../shell/js.cpp:786
#7  0x08052632 in main (argc=0, argv=0xbf993698, envp=0xbf99369c) at ../../shell/js.cpp:4638
(gdb)

Comment 6

[Igor Bukanov]

Here is slightly simpler example that also works when run through a file:

@watson-32~> cat ~/s/x.js
function boom()
{
    var c = {__proto__: []};
    var a = {__proto__: {__proto__: {}}};
    c.hasOwnProperty("t");
    c.__proto__ = a;
    c.hasOwnProperty("v");
}

boom();
@watson-32~> ~/build/js32.tm.dbg/js ~/s/x.js
Assertion failure: OBJ_SCOPE(pobj)->object == pobj, at /home/igor/m/tm/js/src/jsinterp.cpp:397
Trace/breakpoint trap

It seems that in all cases one needs a fast array on the prototype chain to lead to the bug. I am not sure the exact consequences of that so I mark the bug as restricted for now.

Comment 7

[Igor Bukanov]

The assert is not harmless as the following modification of the example shows with non-debug build of js shell:

@watson-32~> cat ~/s/x.js
var c = {__proto__: []};
var a = {__proto__: {__proto__: {}}};
c.hasOwnProperty;
c.__proto__ = a;
c.hasOwnProperty;
@watson-32~> ~/build/js32.tm.opt/js ~/s/x.js
Segmentation fault

What is going on here is that the first c.hasOwnProperty finds the property at the prototype depth == 3 in Object.prototype and records that in the cache together with the shape of the scope. Then c.__proto__ = a mutates the prototype chain so its depth becomes 5. Then the second c.hasOwnProperty comes. It finds c.hasOwnProperty in the cache and then access the third element at the prototype chain. That is {__proto__: {}}. But that object shares the scope with Object.protype so its shape matches. Then the code proceeds with accessing the slot of {__proto__: {}} that corresponds to the slot for Object.prototype.hasOwnProperty. In this particular it segfaults as JSObject.dslots is null.

But the bug is not limitted just to null segfaults as one can put, say, 1e6 of properties in Object.prototype and then the code can be used to access the data at the address around 4e6. That can be junk and if that junk would match GC-thing value, this would lead to heap corruption. Another possibility for an exploit is to construct prototype chains with shared scopes with objects that use private slots but still have shares scopes.

Comment 8

[Brendan Eich [:brendan]]

This is why the code in ProcessSetSlotRequests in jsgc.cpp has this paragraph:


        /*
         * Regenerate property cache shape ids for all of the scopes along the
         * old prototype chain, in case any property cache entries were filled
         * by looking up starting from obj.
         */
        while (oldproto && OBJ_IS_NATIVE(oldproto)) {
            scope = OBJ_SCOPE(oldproto);
            SCOPE_MAKE_UNIQUE_SHAPE(cx, scope);
            oldproto = STOBJ_GET_PROTO(scope->object);
        }

But of course the dense array [] stops this loop from regenerating the shape of Object.prototype. D'oh!

Easy fix, just remove OBJ_IS_NATIVE(oldproto) from the loop condition and  move it into the body, with an if-else to do what the current body does if native, else follow JSSLOT_PROTO for non-natives.

/be

Comment 9

[Brendan Eich [:brendan]]

Alternative would make dense array on prototype chain slow, but the more general fix in ProcessSetSlotRequests is needed anyway. We should review other places where a dense array on the proto chain could combine with bad OBJ_IS_NATIVE tests to make for bugs like this one.

/be

Comment 10

[Igor Bukanov]

Another alternative would be not to cache properties that comes after a non-native object is discovered on the prototype chain. It would not stop caching of Array.prototype method lookups for fast arrays due to special code that explicitly looks for fast array methods in their prototypes.

AFAICS such caching of properties behind non-native objects on the protype chain is not correct in general as the non-native object can be mutated without changing its shape which includes altering of its prototype.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> Another alternative would be not to cache properties that comes after a
> non-native object is discovered on the prototype chain. It would not stop
> caching of Array.prototype method lookups for fast arrays due to special code
> that explicitly looks for fast array methods in their prototypes.

And Array.prototype is slow, and people don't delegate to dense arrays, only fuzz-tests do.

> AFAICS such caching of properties behind non-native objects on the protype
> chain is not correct in general as the non-native object can be mutated without
> changing its shape which includes altering of its prototype.

You're right, and this is the true fix. The bogus code is in js_LookupPropertyWithFlags, here:

This is easy to patch too (jsobj.cpp mozilla-central tip line numbers):

3982	        if (!OBJ_IS_NATIVE(proto)) {
3983	            if (!OBJ_LOOKUP_PROPERTY(cx, proto, id, objp, propp))
3984	                return -1;
3985	            return protoIndex + 1;
3986	        }

The return value should be PCVCAP_PROTOSIZE.

/be

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> This is easy to patch too (jsobj.cpp mozilla-central tip line numbers):
> 
> 3982            if (!OBJ_IS_NATIVE(proto)) {
> 3983                if (!OBJ_LOOKUP_PROPERTY(cx, proto, id, objp, propp))
> 3984                    return -1;
> 3985                return protoIndex + 1;
> 3986            }
> 
> The return value should be PCVCAP_PROTOSIZE.

That would not work as js_FillPropertyCache recalculates protoIndex on its own. I guess during recalculation js_FillPropertyCache can just check that the object is native.

Comment 13

[Brendan Eich [:brendan]]

That's right, I had to recalc in fill to get the true (no scope->object shared scope skipping) index.

/be

Comment 14

[Brendan Eich [:brendan]]

Looks exploitable so re-nominating.

/be

Comment 15

[Igor Bukanov]

Here is a test case with a better test coverage:

var c = {__proto__: []};
var a = {__proto__: {__proto__: {}}};
var tmp1 = c.hasOwnProperty;
c.__proto__ = a;
var tmp2 = c.hasOwnProperty;

if (tmp1 !== Object.prototype.hasOwnProperty)
    throw "Bad";  
if (tmp1 !== tmp2)
    throw "Bad";

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Reinstating accidentally-cleared blocking1.9.1? re-nomination flag.

Comment 17

[Igor Bukanov]

Attachment: v1

As discussed, the patch makes sure that js_FillPropertyCache does not cache properties coming from natives-behind-non-natives on the prototype chain.

Comment 18

[Igor Bukanov]

The bug exists on 1.9.0 in the same form so nominating for that branch as well.

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 368937 [details] [diff] [review]
v1

Great, thanks.

/be

Comment 20

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/37ae217b403e

Comment 21

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/37ae217b403e

Comment 22

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: backported 1.9.0.x patch v1

Here's the backported patch for the 1.9.0.x branch.

$ diff ~/Desktop/190patch-v1.diff ~/Desktop/tmpatch.diff
1c1
< Index: js/src/jsinterp.c
---
> Index: tm/js/src/jsinterp.cpp
3,8c3,5
< RCS file: /cvsroot/mozilla/js/src/jsinterp.c,v
< retrieving revision 3.505
< diff -u -8 -p -r3.505 jsinterp.c
< --- js/src/jsinterp.c	3 Mar 2009 01:53:11 -0000	3.505
< +++ js/src/jsinterp.c	24 Mar 2009 19:05:56 -0000
< @@ -158,17 +158,23 @@ js_FillPropertyCache(JSContext *cx, JSOb
---
> --- tm.orig/js/src/jsinterp.cpp	2009-03-23 19:50:10.000000000 +0100
> +++ tm/js/src/jsinterp.cpp	2009-03-23 19:56:58.000000000 +0100
> @@ -164,17 +164,23 @@ js_FillPropertyCache(JSContext *cx, JSOb

Comment 23

[Samuel Sidler (old account; do not CC)]

Comment on attachment 369116 [details] [diff] [review]
backported 1.9.0.x patch v1

Approved for 1.9.0.10. a=ss

Comment 24

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b828e1121187

Comment 25

[Igor Bukanov]

landed to 1.9.0:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.506; previous revision: 3.505

Comment 26

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-427196-01.js

Comment 27

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-427196-02.js

Comment 28

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-427196-03.js

Comment 29

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-427196-03.js

fix gTestfile

Comment 30

[Bob Clary [:bc] (inactive)]

v 1.9.0, 1.9.1, 1.9.2

Comment 31

[Daniel Veditz [:dveditz]]

These testcases do not appear to affect the 1.8 branch (and wouldn't be expected to given the regression range).

Comment 32

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/56cf031fdc6c

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-427196-01.js,v  <--  regress-427196-01.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-427196-02.js,v  <--  regress-427196-02.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-427196-03.js,v  <--  regress-427196-03.js
initial revision: 1.1