642894 - TM: Assertion failure: newShape->slot == lastProperty()->slot, at ../jsobjinlines.h:795

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Another mutable __proto__ bug:
--
function f() {}
function g(o) {
    o.__proto__ = function() {};
    f = Object.prototype;
    Object.seal(o);
}
g(f);
g(f);
--
Asserts on JM branch (works on TM branch):

$ ./js test.js
Assertion failure: newShape->slot == lastProperty()->slot, at ../jsobjinlines.h:795

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Stack

Comment 2

[Brian Hackett [Laid off!]]

Hmm.  This is a TM bug: the assignment to __proto__ is a SETMETHOD, the function(){} doesn't get cloned on the first call to g and has properties added to it directly when Object.seal snapshots it.

My understanding of js_CloneFunctionObject is that it should only be used on functions that are empty, i.e. we shouldn't be putting properties on a function and then cloning it later.  That isn't asserted anywhere by TM (why this testcase works in the TM branch), but JM has a weakened form of such an assertion which is tripping here (we copy the type/shape from the original function to the cloned function).

This testcase exposes the problem in a TM shell:

function foo() {
  var x = {};
  x.__proto__ = function() { return 0 }
  return x;
}
var a = foo();
var b = foo();
print(a.__proto__ === b.__proto__);

d8/jsc:
false

js:
true

Comment 3

[Jeff Walden [:Waldo]]

So obj_getProto would have to invoke the method barrier, basically, to eliminate this.

Comment 4

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

Testcase from bug 648837 comment 2:

a = <x><y/></x>;
a.function::__proto__ = null;
-a

This testcase seems to also be related. Tested on JM rev 17cbc8fed578 with -n,
64-bit shell in 10.6.

Comment 5

[Jeff Walden [:Waldo]]

I'm not so sure about it being related, myself -- it doesn't involve function cloning, or a needed __proto__ method barrier, at all.  But I think bug 646129's patch would likely fix it.

Comment 6

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

eval("\
    for(let y;;y,x.E=function(){}) {\
        function x(){}(<x/>)\
    }\
")

is a testcase that also asserts identically, but which crashes in a 32-bit opt shell on changeset 325744fbf7f0 without any CLI arguments at JSObject::setSlot.

Snippet of stack:

(gdb) bt
#0  0x000e7235 in JSObject::setSlot () at jsobj.h:706
#1  0x000e7235 in JSObject::nativeSetSlot () at /Users/fuzz2/Desktop/jsfunfuzz-dbg-32-jm-68840-325744fbf7f0/patched/compilePath/js/src/jsobjinlines.h:1089
#2  0x000e7235 in js_NativeSet (cx=0x51a660, obj=0x6110e0, shape=0x617258, added=false, strict=false, vp=0xbfffe640) at /Users/fuzz2/Desktop/jsfunfuzz-dbg-32-jm-68840-325744fbf7f0/patched/compilePath/js/src/jsobj.cpp:5669
#3  0x000e7a02 in js_SetPropertyHelper (cx=0x51a660, obj=0x6110e0, id=3261648, defineHow=<value temporarily unavailable, due to optimizations>, vp=0xbfffe640, strict=0) at /Users/fuzz2/Desktop/jsfunfuzz-dbg-32-jm-68840-325744fbf7f0/patched/compilePath/js/src/jsobj.cpp:6174
#4  0x000a722a in js::Interpret (cx=0x51a660, entryFrame=0x10000b0, inlineCallCount=0, interpMode=js::JSINTERP_NORMAL) at /Users/fuzz2/Desktop/jsfunfuzz-dbg-32-jm-68840-325744fbf7f0/patched/compilePath/js/src/jsinterp.cpp:4439

Comment 7

[Brian Hackett [Laid off!]]

(In reply to comment #6)
> eval("\
>     for(let y;;y,x.E=function(){}) {\
>         function x(){}(<x/>)\
>     }\
> ")

Maybe dupe of bug 648837 too (also a bug in TM that bites JM).

Comment 8

[Brian Hackett [Laid off!]]

Attachment: patch

Patch for the first testcase.  Under SETMETHOD on __proto__, we took a path that called the shape's set() property without cloning the function first.

Comment 9

[Brian Hackett [Laid off!]]

The testcase in comment 4 gives me a stack overflow on TM tip, JM tip and the named JM revision.  Should be fixed by bug 646129, made a note in that bug.

Comment 10

[Daniel Veditz [:dveditz]]

Assigning to bhackett because he's clearly working on it. Making a hidden security bug based on comment in bug 648837 that this is similar. Did it turn out to be similar and potentially exploitable?

Comment 11

[Brian Hackett [Laid off!]]

This is a similar case to bug 648837; this could cause semantic bugs on TM but not crashes, and is only a problem on the Jaegermonkey branch.

Above patch landed to JM:

http://hg.mozilla.org/projects/jaegermonkey/rev/94ae102189f0

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 538062 [details] [diff] [review]
patch

Review of attachment 538062 [details] [diff] [review]:
-----------------------------------------------------------------