1271110 - Assertion failure: fop->runtime()->gc.nursery.isEmpty(), at js/src/jit/BaselineJIT.cpp:492

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision bae525a694e2 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

// jsfunfuzz-generated
var x1 = [];
var x2 = [];
var x3 = [];
var x4 = [];
(function() {
    var gns = Object.getOwnPropertyNames(this);
    for (var i = 0; i < 49; ++i) {
        var gn = gns[i];
        var g = this[gn];
        if (typeof g == "function") {
            var hns = Object.getOwnPropertyNames(gn);
            for (var j = 0; j < hns.length; ++j) {
                x1.push("");
                x1.push("");
                x2.push("");
                x2.push("");
                x3.push("");
                x3.push("");
                x4.push("");
                x4.push("");
            }
        }
    }
})();
// Adapted from randomly chosen test: js/src/tests/ecma/extensions/11.6.2-1.js
try {
    __proto__ = function(){};
} catch (e) {
    "" + e;
}
// jsfunfuzz-generated
startgc(9222);
Function("\
    (function() {})();\
    oomTest(Debugger.Script);\
")();

Backtrace:

0   js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010c978e7b js::jit::BaselineScript::Destroy(js::FreeOp*, js::jit::BaselineScript*) + 267 (BaselineJIT.cpp:492)
1   js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010d2037ee JS::Zone::discardJitCode(js::FreeOp*) + 350 (jsscript.h:1753)
2   js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010d097858 js::ObjectGroup::sweep(js::AutoClearTypeInferenceStateOnOOM*) + 2008 (TypeInference.cpp:4442)
3   js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cfab351 js::TrackPropertyTypes(js::ExclusiveContext*, JSObject*, jsid) + 113 (ObjectGroup.h:184)
4   js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cf03c4a js::ErrorObject::init(JSContext*, JS::Handle<js::ErrorObject*>, JSExnType, js::ScopedJSFreePtr<JSErrorReport>*, JS::Handle<JSString*>, JS::Handle<JSObject*>, unsigned int, unsigned int, JS::Handle<JSString*>) + 1338 (TypeInference-inl.h:425)
5   js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cf0423d js::ErrorObject::create(JSContext*, JSExnType, JS::Handle<JSObject*>, JS::Handle<JSString*>, unsigned int, unsigned int, js::ScopedJSFreePtr<JSErrorReport>*, JS::Handle<JSString*>, JS::Handle<JSObject*>) + 365 (ErrorObject.cpp:108)
6   js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cd62111 js::ErrorToException(JSContext*, char const*, JSErrorReport*, JSErrorFormatString const* (*)(void*, unsigned int), void*) + 545 (RootingAPI.h:661)
7   js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cd54d58 js::ReportErrorNumberVA(JSContext*, unsigned int, JSErrorFormatString const* (*)(void*, unsigned int), void*, unsigned int, js::ErrorArgumentsType, __va_list_tag*) + 296 (jscntxt.cpp:226)
8   js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cce3d9e JS_ReportErrorNumber(JSContext*, JSErrorFormatString const* (*)(void*, unsigned int), void*, unsigned int, ...) + 142 (jsapi.cpp:5572)
9   js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cf00850 DebuggerScript_construct(JSContext*, unsigned int, JS::Value*) + 32 (Debugger.cpp:6245)
10  js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cf9016e js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 222 (jscntxtinlines.h:236)
11  js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cf826be js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 702 (Interpreter.cpp:468)
12  js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cf82cce js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) + 46 (Interpreter.cpp:544)
13  js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cceb453 JS_CallFunction(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSFunction*>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) + 723 (jsapi.cpp:2883)
14  js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010d1560ba OOMTest(JSContext*, unsigned int, JS::Value*) + 1162 (TestingFunctions.cpp:1309)
15  js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cf9016e js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 222 (jscntxtinlines.h:236)
16  js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010cf826be js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 702 (Interpreter.cpp:468)
17  js-dbg-64-dm-clang-darwin-bae525a694e2	0x000000010c99a056 js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 1862 (BaselineIC.cpp:5971)
18  ???                           	0x000000010e8e8dab 0 + 4539190699
19  ???                           	0x00007fa0d9076e18 0 + 140328812637720

For detailed crash information, see attachment.

GC is in the testcase -> setting s-s pending further analysis

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9f957b27ddff
user:        Jon Coppeard
date:        Thu May 05 11:01:11 2016 +0100
summary:     Bug 1264300 - Don't evict the nursery unnecessarily in ZoneCellIter r=sfink a=abillings

Jon, is bug 1264300 a likely regressor?

(fwiw, I took a long time to reduce the testcase from a much larger one, and oomTest seems involved, but so is startgc, so keeping it locked for now.)

Comment 3

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

Attachment: OOM_VERBOSE=1 stack from m-c rev bae525a694e2

Comment 4

[Fuzzing Team]

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

Comment 5

[Jon Coppeard (:jonco)]

It seems with my recent changes to cell iteration I've fixed and then broken this again.

The problem is that BaselineScripts own HeapPtrObjects as part of IC data.  If we destroy a BaselineScript outside of GC we need to evict the nursery first, or the storebuffer may end up having pointers into freed memory.  This problem can currently be triggered by AutoClearTypeInferenceStateOnOOM calling Zone::discardJitCode.

Here's a timeline of this issue:

 - Bug 945250 fixed a similar issue for calls to ReleaseAllJITCode and added an assertion (and helpful comment) to BaselineScript::Destroy.  Bug present for calls to discardJITCode via AutoClearTypeInferenceStateOnOOM.

 - Bug 1244412 removed ZoneCellIterUnderGC and made ZoneCellIter evict the nursery whenever it was called outside of a GC.  That fixed this issue.

 - Bug 1259042 reinstated ZoneCellIterUnderGC (it's useful for the hazard analysis) but discardJitCode still calls ZoneCellIter so this remains fixed.

 - Bug 1264300 changed ZoneCellIter to only evict if called for nursery types, reintroducing the bug.

Annoyingly the last bug has already been uplifted to Aurora.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1271110-discard-jit-code

I think the best immediate fix is to always evict the nursery when discarding JIT code.

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8750351 [details] [diff] [review]
bug1271110-discard-jit-code

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

For now, this is the right thing to do.

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 8750351 [details] [diff] [review]
bug1271110-discard-jit-code

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I think it would be very difficult to arrange an exploit to take advantage of this although it's theoretically possible.

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? Aurora currently, see comment 5.  

If not all supported branches, which bug introduced the flaw? Bug 1264300.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  The same patch should apply.

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

Comment 9

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

Comment on attachment 8750351 [details] [diff] [review]
bug1271110-discard-jit-code

sec-approval+ for trunk. Please check this in and nominate an aurora patch so we can get it approved and fixed on aurora as well.

Comment 10

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80d226fddf93

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80d226fddf93

Comment 13

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8750351 [details] [diff] [review]
bug1271110-discard-jit-code

Approval Request Comment
[Feature/regressing bug #]: see comment 5
[User impact if declined]: difficult to exploit UAF
[Describe test coverage new/current, TreeHerder]: it's been in m-c for a few days now
[Risks and why]: low risk, this is reverting to previous behavior
[String/UUID change made/needed]: none

The same patch applies to aurora. (At least with hg graft, it does; a straight hg import will complain about the surrounding context.)

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aea53454e86f

Comment 15

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx48

Comment 16

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx49