Closed Bug 642894 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking-fx --- -

People

(Reporter: jandem, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

(Whiteboard: [sg:none])

Attachments

(2 files)

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
Attached file Stack
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
So obj_getProto would have to invoke the method barrier, basically, to eliminate this.
Severity: normal → critical
blocking-fx: --- → ?
Summary: TI: Assertion failure: newShape->slot == lastProperty()->slot, at ../jsobjinlines.h:795 → TM: Assertion failure: newShape->slot == lastProperty()->slot, at ../jsobjinlines.h:795
Version: unspecified → Trunk
blocking-fx: ? → -
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.
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.
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
(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).
Attached patch patchSplinter Review
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.
Attachment #538062 - Flags: review?(dmandelin)
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.
Attachment #538062 - Flags: review?(dmandelin) → review?(jwalden+bmo)
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?
Assignee: general → bhackett1024
Group: core-security
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
Whiteboard: [sg:none]
Comment on attachment 538062 [details] [diff] [review]
patch

Review of attachment 538062 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #538062 - Flags: review?(jwalden+bmo) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.