1817933 - Assertion failure: (asBits_ & js::gc::CellAlignMask) == 0 (GC pointer is not aligned. Is this memory corruption?) coming from js::DebuggerFrame::evalcoming from

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Samuel Groß]

The following sample triggers an assertion failure on debug builds of Spidermonkey from current HEAD:

const v4 = typeof reportCompare;
with (this) {
    function f7() {
        const o10 = {
        };
        with (o10) {
            const o15 = {
                "newCompartment": true,
            };
            try {
                x.reduce(x, v4, f7, v4, o10);
            } catch(e18) {
            }
            const t49 = newGlobal(o15).Debugger;
            const t50 = t49().constructor;
            t50(Uint8ClampedArray).getNewestFrame().eval("var x = 23;");
            typeof reportCompare;
            try {
                PI.fill();
            } catch(e36) {
            }
        }
        typeof x;
        return o;
    }
    Object.prototype.__defineGetter__("x", f7);
    x;
}
// CRASH INFO
// ==========
// TERMSIG: 11
// STDERR:
// Assertion failure: (asBits_ & js::gc::CellAlignMask) == 0 (GC pointer is not aligned. Is this memory corruption?), at /home/builder/firefox/obj-fuzzbuild/dist/include/js/Value.h:733
// #01: JS::Value::toObject() const[./spidermonkey/js +0x186b49f]
// #02: ???[./spidermonkey/js +0x1952764]
// #03: ???[./spidermonkey/js +0x2605fa1]
// #04: ??? (???:???)
// STDOUT:
// 
// EXECUTION TIME: 224ms

Here is (a part of) the backtrace from gdb:

#0  0x00005555573991fd in JS::Value::unboxGCPointer<JSObject, (JSValueTag)131068> (this=0x7fffffdffe00) at obj-debug/dist/include/js/Value.h:732
#1  0x00005555573802a1 in JS::Value::toObject (this=0x7fffffdffe00) at obj-debug/dist/include/js/Value.h:943
#2  0x0000555557536821 in js::TypeOfValue (v=...) at js/src/vm/Interpreter.cpp:927
#3  0x0000555558388199 in js::jit::DoTypeOfFallback (cx=0x7ffff772f100, frame=0x7fffffdffe08, stub=0x7ffff56b1540, val=..., res=...) at js/src/jit/BaselineIC.cpp:1972
#4  0x000025fe59e60030 in ?? ()
...
#9  0x00005555594408b0 in js::jit::tailCallVMFunctions ()
#10 0x00007fffffdffe50 in ?? ()
...
#22 0x00005555583bef50 in mozilla::detail::PoisonObject<JS::AutoAssertNoGC> (p=0xfff9800000000000) at obj-debug/dist/include/mozilla/Maybe.h:89
#23 0x000025fe59e44d89 in ?? ()
#24 0x0000000000000003 in ?? ()
#25 0x0000024c11d61ee8 in ?? ()
#26 0xfffe03be97400798 in ?? ()
#27 0x00005555583beef5 in mozilla::Maybe<JS::AutoAssertNoGC>::poisonData (this=0x25fe59e6ee68) at obj-debug/dist/include/mozilla/Maybe.h:367
#28 0x00005555588cc3e1 in EnterJit (cx=0x7ffff772f100, state=..., code=0x25fe59e86870 "UH\213\354\351#") at js/src/jit/Jit.cpp:104
#29 0x00005555588cbc0d in js::jit::MaybeEnterJit (cx=0x7ffff772f100, state=...) at js/src/jit/Jit.cpp:205
#30 0x000055555751cf42 in js::RunScript (cx=0x7ffff772f100, state=...) at js/src/vm/Interpreter.cpp:421
#31 0x00005555575344f3 in js::InternalCallOrConstruct (cx=0x7ffff772f100, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Getter) at js/src/vm/Interpreter.cpp:579
#32 0x0000555557534a69 in InternalCall (cx=0x7ffff772f100, args=..., reason=js::CallReason::Getter) at js/src/vm/Interpreter.cpp:614
#33 0x0000555557534c19 in js::Call (cx=0x7ffff772f100, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Getter) at js/src/vm/Interpreter.cpp:646
#34 0x00005555575359a7 in js::CallGetter (cx=0x7ffff772f100, thisv=..., getter=..., rval=...) at js/src/vm/Interpreter.cpp:768
#35 0x000055555787675a in CallGetter (cx=0x7ffff772f100, obj=..., receiver=..., id=..., prop=..., vp=...) at js/src/vm/NativeObject.cpp:2017
#36 0x000055555786c617 in GetExistingProperty<(js::AllowGC)1> (cx=0x7ffff772f100, receiver=..., obj=..., id=..., prop=..., vp=...) at js/src/vm/NativeObject.cpp:2045
#37 0x000055555786cdfe in NativeGetPropertyInline<(js::AllowGC)1> (cx=0x7ffff772f100, obj=..., receiver=..., id=..., nameLookup=NotNameLookup, vp=...) at js/src/vm/NativeObject.cpp:2193
#38 0x000055555786cb65 in js::NativeGetProperty (cx=0x7ffff772f100, obj=..., receiver=..., id=..., vp=...) at js/src/vm/NativeObject.cpp:2224
#39 0x00005555573ceaea in js::GetProperty (cx=0x7ffff772f100, obj=..., receiver=..., id=..., vp=...) at js/src/vm/ObjectOperations-inl.h:118
#40 0x0000555557720053 in with_GetProperty (cx=0x7ffff772f100, obj=..., receiver=..., id=..., vp=...) at js/src/vm/EnvironmentObject.cpp:793
#41 0x00005555573cea94 in js::GetProperty (cx=0x7ffff772f100, obj=..., receiver=..., id=..., vp=...) at js/src/vm/ObjectOperations-inl.h:115
#42 0x00005555573ce923 in js::GetProperty (cx=0x7ffff772f100, obj=..., receiver=..., id=..., vp=...) at js/src/vm/ObjectOperations-inl.h:132
#43 0x000055555770df8d in (anonymous namespace)::DebugEnvironmentProxyHandler::getMaybeSentinelValue (this=0x5555593ffd40 <(anonymous namespace)::DebugEnvironmentProxyHandler::singleton>, cx=0x7ffff772f100, debugEnv=..., id=..., vp=...) at js/src/vm/EnvironmentObject.cpp:2243
#44 0x000055555770dbb9 in js::DebugEnvironmentProxy::getMaybeSentinelValue (cx=0x7ffff772f100, env=..., id=..., vp=...) at js/src/vm/EnvironmentObject.cpp:2492
#45 0x00005555577f70d0 in js::LookupNameUnqualified (cx=0x7ffff772f100, name=..., envChain=..., objp=...) at js/src/vm/JSObject.cpp:1653
#46 0x0000555557521940 in Interpret (cx=0x7ffff772f100, state=...) at js/src/vm/Interpreter.cpp:2603
#47 0x000055555751cfd1 in js::RunScript (cx=0x7ffff772f100, state=...) at js/src/vm/Interpreter.cpp:431
#48 0x0000555557535eb1 in js::ExecuteKernel (cx=0x7ffff772f100, script=..., envChainArg=..., evalInFrame=..., result=...) at js/src/vm/Interpreter.cpp:812
#49 0x0000555557daf5b3 in EvaluateInEnv (cx=0x7ffff772f100, env=..., frame=..., chars=..., evalOptions=..., rval=...) at js/src/debugger/Frame.cpp:1000
#50 0x0000555557daed01 in js::DebuggerGenericEval (cx=0x7ffff772f100, chars=..., bindings=..., options=..., dbg=0x7ffff4f8e400, envArg=..., iter=0x7fffffe04690) at js/src/debugger/Frame.cpp:1086
#51 0x0000555557daf815 in js::DebuggerFrame::eval (cx=0x7ffff772f100, frame=..., chars=..., bindings=..., options=...) at js/src/debugger/Frame.cpp:1105

I'm filing this as a security issue, however as the crash appears to be related to the Debugger API, the bug may not be reachable from web content.

Comment 1

[Nicolas B. Pierron [:nbp]]

While I would agree about the Debugger being a lower priority, the fact that this code is returning to Baseline JIT-ed code (tailCallVMFunction), one would expect that we should be able to handle all cases safely without complex machinery, and the only failure case would be if a poisoned Value appears in the baseline frame (which seems suggested by frame #22, but I do not know how much we can trust this frame as I would not expect it to re-enter JS evaluation cycle, especially given the <AutoAssertNoGC>).

Jan, would you mind looking at this issue, in case this hides something else?

Comment 2

[Iain Ireland [:iain]]

Reduced testcase:

with (this) {
    Object.prototype.__defineGetter__("x", () => {
        const o = {};
        with (o) {
            try { x } catch {}
	    var g = newGlobal({"newCompartment": true});
            const dbg = new g.Debugger(this);
            dbg.getNewestFrame().eval("var x = 0;");
        }
	typeof x // ***Fails here***
    });
    x;
}

For reasons I haven't completely worked out yet, when we load x out of the environment for the line marked above, we read it out of an uninitialized slot, returning a poison value and crashing when we try to access it. The code that does the load is attached by GetNameIRGenerator::tryAttachEnvironmentName. I noticed while poking the code in rr that we call NeedEnvironmentShapeGuard to decide whether we can skip a shapeguard for a call object with immutable bindings. I have a strong suspicion that the evalInFrame is invalidating our assumption here by mutating immutable bindings. If I force NeedEnvironmentShapeGuard to return true, the bug goes away.

Comment 3

[Iain Ireland [:iain]]

Here's a more straightforward version of the bug:

var g = newGlobal({"newCompartment": true});
const dbg = new g.Debugger(this);

with ({x: 3}) { // force getname
    function foo(n) {
	() => {n} // force call object

	if (n < 20) {
	    foo(n+1);
	} else {
    	    dbg.getNewestFrame().eval("var x = 23;");
	}

	+x // Access x.
    }
    foo(0);
}

We call foo recursively often enough to get into blinterp, where we can attach ICs. Then we use the debugger to eval var x = 23 in the most recent frame. This sets a property on the call object for foo*. When we access x on the next line, we attach a GetName IC (because we're wrapped in a with), which finds x on the call object and attaches an IC accordingly. However, because the call object has "immutable" bindings, NeedEnvironmentShapeGuard decides to skip the shape guard and load directly from the slot.

When we return to the calling frame, we hit the IC we just attached and load from the slot of the call object. However, this call object wasn't the target of an eval, and doesn't have a value in that slot. We return nursery poison.

This is a debugger-only bug. I'm not sure what the best fix is: maybe NeedEnvironmentShapeGuard should check to see if the call object has ever been mutated by the debugger? The easy fix is to always generate the shape guard, but it seems unfortunate to pessimize this code to work around a debugger-only corner case.

Comment 4

[Iain Ireland [:iain]]

Another place we could consider looking to fix this bug is in DebugEnvironmentProxyHandler::defineProperty. This is the code that adds the new property to the CallObject in the first place. My mental model for how DebugEnvironmentProxy should work is fuzzy, but it doesn't seem immediately unreasonable to propose that variables defined in the debugger could live on the proxy itself, not the call object. Or maybe we should just throw an error if somebody tries defining a property on a call object for a script with immutable bindings; it's not clear to me how useful this functionality is in the first place.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1817933: Don't elide shapeguard for debugger targets r=jandem

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee39fbb05210

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta and ESR? Please nominate if so.

Comment 9

[Iain Ireland [:iain]]

This is debugger-only. It can ride the trains.

Comment 10

[Daniel Veditz [:dveditz]]

This does not appear to be exploitable from web content, and no plausible evidence you could convince our dev tools to do anything like this. Changing to sec-other.

Comment 11

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons