1250964 - Crash [@ js::jit::ICStub::markCode] or Crash [@ FromExecutable] or Crash [@ markCode] with use-after-free

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 789a12291942 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --no-threads --ion-eager):

gczeal(14, 7)[test()]
function printBugNumber(num) {
    BUGNUMBER = num;
    unescape('BUGNUMBER ' + num);
}
function test() {
    printBugNumber(test);
    function f() BUGNUMBER();
    array = [function() f()];
    for (;;) array[0]();
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::jit::ICStub::markCode (this=this@entry=0x7ffff6989020, trc=trc@entry=0x7fffffff9178, name=name@entry=0xebe4e7 "shared-stub-jitcode") at js/src/jit/SharedIC.cpp:149
#0  js::jit::ICStub::markCode (this=this@entry=0x7ffff6989020, trc=trc@entry=0x7fffffff9178, name=name@entry=0xebe4e7 "shared-stub-jitcode") at js/src/jit/SharedIC.cpp:149
#1  0x00000000007c5521 in js::jit::ICStub::trace (this=this@entry=0x7ffff6989020, trc=trc@entry=0x7fffffff9178) at js/src/jit/SharedIC.cpp:164
#2  0x00000000007c6c93 in js::jit::ICEntry::trace (this=<optimized out>, trc=trc@entry=0x7fffffff9178) at js/src/jit/SharedIC.cpp:101
#3  0x000000000068be0d in js::jit::IonScript::trace (this=0x7ffff694c800, trc=trc@entry=0x7fffffff9178) at js/src/jit/Ion.cpp:1070
#4  0x000000000068c579 in js::jit::IonScript::Trace (trc=trc@entry=0x7fffffff9178, script=<optimized out>) at js/src/jit/Ion.cpp:1260
#5  0x00000000006c17ef in MarkIonJSFrame (frame=..., trc=0x7fffffff9178) at js/src/jit/JitFrames.cpp:1024
#6  MarkJitActivation (activations=..., trc=<optimized out>) at js/src/jit/JitFrames.cpp:1422
#7  js::jit::MarkJitActivations (rt=<optimized out>, trc=trc@entry=0x7fffffff9178) at js/src/jit/JitFrames.cpp:1447
#8  0x0000000000c46299 in js::gc::GCRuntime::markRuntime (this=this@entry=0x7ffff695d430, trc=trc@entry=0x7fffffff9178, traceOrMark=traceOrMark@entry=js::gc::GCRuntime::MarkRuntime) at js/src/gc/RootMarking.cpp:324
#9  0x00000000008e1390 in js::gc::GCRuntime::updatePointersToRelocatedCells (this=this@entry=0x7ffff695d430, zone=zone@entry=0x7ffff52bc000) at js/src/jsgc.cpp:2793
#10 0x00000000009055a5 in js::gc::GCRuntime::compactPhase (this=this@entry=0x7ffff695d430, reason=reason@entry=JS::gcreason::DEBUG_GC, sliceBudget=...) at js/src/jsgc.cpp:5760
#11 0x00000000009059d2 in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff695d430, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6220
#12 0x00000000009067c0 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff695d430, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6395
#13 0x0000000000906d01 in js::gc::GCRuntime::collect (this=this@entry=0x7ffff695d430, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6501
#14 0x0000000000906f33 in js::gc::GCRuntime::gc (this=0x7ffff695d430, gckind=<optimized out>, reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6559
#15 0x000000000090859c in js::gc::GCRuntime::runDebugGC (this=this@entry=0x7ffff695d430) at js/src/jsgc.cpp:7044
#16 0x0000000000c07c7a in js::gc::GCRuntime::gcIfNeededPerAllocation (this=this@entry=0x7ffff695d430, cx=cx@entry=0x7ffff6907800) at js/src/gc/Allocator.cpp:28
#17 0x0000000000c1512f in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=0x7ffff695d430, cx=0x7ffff6907800, kind=js::gc::OBJECT4_BACKGROUND) at js/src/gc/Allocator.cpp:55
#18 0x0000000000c1a4c7 in js::Allocate<JSObject, (js::AllowGC)1> (cx=cx@entry=0x7ffff6907800, kind=kind@entry=js::gc::OBJECT4_BACKGROUND, nDynamicSlots=0, heap=heap@entry=js::gc::DefaultHeap, clasp=0x1be27c0 <js::ArrayObject::class_>) at js/src/gc/Allocator.cpp:121
#19 0x000000000053190d in js::ArrayObject::createArrayInternal (cx=0x7ffff6907800, kind=js::gc::OBJECT4_BACKGROUND, heap=js::gc::DefaultHeap, shape=..., group=...) at js/src/vm/ArrayObject-inl.h:54
#20 0x0000000000531b10 in js::ArrayObject::createArray (cx=cx@entry=0x7ffff6907800, kind=kind@entry=js::gc::OBJECT4_BACKGROUND, heap=<optimized out>, shape=..., shape@entry=..., group=..., group@entry=..., length=length@entry=1, metadata=...) at js/src/vm/ArrayObject-inl.h:82
#21 0x000000000051ae57 in NewArray<4294967295u> (cxArg=0x7ffff6907800, length=1, protoArg=..., newKind=js::GenericObject) at js/src/jsarray.cpp:3391
#22 0x000000000051b35d in NewArrayTryUseGroup<4294967295u> (cx=0x7ffff6907800, group=..., length=1, newKind=js::GenericObject, forceAnalyze=<optimized out>) at js/src/jsarray.cpp:3532
#23 0x00000000006425dd in js::jit::NewArrayWithGroup (cx=<optimized out>, length=<optimized out>, group=..., convertDoubleElements=<optimized out>) at js/src/jit/CodeGenerator.cpp:4741
[...]
#33 0x0000000000000000 in ?? ()
rax	0xe5e5e5e5e5e5e5e5	-1880844493789993499
rbx	0x7fffffff9178	140737488327032
rcx	0x3b4d4b4f2d2b2f	16692009472174895
rdx	0xebe4e7	15459559
rsi	0x7fffffff9178	140737488327032
rdi	0x7ffff6989020	140737330581536
rbp	0x7fffffff8e00	140737488326144
rsp	0x7fffffff8df0	140737488326128
r8	0x7ffff7fa20a5	140737353752741
r9	0x7ffff51ae5ff	140737305568767
r10	0x56cdedd0	1456336336
r11	0x1	1
r12	0x7fffffff9178	140737488327032
r13	0x7ffff6989020	140737330581536
r14	0x7fffffff8ef0	140737488326384
r15	0x7fffffff9178	140737488327032
rip	0x7c544b <js::jit::ICStub::markCode(JSTracer*, char const*)+11>
=> 0x7c544b <js::jit::ICStub::markCode(JSTracer*, char const*)+11>:	mov    -0x8(%rax),%rcx
   0x7c544f <js::jit::ICStub::markCode(JSTracer*, char const*)+15>:	cmp    (%rcx),%rax



Crash indicates use-after-free and involves the GC. Marking s-s and sec-critical.

Comment 1

[Jan de Mooij [:jandem]]

Looks like shared stubs.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Which branches does this affect?

Comment 3

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

Bug 1241087 turned on shared stubs for Firefox 47 onwards, so it's just 47 for now if that's what you're wondering.

I'm running another bisection with --ion-shared-stubs=on to find out the actual regressor.

Comment 4

[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/f08d8ca4ca83
user:        Hannes Verschore
date:        Wed Aug 19 15:15:57 2015 +0200
summary:     Bug 1171945: IonMonkey - Part 6: Use binarystub in jsop_binary_arith, r=jandem

Hannes, is bug 1171945 a likely regressor?

Note: I removed the infinite loop in the testcase in comment 0 so the testcase I tested with is:

gczeal(14, 7)[test()]
function printBugNumber(num) {
    BUGNUMBER = num;
    unescape('BUGNUMBER ' + num);
}
function test() {
    printBugNumber(test);
    function f() BUGNUMBER();
    array = [function() f()];
    for (var i = 0; i < 2; i++) array[0]();
}

Comment 5

[Naveed Ihsanullah [:naveed]]

Hannes please take a look and re-assign as needed. Thanks.

Comment 6

[Hannes Verschore [:h4writer]]

We are adding shared stubs to IonScripts that have been invalidated. That shouldn't happen. We should find a way to know the IonScript has been invalidated. Which is annoying currently, since we have the outerScript and can get the ionScript, but it is quite expensive. Therefore we only do it when we really want to add a stub, instead of always check it.
Searching for a way to do this cleanest

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Don't add stubs if IonScript is invalidated

The problem is that we still can add stubs when the IonScript has been invalidated. That made a worrisome problem. A stub new stub cold get added in the optimized stub space, where-after a gc could throw it away. But we wouldn't delete/purge it from the invalidated ionscript.

I added a way to invalid fallback stubs, which happens upon purging the caches from an IonScript. When having a stub that is invalid, we cannot add new stubs.

I reused DebugModeOSRVolatileStub to do the check. But I can open follow-up bug to rename it to InvalidationStubChecker?

Comment 8

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #7)
> I reused DebugModeOSRVolatileStub to do the check. But I can open follow-up
> bug to rename it to InvalidationStubChecker?

Didn't want to do it directly, since that would increase complexity of patch and possibly need some name suggestions. IMHO it would be better to do that in non-security bug, without pressure.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8727781 [details] [diff] [review]
Don't add stubs if IonScript is invalidated

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

::: js/src/jit/SharedIC.cpp
@@ +930,5 @@
>          MOZ_ASSERT(it.isExitFrame());
>          ++it;
>          MOZ_ASSERT(it.isIonJS());
>          outerScript_ = it.script();
> +        MOZ_ASSERT(!it.ionScript()->invalidated());

Nit: can we also MOZ_ASSERT(!invalid()) in addNewStub, and maybe also addOptimizedMonitorStub?

Comment 10

[Hannes Verschore [:h4writer]]

Attachment: Don't add stubs if IonScript is invalidated

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Non-trivial.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Only that sometimes something is invalid and shouldn't get used, but no information on when this is the case.

Which older supported branches are affected by this flaw?
Firefox 47 has this issue too

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

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

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

Comment 11

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

Comment on attachment 8730120 [details] [diff] [review]
Don't add stubs if IonScript is invalidated

sec-approval+ for trunk.
Since this affects Aurora, we'll want a patch made for it and nominated there as well.

Comment 12

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

Looks like this landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ddac87ea0c and had a followup land in https://hg.mozilla.org/integration/mozilla-inbound/rev/f337b2e34093

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/a4ddac87ea0c
https://hg.mozilla.org/mozilla-central/rev/f337b2e34093

Comment 14

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 15

[Hannes Verschore [:h4writer]]

Comment on attachment 8730120 [details] [diff] [review]
Don't add stubs if IonScript is invalidated

Approval Request Comment
[Feature/regressing bug #]:
bug 1241087

[User impact if declined]:
Crash with potential attack vector

[Describe test coverage new/current, TreeHerder]:
In tree for 2-3 days + passes all jit-tests.

[Risks and why]:
Added an extra check before adding shared stubs to ion. Not adding a stub is never an issue. Therefore the risk shouldn't be that high.


[String/UUID change made/needed]:
/

Comment 16

[Hannes Verschore [:h4writer]]

Attachment: Rolled up patch

This includes the removal of the bogus assert that was noticed on inbound.

Approval Request Comment
[Feature/regressing bug #]:
bug 1241087

[User impact if declined]:
Crash with potential attack vector

[Describe test coverage new/current, TreeHerder]:
In tree for 2-3 days + passes all jit-tests.

[Risks and why]:
Added an extra check before adding shared stubs to ion. Not adding a stub is never an issue. Therefore the risk shouldn't be that high.


[String/UUID change made/needed]:
/

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8732113 [details] [diff] [review]
Rolled up patch

Sec-crit issue, taking it. Aurora47+

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/20bc3ca96c29
https://hg.mozilla.org/releases/mozilla-aurora/rev/ba8aa7ee3679

Comment 19

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx47

Comment 20

[Benjamin Smedberg]

This crash signature shows up in the Firefox 46 e10s experiment. Is it possible that this bug affects 46 with enabled e10s?

If not, should I file a separate bug to track the possibility that there is a separate bug with e10s and the same symptoms?

Comment 21

[Hannes Verschore [:h4writer]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #20)
> This crash signature shows up in the Firefox 46 e10s experiment. Is it
> possible that this bug affects 46 with enabled e10s?
> 
> If not, should I file a separate bug to track the possibility that there is
> a separate bug with e10s and the same symptoms?

This was only enabled in FF47. Therefore I think having a separate bug would indeed be good!
What you are seeing should be something else if this happens in FF46.