1315856 - Assertion failure: (ptrBits & 0x7) == 0, at dist/include/js/Value.h:671

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 908557c762f7 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --disable-oom-functions --disable-oom-functions --baseline-eager --ion-eager --ion-check-range-analysis):

See attachment.


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x00000000004645e8 in JS::Value::toObjectOrNull (this=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:671
#0  0x00000000004645e8 in JS::Value::toObjectOrNull (this=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:671
#1  JS::Value::toObject (this=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:661
#2  0x0000000000d33a30 in js::DispatchTyped<DoMarkingFunctor<JS::Value>, js::GCMarker*&> (f=..., val=...) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:1483
#3  0x0000000000d33ae6 in DoMarking<JS::Value> (thing=..., gcmarker=0x7ffff6961d58) at js/src/gc/Marking.cpp:812
#4  DispatchToTracer<JS::Value> (trc=<optimized out>, thingp=0x7fffee336848, name=<optimized out>) at js/src/gc/Marking.cpp:661
#5  0x00007ffff7e3beff in ?? ()
#6  0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff6961d58	140737330421080
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7ffffff388b0	140737487538352
rsp	0x7ffffff388b0	140737487538352
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x0	0
r13	0x7fffffff27f0	140737488300016
r14	0x1	1
r15	0x0	0
rip	0x4645e8 <JS::Value::toObject() const+168>
=> 0x4645e8 <JS::Value::toObject() const+168>:	movl   $0x0,0x0
   0x4645f3 <JS::Value::toObject() const+179>:	ud2    


The attached testcase is unreduced because reducing it turns it highly intermittent. It would be very beneficial for fuzzing to know, why this happens and what we can do about it. Also marking s-s and sec-critical because this is a known-critical assertion often indicating memory corruption of some sort.

Comment 1

[Christian Holler (:decoder)]

Missing testcase

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase for comment 1

Comment 3

[Fuzzing Team]

JSBugMon: Cannot process bug: Error: Failed to isolate test from comment

Comment 4

[Fuzzing Team]

JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Any chance you can take a look at this, Jon?

Comment 6

[Jon Coppeard (:jonco) (PTO until July 30th)]

Sure.  Looks like we're trying to mark fresh nursery memory from an Ion pre-barrier:

Thread 1 received signal SIGSEGV, Segmentation fault.
0x000000000045cbba in JS::Value::toObjectOrNull (this=0x7f3efd2004c0) at /home/jon/clone/dev/js/src/test2-build/dist/include/js/Value.h:671
671	        MOZ_ASSERT((ptrBits & 0x7) == 0);
(rr) bt
#0  0x000000000045cbba in JS::Value::toObjectOrNull (this=0x7f3efd2004c0) at /home/jon/clone/dev/js/src/test2-build/dist/include/js/Value.h:671
#1  0x000000000045cb06 in JS::Value::toObject (this=0x7f3efd2004c0) at /home/jon/clone/dev/js/src/test2-build/dist/include/js/Value.h:661
#2  0x00000000010d105f in js::DispatchTyped<DoMarkingFunctor<JS::Value>, js::GCMarker*&> (f=..., val=...) at /home/jon/clone/dev/js/src/test2-build/dist/include/js/Value.h:1483
#3  0x00000000010cd9d9 in DoMarking<JS::Value> (gcmarker=0x7f3f01847d58, thing=...) at /home/jon/clone/dev/js/src/gc/Marking.cpp:812
#4  0x00000000010c61df in DispatchToTracer<JS::Value> (trc=0x7f3f01847d58, thingp=0x7f3efd2004c0, name=0x14ded41 "write barrier") at /home/jon/clone/dev/js/src/gc/Marking.cpp:661
#5  0x00000000010b181a in js::TraceManuallyBarrieredEdge<JS::Value> (trc=0x7f3f01847d58, thingp=0x7f3efd2004c0, name=0x14ded41 "write barrier") at /home/jon/clone/dev/js/src/gc/Marking.cpp:452
#6  0x000000000093dcbc in js::jit::MarkValueFromIon (rt=0x7f3f01845208, vp=0x7f3efd2004c0) at /home/jon/clone/dev/js/src/jit/VMFunctions.cpp:1280

(rr) p/x ptrBits
$2 = 0x2f2f2f2f2f2f

Comment 7

[Marcia Knous [:marcia]]

Tracking 53+ for this sec critical issue.

Comment 8

[Jon Coppeard (:jonco) (PTO until July 30th)]

When I run the testcase with IONFLAGS=all I get an assertion failure during SpewResumePoint:

Current resume point 0x7ffff2f589f8 details:
    frame count: 3
    taken at block 37 entry
Assertion failure: containsPC(pc), at /home/jon/clone/dev/js/src/jsscript.h:1089

Thread 8 "JS Helper" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff3e22700 (LWP 13232)]
0x0000000000461443 in JSScript::pcToOffset (this=0x7ffff3581670, pc=0x7ffff310fc8b "\346T") at /home/jon/clone/dev/js/src/jsscript.h:1089
1089	        MOZ_ASSERT(containsPC(pc));
Python Exception <class 'gdb.error'> No symbol "AsmJSFaultHandler" in current context.: 

(gdb) bt 10
#0  0x0000000000461443 in JSScript::pcToOffset(unsigned char const*) const (this=0x7ffff3581670, pc=0x7ffff310fc8b "\346T") at /home/jon/clone/dev/js/src/jsscript.h:1089
#1  0x0000000000821aef in SpewResumePoint(js::jit::MBasicBlock*, js::jit::MInstruction*, js::jit::MResumePoint*) (block=0x7ffff2f58858, ins=0x0, resumePoint=0x7ffff2f589f8)
    at /home/jon/clone/dev/js/src/jit/Lowering.cpp:4720
#2  0x0000000000822084 in js::jit::LIRGenerator::updateResumeState(js::jit::MBasicBlock*) (this=0x7ffff3e20cc0, block=0x7ffff2f58858)
    at /home/jon/clone/dev/js/src/jit/Lowering.cpp:4802
#3  0x00000000008220d4 in js::jit::LIRGenerator::visitBlock(js::jit::MBasicBlock*) (this=0x7ffff3e20cc0, block=0x7ffff2f58858) at /home/jon/clone/dev/js/src/jit/Lowering.cpp:4809
#4  0x0000000000822789 in js::jit::LIRGenerator::generate() (this=0x7ffff3e20cc0) at /home/jon/clone/dev/js/src/jit/Lowering.cpp:4885
#5  0x000000000070eee4 in js::jit::GenerateLIR(js::jit::MIRGenerator*) (mir=0x7ffff2f121c8) at /home/jon/clone/dev/js/src/jit/Ion.cpp:1931
#6  0x000000000070f3d2 in js::jit::CompileBackEnd(js::jit::MIRGenerator*) (mir=0x7ffff2f121c8) at /home/jon/clone/dev/js/src/jit/Ion.cpp:2026

(gdb) p pc
$2 = (const jsbytecode *) 0x7ffff310fc8b "\346T"
(gdb) p code()
$3 = (jsbytecode *) 0x7ffff6936da8 "\217"
(gdb) p codeEnd()
$4 = (jsbytecode *) 0x7ffff6936df6 "\220\210\bˈ\016Ȑ\210\bˈ\016Ȑ\210\bΈ\001\f\226\210\b\221\220\232\200"

(gdb) up
#1  0x0000000000821aef in SpewResumePoint (block=0x7ffff2f58858, ins=0x0, resumePoint=0x7ffff2f589f8) at /home/jon/clone/dev/js/src/jit/Lowering.cpp:4720
4720	            int(resumePoint->block()->info().script()->pcToOffset(resumePoint->pc())));

(gdb) list
4715	    out.printf("\n");
4716	
4717	    out.printf("    pc: %p (script: %p, offset: %d)\n",
4718	            (void*)resumePoint->pc(),
4719	            (void*)resumePoint->block()->info().script(),
4720	            int(resumePoint->block()->info().script()->pcToOffset(resumePoint->pc())));
4721	
4722	    for (size_t i = 0, e = resumePoint->numOperands(); i < e; i++) {
4723	        MDefinition* in = resumePoint->getOperand(i);
4724	        out.printf("    slot%u: ", (unsigned)i);

Nicholas, do you know if this is serious / possibly related?

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Jon Coppeard (:jonco) from comment #8)
> When I run the testcase with IONFLAGS=all I get an assertion failure during
> SpewResumePoint:
> 
> Current resume point 0x7ffff2f589f8 details:
>     frame count: 3
>     taken at block 37 entry
> Assertion failure: containsPC(pc), at
> /home/jon/clone/dev/js/src/jsscript.h:1089
> 
> Nicholas, do you know if this is serious / possibly related?

This indeed looks serious.
Still, this is not related to the initial issue.

I made a fix, I will attach it in Bug 1319888.

Sorry for the delay.

Comment 10

[Jon Coppeard (:jonco) (PTO until July 30th)]

Attachment: patch1315856

The assertion is happening because we're triggering a write barrier on uninitialised nursery memory and trying to mark the contents.

We create a call object with IonBuilder::createCallObject which initialises slots corresponding to positional formal parameters.  Then we do IonBuilder::jsop_setaliasedvar for an uninitialised slot which uses MStoreSlot::NewBarriered to do a barriered write.

The attached patch initialises the remaining slots and makes the problem go away.  I've no idea whether it's the right approach though.

Jan, do you know what's supposed to happen here?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #10)
> Jan, do you know what's supposed to happen here?

We should have initialized these slots when we allocated the object. Looking into it.

Comment 12

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]:

Bug in fillSlotsWithConstantValue, it doesn't handle the start != 0 case correctly (it always starts at the first slot). Seems like a pretty old regression from bug 969012. I'm surprised we didn't run into this sooner.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Patch

This is the most minimal fix. 

What makes this code confusing is that we make the following two calls here:

fillSlotsWithUninitialized: [0, startOfUndefined - nfixed)
fillSlotsWithUndefined:     [startOfUndefined - nfixed, ndynamic)

This suggests we initialize all slots from 0 to ndynamic. However, we use the start/end arguments only as a length and ignore the start value. I think it would be clearer if these functions took a length argument instead of start/end, but let's land the safe fix first.

Comment 14

[Jon Coppeard (:jonco) (PTO until July 30th)]

Comment on attachment 8815267 [details] [diff] [review]
Patch

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

Yes that is confusing!

Comment 15

[Jan de Mooij [:jandem]]

Actually, this is a regression from bug 1263355 (Firefox 51). Before that, we did the following:

if (ndynamic) {
    ...
    // Initially fill all dynamic slots with undefined.
    fillSlotsWithUndefined(Address(obj, 0), temp, 0, ndynamic);

    ...code to overwrite some of these with the uninitialized value...
}

So we always initialized all dynamic slots to undefined.

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8815267 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not trivial but possible for someone determined and with some knowledge of VMs.

> 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?
51+ (so aurora/beta).

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

> 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, the patch changes just 2 lines of code and is pretty straight-forward.

Comment 17

[Gerry Chang [:gchang]]

Track 51+ as sec-critical.

Comment 18

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

Comment on attachment 8815267 [details] [diff] [review]
Patch

Sec-approval+ for trunk. Please nominate patches for affected branches as well so we can avoid shipping this issue.

Comment 19

[Jan de Mooij [:jandem]]

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

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8815267 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1263355.
[User impact if declined]: Security issues, crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[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?]: Fix is well understood. Fixes the test case the fuzzers found.
[String changes made/needed]: None.

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8815267 [details] [diff] [review]
Patch

Fix a sec-critical. Beta51+ and Aurora52+. Should be in 51 beta 7.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4ac86351a69e

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eaaf39842d5c