1369994 - Assertion failure: isInt32(), at dist/include/js/Value.h:605

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision aeb3d0ca558f (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --ion-offthread-compile=off --baseline-eager):

var lfLogBuffer = `
function f() {
    var i32 = new Int32Array(1);
    var f32 = new Float32Array(i32.buffer);
    for (var i = 0; i < 3; i++) {
      var { regExp, get,     }  = gczeal(9,10) 
        ? (yield) : (yield) = call(f32, "i32.store", []);
    }
}
f();
`;
loadFile(lfLogBuffer);
function loadFile(lfVarx) {
    try {
      oomTest(function() { eval(lfVarx); });
    } catch (lfVare) {}
}



Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000000000067ffc0 in JS::Value::toInt32 (this=<optimized out>) at dist/include/js/Value.h:605
#1  js::TypedArrayObject::length (this=this@entry=0x7ff2e4299100) at js/src/vm/TypedArrayObject.h:164
#2  0x0000000000c43f11 in js::TypedArrayObject::assertZeroLengthArrayData (this=0x7ff2e4299100) at js/src/vm/TypedArrayObject.cpp:147
#3  0x0000000000c43f7f in js::TypedArrayObject::elements (this=0x7ff2e4299100) at js/src/vm/TypedArrayObject.h:173
#4  js::TypedArrayObject::hasInlineElements (this=0x7ff2e4299100) at js/src/vm/TypedArrayObject.cpp:272
#5  0x0000000000c486b0 in js::TypedArrayObject::objectMoved (obj=<optimized out>, old=0x7ff2e4299100) at js/src/vm/TypedArrayObject.cpp:194
#6  0x00000000009c81c3 in RelocateCell (thingSize=96, thingKind=js::gc::AllocKind::OBJECT8_BACKGROUND, src=0x7ff2e4299100, zone=0x7ff2e40e4000) at js/src/jsgc.cpp:1993
#7  RelocateArena (sliceBudget=..., arena=0x7ff2e4299000) at js/src/jsgc.cpp:2023
#8  js::gc::ArenaList::relocateArenas (this=this@entry=0x7ff2e40e4238, toRelocate=0x0, toRelocate@entry=0x7ff2e42cb000, relocated=0x7ff2e42cb000, sliceBudget=..., stats=...) at js/src/jsgc.cpp:2063
#9  0x00000000009c93ae in js::gc::ArenaLists::relocateArenas (this=this@entry=0x7ff2e40e40b0, zone=zone@entry=0x7ff2e40e4000, relocatedListOut=@0x7ffe1a1569d8: 0x7ff2e4239000, reason=reason@entry=JS::gcreason::DEBUG_GC, sliceBudget=..., stats=...) at js/src/jsgc.cpp:2110
#10 0x00000000009c954b in js::gc::GCRuntime::relocateArenas (this=this@entry=0x7ff2e595e5f8, zone=zone@entry=0x7ff2e40e4000, reason=reason@entry=JS::gcreason::DEBUG_GC, relocatedListOut=@0x7ffe1a1569d8: 0x7ff2e4239000, sliceBudget=...) at js/src/jsgc.cpp:2147
#11 0x00000000009c98e7 in js::gc::GCRuntime::compactPhase (this=this@entry=0x7ff2e595e5f8, reason=reason@entry=JS::gcreason::DEBUG_GC, sliceBudget=..., lock=...) at js/src/jsgc.cpp:5727
#12 0x00000000009d7939 in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ff2e595e5f8, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC, lock=...) at js/src/jsgc.cpp:6228
#13 0x00000000009d8b94 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ff2e595e5f8, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6465
#14 0x00000000009d9488 in js::gc::GCRuntime::collect (this=this@entry=0x7ff2e595e5f8, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6614
#15 0x00000000009dac1c in js::gc::GCRuntime::runDebugGC (this=this@entry=0x7ff2e595e5f8) at js/src/jsgc.cpp:7151
#16 0x0000000000d28ac5 in js::gc::GCRuntime::gcIfNeededAtAllocation (this=0x7ff2e595e5f8, cx=cx@entry=0x7ff2e5924000) at js/src/gc/Allocator.cpp:230
#17 0x0000000000d36918 in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=<optimized out>, cx=0x7ff2e5924000, kind=js::gc::AllocKind::OBJECT_LIMIT) at js/src/gc/Allocator.cpp:191
#18 0x0000000000d3a092 in js::Allocate<JSScript, (js::AllowGC)1> (cx=cx@entry=0x7ff2e5924000) at js/src/gc/Allocator.cpp:142
#19 0x0000000000a2c2d4 in JSScript::Create (cx=0x7ff2e5924000, options=..., sourceObject=..., sourceObject@entry=..., bufStart=bufStart@entry=0, bufEnd=246, toStringStart=0, toStringEnd=246) at js/src/jsscript.cpp:2627
#20 0x0000000000a8163e in BytecodeCompiler::createScript (this=0x7ffe1a157340, toStringStart=<optimized out>, toStringEnd=<optimized out>) at js/src/frontend/BytecodeCompiler.cpp:272
#21 0x0000000000aadc7d in BytecodeCompiler::createScript (this=0x7ffe1a157340) at js/src/frontend/BytecodeCompiler.cpp:266
#22 BytecodeCompiler::compileScript (this=this@entry=0x7ffe1a157340, environment=..., environment@entry=..., sc=sc@entry=0x7ffe1a1572e0) at js/src/frontend/BytecodeCompiler.cpp:336
#23 0x0000000000aae46a in BytecodeCompiler::compileEvalScript (enclosingScope=..., environment=..., this=0x7ffe1a157340) at js/src/frontend/BytecodeCompiler.cpp:402
#24 js::frontend::CompileEvalScript (cx=cx@entry=0x7ff2e5924000, alloc=..., environment=environment@entry=..., enclosingScope=..., options=..., srcBuf=..., sourceObjectOut=0x0) at js/src/frontend/BytecodeCompiler.cpp:601
#25 0x000000000056bf90 in EvalKernel (cx=cx@entry=0x7ff2e5924000, v=..., v@entry=..., evalType=evalType@entry=DIRECT_EVAL, caller=..., env=env@entry=..., pc=<optimized out>, vp=...) at js/src/builtin/Eval.cpp:318
#26 0x000000000056c75e in js::DirectEval (cx=cx@entry=0x7ff2e5924000, v=..., vp=vp@entry=...) at js/src/builtin/Eval.cpp:438
#27 0x000000000060c7f0 in js::jit::DoCallFallback (cx=0x7ff2e5924000, frame=0x7ffe1a158b38, stub_=<optimized out>, argc=<optimized out>, vp=0x7ffe1a158ae8, res=...) at js/src/jit/BaselineIC.cpp:2437
#28 0x00000e1b84031d77 in ?? ()
rax	0x0	0
rbx	0x7ff2e4299100	140681186742528
rcx	0x7ff2e5c5aa2d	140681213749805
rdx	0x0	0
rsi	0x7ff2e5f29770	140681216694128
rdi	0x7ff2e5f28540	140681216689472
rbp	0x7ffe1a1565d0	140729336030672
rsp	0x7ffe1a1565d0	140729336030672
r8	0x7ff2e5f29770	140681216694128
r9	0x7ff2e701a740	140681234458432
r10	0x58	88
r11	0x7ff2e5bd1750	140681213187920
r12	0x7ff2e42ef280	140681187095168
r13	0x7ff2e4299100	140681186742528
r14	0x60	96
r15	0x7ff2e42ef280	140681187095168
rip	0x67ffc0 <js::TypedArrayObject::length() const+112>
=> 0x67ffc0 <js::TypedArrayObject::length() const+112>:	movl   $0x0,0x0
   0x67ffcb <js::TypedArrayObject::length() const+123>:	ud2    


Marking s-s because the test uses gczeal and TypedArrays plus the assertion shows a type problem, so this is very likely a security issue. The testcase is intermittent but should reproduce in less than 10 runs. If you could figure out how to make this testcase deterministic that would be good for future testing.

Comment 1

[Fuzzing Team]

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

Comment 2

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/0ded74baeaf2
user:        Lee Salzman
date:        Tue May 09 22:30:58 2017 -0400
summary:     Bug 1340627 - part 1 - update Skia source to m59. r=mchang

This iteration took 273.524 seconds to run.

Comment 3

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

Comment 2 is probably invalid due to the intermittent testcase. Since this involves TypedArrays, setting needinfo? from Steve as a start.

Comment 4

[Andrew McCreight [:mccr8]]

It looks like compacting GC is involved, so maybe Jon could take a look.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1369994-typed-array-init

The condition in the assertion doesn't always hold because we can fail initialisation of template objects part way through.  The patch shuffles around the initialisation code so that doesn't happen.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8878468 [details] [diff] [review]
bug1369994-typed-array-init

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

Do you think this is s-s? If yes we shouldn't add the test.

Comment 7

[Jon Coppeard (:jonco)]

I can't immediately see how this could create a vulnerability, but leaving s-s since typed arrays are sufficiently complicated that there could be one.  I'll remove the test.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug1369994-typed-array-init v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Very difficult, requiring arranging an OOM to happen during JIT compilation.

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? Everything back to FF 50.

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

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

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

Comment 9

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

sec-approval+ for checkin on June 26 or after. We'll want Beta and ESR52 patches nominated and approved to go in after that.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7414bbec32d3e075551485eb83580021aea70d8

This grafts cleanly to Beta and ESR52, so please nominate it when you get a chance. Sorry for adding the wrong reviewer to the commit message too, will fix on uplift :(

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/725d1be7b81d for https://treeherder.mozilla.org/logviewer.html#?job_id=109948501&repo=mozilla-inbound

Comment 12

[Jon Coppeard (:jonco)]

Fixed and re-pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d516c35eabf14738e03728f9be602b732e63078c

Comment 13

[Jon Coppeard (:jonco)]

Attachment: bug1369994-typed-array-init v3

Updated patch.

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 16

[Jon Coppeard (:jonco)]

Comment on attachment 8881465 [details] [diff] [review]
bug1369994-typed-array-init v3

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1248163.
[User impact if declined]: Possible crash / security vulnerability.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a simple change to initialise a template object before a fallible operation to avoid exposing it to GC before it's been initialised. 
[String changes made/needed]: None.

Comment 17

[Jon Coppeard (:jonco)]

Comment on attachment 8881465 [details] [diff] [review]
bug1369994-typed-array-init v3

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-high bug.
Fix Landed on Version: 56.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8881465 [details] [diff] [review]
bug1369994-typed-array-init v3

sec-high fix for beta55

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0e5aa29c186d

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/267b649087ff

Comment 21

[Andrei Vaida [:avaida]]

(In reply to Jon Coppeard (:jonco) from comment #16)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Jon Coppeard's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 22

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx55