1205937 - Crash [@ js::gc::IsInsideNursery(js::gc::Cell const*)]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision e7d613b3bcfe (build with --enable-gczeal --enable-optimize="-O2 -g" --enable-address-sanitizer --enable-posix-nspr-emulation --disable-jemalloc --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2 --ion-extra-checks --ion-eager):

gczeal(7,1);
eval(`
var otherGlobalSameCompartment = newGlobal("same-compartment");
eval = otherGlobalSameCompartment.eval;
`);
loadFile(`
/**







Yes, I'm a multi-line comment spanning 18 lines.
Don't you dare to make me smaller or larger...







*/
new TestCase( 
              SECTION,
              "MakeDate(Number.POSITIVE_INFINITY, 0)",
              Number.NaN,
              MakeDate(Number.POSITIVE_INFINITY, 0)
);
new TestCase( 
              SECTION,
              "MakeDate(Number.NEGATIVE_INFINITY, 0)",
              Number.NaN,
              NaN[MakeDate+4]
);
new TestCase( 
              SECTION,
              "MakeDate(0, Number.POSITIVE_INFINITY)",
              Number.target ,
              MakeDate(0, Number.POSITIVE_INFINITY)
);
new TestCase( 
	      SECTION,
              "MakeDate(0, Number.NEGATIVE_INFINITY)",
              Number.NaN,
              MakeDate(0, ( (yield    )  , SECTION   ? (yield    )  : (yield    )        , (yield    )       ) .NEGATIVE_INFINITY)
);
test();
`);
function loadFile(lfVarx) {
  eval("(function() { " + lfVarx + " })();"); 
}



Backtrace:

==65394==ERROR: AddressSanitizer: SEGV on unknown address 0x7fff000fffe8 (pc 0x000000dbe2f9 sp 0x7fffb39d1540 bp 0x7fffb39d1630 T0)
    #0 0xdbe2f8 in js::gc::IsInsideNursery(js::gc::Cell const*) js/src/opt64asan/js/src/../../dist/include/js/HeapAPI.h:312
    #1 0xdbe2f8 in void js::TenuringTracer::traverse<JSObject*>(JSObject**) js/src/gc/Marking.cpp:1888
    #2 0xdbe2f8 in void js::TenuringTracer::traverse<JS::Value>(JS::Value*) js/src/gc/Marking.cpp:1900
    #3 0xdbe2f8 in void DispatchToTracer<JS::Value>(JSTracer*, JS::Value*, char const*) js/src/gc/Marking.cpp:602
    #4 0xea31e5 in js::jit::BaselineFrame::trace(JSTracer*, js::jit::JitFrameIterator&) js/src/jit/BaselineFrame.cpp:53
    #5 0x11d0996 in js::jit::MarkJitActivation(JSTracer*, js::jit::JitActivationIterator const&) js/src/jit/JitFrames.cpp:1546
    #6 0x11d0996 in js::jit::MarkJitActivations(JSRuntime*, JSTracer*) js/src/jit/JitFrames.cpp:1581
    #7 0xd4d9e5 in js::gc::GCRuntime::markRuntime(JSTracer*, js::gc::GCRuntime::TraceOrMarkRuntime) js/src/gc/RootMarking.cpp:332
    #8 0xd48366 in js::Nursery::collect(JSRuntime*, JS::gcreason::Reason, js::Vector<js::ObjectGroup*, 0ul, js::SystemAllocPolicy>*) js/src/gc/Nursery.cpp:460
    #9 0x175214d in js::gc::GCRuntime::minorGCImpl(JS::gcreason::Reason, js::Vector<js::ObjectGroup*, 0ul, js::SystemAllocPolicy>*) js/src/jsgc.cpp:6502
    #10 0x175214d in js::gc::GCRuntime::minorGC(JS::gcreason::Reason) js/src/gc/GCRuntime.h:606
    #11 0x16aca64 in js::gc::GCRuntime::runDebugGC() js/src/jsgc.cpp:6725
    #12 0x8e692f in js::gc::GCRuntime::gcIfNeededPerAllocation(JSContext*) js/src/gc/Allocator.cpp:28
    #13 0x90e9b6 in JSContext::runtime() const js/src/gc/Allocator.cpp:55
    #14 0x90e9b6 in js::BaseShape* js::Allocate<js::BaseShape, (js::AllowGC)1>(js::ExclusiveContext*) js/src/gc/Allocator.cpp:211
    #15 0xb6e66a in js::BaseShape::getUnowned(js::ExclusiveContext*, js::StackBaseShape&) js/src/vm/Shape.cpp:1262
    #16 0xb724bb in js::EmptyShape::getInitialShape(js::ExclusiveContext*, js::Class const*, js::TaggedProto, unsigned long, unsigned int) js/src/vm/Shape.cpp:1502
    #17 0x16d22e5 in NewObject(js::ExclusiveContext*, JS::Handle<js::ObjectGroup*>, js::gc::AllocKind, js::NewObjectKind, unsigned int) js/src/jsobj.cpp:678
    #18 0x16d1ad4 in js::NewObjectWithGivenTaggedProto(js::ExclusiveContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind, unsigned int) js/src/jsobj.cpp:745:29
    #19 0x9a846d in js::NewObjectWithGivenTaggedProto(js::ExclusiveContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::NewObjectKind, unsigned int) js/src/jsobjinlines.h:641
    #20 0x9a846d in js::NewObjectWithGivenProto(js::ExclusiveContext*, js::Class const*, JS::Handle<JSObject*>, js::NewObjectKind) js/src/jsobjinlines.h:676
    #21 0x9a846d in js::NewNativeObjectWithGivenProto(js::ExclusiveContext*, js::Class const*, JS::Handle<JSObject*>, js::NewObjectKind) js/src/vm/NativeObject-inl.h:337
    #22 0x9a846d in CreateBlankProto(JSContext*, js::Class const*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) js/src/vm/GlobalObject.cpp:472
    #23 0x93a286 in js::GlobalObject::createBlankPrototype(JSContext*, js::Class const*) js/src/vm/GlobalObject.cpp:488
    #24 0x16c0170 in js::GlobalObject::initIteratorClasses(JSContext*, JS::Handle<js::GlobalObject*>) js/src/jsiter.cpp:1383
    #25 0x16c16ba in js::InitIteratorClasses(JSContext*, JS::Handle<JSObject*>) js/src/jsiter.cpp:1453
    #26 0x9a4dd2 in js::GlobalObject::resolveConstructor(JSContext*, JS::Handle<js::GlobalObject*>, JSProtoKey) js/src/vm/GlobalObject.cpp:133
    #27 0x99e890 in js::GlobalObject::ensureConstructor(JSContext*, JS::Handle<js::GlobalObject*>, JSProtoKey) js/src/vm/GlobalObject.cpp:100
    #28 0x99e890 in js::GlobalObject::getOrCreateLegacyGeneratorObjectPrototype(JSContext*, JS::Handle<js::GlobalObject*>) js/src/vm/GlobalObject.h:555
    #29 0x99e890 in js::GeneratorObject::create(JSContext*, js::AbstractFramePtr) js/src/vm/GeneratorObject.cpp:41

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV js/src/opt64asan/js/src/../../dist/include/js/HeapAPI.h:312 js::gc::IsInsideNursery(js::gc::Cell const*)
==65394==ABORTING


Honestly I have no clue what's going on here. This test crashes only in 64 bit ASan builds, not in any other build type I tried. It also only crashes when the exact number of line breaks is preserved in that second template string.

Comment 1

[Christian Holler (:decoder)]

Fwiw, I used this build to reproduce:

https://fuzzing.own-hero.net/builds/jsshell-mc-64-opt-asan-latest.zip

Comment 2

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 3

[Andrew McCreight [:mccr8]]

Waldo, do you know who might look at an issue with template strings? Thanks.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Testcase as attachment

In a valgrind, debug/no-opt build I get this:

[jwalden@find-waldo-now src]$ valgrind dbg/js/src/js --ion-extra-checks --ion-eager -f /tmp/whitespace.js 
==10498== Memcheck, a memory error detector
==10498== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==10498== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==10498== Command: dbg/js/src/js --ion-extra-checks --ion-eager -f /tmp/whitespace.js
==10498== 
==10498== Conditional jump or move depends on uninitialised value(s)
==10498==    at 0x439212: JSVAL_IS_OBJECT_IMPL(jsval_layout) (Value.h:804)
==10498==    by 0x453AEF: JS::Value::isObject() const (Value.h:1142)
==10498==    by 0x9AA9F3: void js::TenuringTracer::traverse<JS::Value>(JS::Value*) (Marking.cpp:1914)
==10498==    by 0x9EA081: void DispatchToTracer<JS::Value>(JSTracer*, JS::Value*, char const*) (Marking.cpp:612)
==10498==    by 0x9EA1A0: void js::TraceRoot<JS::Value>(JSTracer*, JS::Value*, char const*) (Marking.cpp:465)
==10498==    by 0xA607D0: js::jit::BaselineFrame::trace(JSTracer*, js::jit::JitFrameIterator&) (BaselineFrame.cpp:53)
==10498==    by 0xBB22E5: js::jit::MarkJitActivation(JSTracer*, js::jit::JitActivationIterator const&) (JitFrames.cpp:1546)
==10498==    by 0xBB21F0: js::jit::MarkJitActivations(JSRuntime*, JSTracer*) (JitFrames.cpp:1581)
==10498==    by 0x9A675B: js::gc::GCRuntime::markRuntime(JSTracer*, js::gc::GCRuntime::TraceOrMarkRuntime) (RootMarking.cpp:316)
==10498==    by 0x9B0912: js::Nursery::collect(JSRuntime*, JS::gcreason::Reason, js::Vector<js::ObjectGroup*, 0ul, js::SystemAllocPolicy>*) (Nursery.cpp:461)
==10498==    by 0xE6ED7E: js::gc::GCRuntime::minorGCImpl(JS::gcreason::Reason, js::Vector<js::ObjectGroup*, 0ul, js::SystemAllocPolicy>*) (jsgc.cpp:6534)
==10498==    by 0xEAE16A: js::gc::GCRuntime::minorGC(JS::gcreason::Reason) (GCRuntime.h:606)
==10498== 
==10498== Conditional jump or move depends on uninitialised value(s)
==10498==    at 0x9AA9F6: void js::TenuringTracer::traverse<JS::Value>(JS::Value*) (Marking.cpp:1914)
==10498==    by 0x9EA081: void DispatchToTracer<JS::Value>(JSTracer*, JS::Value*, char const*) (Marking.cpp:612)
==10498==    by 0x9EA1A0: void js::TraceRoot<JS::Value>(JSTracer*, JS::Value*, char const*) (Marking.cpp:465)
==10498==    by 0xA607D0: js::jit::BaselineFrame::trace(JSTracer*, js::jit::JitFrameIterator&) (BaselineFrame.cpp:53)
==10498==    by 0xBB22E5: js::jit::MarkJitActivation(JSTracer*, js::jit::JitActivationIterator const&) (JitFrames.cpp:1546)
==10498==    by 0xBB21F0: js::jit::MarkJitActivations(JSRuntime*, JSTracer*) (JitFrames.cpp:1581)
==10498==    by 0x9A675B: js::gc::GCRuntime::markRuntime(JSTracer*, js::gc::GCRuntime::TraceOrMarkRuntime) (RootMarking.cpp:316)
==10498==    by 0x9B0912: js::Nursery::collect(JSRuntime*, JS::gcreason::Reason, js::Vector<js::ObjectGroup*, 0ul, js::SystemAllocPolicy>*) (Nursery.cpp:461)
==10498==    by 0xE6ED7E: js::gc::GCRuntime::minorGCImpl(JS::gcreason::Reason, js::Vector<js::ObjectGroup*, 0ul, js::SystemAllocPolicy>*) (jsgc.cpp:6534)
==10498==    by 0xEAE16A: js::gc::GCRuntime::minorGC(JS::gcreason::Reason) (GCRuntime.h:606)
==10498==    by 0xE6FC6A: js::gc::GCRuntime::runDebugGC() (jsgc.cpp:6757)
==10498==    by 0x6CEF5E: js::gc::GCRuntime::gcIfNeededPerAllocation(JSContext*) (Allocator.cpp:28)
==10498== 
==10498== 
==10498== HEAP SUMMARY:
==10498==     in use at exit: 0 bytes in 0 blocks
==10498==   total heap usage: 0 allocs, 0 frees, 0 bytes allocated
==10498== 
==10498== All heap blocks were freed -- no leaks are possible
==10498== 
==10498== For counts of detected and suppressed errors, rerun with: -v
==10498== Use --track-origins=yes to see where uninitialised values come from
==10498== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

which appears to be the same issue.  (My clang setup doesn't quite have working ASAN capabilities, although maybe if I refreshed it to tip it might -- perhaps worth doing soon.)  Investigating more.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Tentative patch, passes ion/ tests with --ion-eager as plausible smoketest

After circuitous investigation by efaust and me, it appears that BaselineFrame shouldn't be tracing evalNewTarget when we're doing a global eval -- nothing wrong anywhere else.  This extra check corresponds with what SetEnterJitData does, in terms of testing isForEval (in effect) and then checking that we're not looking at a global frame before tracing evalNewTarget.

As far as possible mistakes, using the wrong verb seems like the only tricky one here.  You know more about the verbs available, and what's appropriate, than I do.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Minimal-est testcase that hits valgrind complaints -- depressingly, a two-liner

Given the only failure things here are if you get BaselineFrame tracing, I dunno if we can get a reproducible testcase that we can land.  I'll see what I can do in an hour-ish after an errand.

Comment 7

[Jeff Walden [:Waldo]]

efaust suggests that maybe in this code in SetEnterJitData, the NullValue() append is unnecessary:

        if (state.script()->isForEval() &&
            !(state.asExecute()->type() & InterpreterFrame::GLOBAL))
        {
            ScriptFrameIter iter(cx);
            if (iter.isFunctionFrame())
                data.calleeToken = CalleeToToken(iter.callee(cx), /* constructing = */ false);

            // Push newTarget onto the stack, as well as Argv.
            if (!vals.reserve(2))
                return false;

            data.maxArgc = 2;
            data.maxArgv = vals.begin();
            vals.infallibleAppend(state.asExecute()->thisv());
            if (iter.isFunctionFrame()) {
                if (state.asExecute()->newTarget().isNull())
                    vals.infallibleAppend(iter.newTarget());
                else
                    vals.infallibleAppend(state.asExecute()->newTarget());
            } else {
                vals.infallibleAppend(NullValue()); // <---
            }
        }

It *appears* that arm would only be hit for module code.  Maybe.  If you can have eval in modules, which I'm not sure about.  Paging jonco to discuss this; seems like this should turn into an assert-not-reached or so, if modules can't hit it.  At least in theory.  I haven't tested removal of this in the slightest, even locally.

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> If you can have eval in modules

You can have eval in modules (modules are always strict however).

I don't understanding exactly what's going on here - is this path executed for a function inside an eval?

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8664468 [details] [diff] [review]
Tentative patch, passes ion/ tests with --ion-eager as plausible smoketest

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

::: js/src/jit/BaselineFrame.cpp
@@ +50,5 @@
>  
>      if (isEvalFrame()) {
>          TraceRoot(trc, &evalScript_, "baseline-evalscript");
> +        if (!isGlobalFrame())
> +            TraceRoot(trc, evalNewTargetAddress(), "baseline-evalNewTarget");

Should evalNewTargetAddress MOZ_ASSERT(isFunctionFrame()) ?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> As far as possible mistakes, using the wrong verb seems like the only tricky
> one here.  You know more about the verbs available, and what's appropriate,
> than I do.

For each baseline/interpreter frame, either isGlobalFrame() or isFunctionFrame() returns true. In addition, both frame types can be marked as eval frames. So checking either !isGlobalFrame() or isFunctionFrame() is fine.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> efaust suggests that maybe in this code in SetEnterJitData, the NullValue()
> append is unnecessary:

There's a similar infallibleAppend(NullValue()) in EnterBaselineAtBranch.

Comment 11

[Jeff Walden [:Waldo]]

Just landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/62ab5e142af3

...and then, thinking more, I remembered that |new.target| is *not* relevant just to ES6 classes, that are only enabled in nightly builds and haven't made their way into any other branches/releases yet.  So this probably shouldn't have landed yet.  Pfui.  :-(  Time to request post-hoc approvals, I guess.  :-(

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Patch as landed on trunk

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Tricky.  As originally presented, it depends on precise GC timing, that may be mostly impossible to make happen deliberately.  But I'm not certain the problem is fully limited to that case.

> Do comments in the patch, the check-in comment, or tests included
> in the patch paint a bulls-eye on the security problem?

No.

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?

Probably bug 1141865, Mozilla 41, which introduced new.target.

> Do you have backports for the affected branches? If not, how
> different, hard to create, and risky will they be?

Patch likely applies straightforwardly to branches.

> How likely is this patch to cause regressions; how much
> testing does it need?

Patch is pretty clear, regressions seem unlikely.  I expect fuzzers will be more productive at analyzing it, than any amount of in-field testing, given the requirement for eval and a nested function and the GC-trickiness and such.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Patch to be landed on trunk

Previous patch got backed out -- turns out, some function frames don't have the function bit set in them, because asExecute()->type() is not the same bits as InterpreterFrame::*.  So I reverted that extra tweak to the original patch, that simply tests isFunctionFrame() in BaselineFrame::trace and asserts it as well in evalNewTargetAddress().

This patch, chugging away on try, should be good.  https://treeherder.mozilla.org/#/jobs?repo=try&revision=234497d74e6c  sec-approval comments above still apply here.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8665876 [details] [diff] [review]
Patch to be landed on trunk

sec-approval+ for trunk. We should get this on Aurora and Beta too after it goes onto trunk.

Comment 15

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70a14c7444e5

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 8665876 [details] [diff] [review]
Patch to be landed on trunk

Approval Request Comment
[Feature/regressing bug #]: probably bug 1141865 introducing new.target
[User impact if declined]: crashes, not certain if exploitable
[Describe test coverage new/current, TreeHerder]: requires very very particular GC timing requirements, so believed not testable
[Risks and why]: low -- certainly new.target doesn't have relevance to non-function frames, so the patch clearly makes us do less work in a place where that work avoided makes no sense
[String/UUID change made/needed]: N/A

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70a14c7444e5

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8665876 [details] [diff] [review]
Patch to be landed on trunk

Fix a crash, taking it.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34f6e9cb20a6

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5290319f9c0a