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

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks 1 bug, 6 keywords)

Trunk
mozilla53
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 unaffected, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

(Whiteboard: [jsbugmon:][post-critsmash-triage])

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

3 years ago
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.
Reporter

Comment 1

3 years ago
Missing testcase

Updated

3 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]

Comment 3

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

Updated

3 years ago
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]

Comment 4

3 years ago
JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment
Any chance you can take a look at this, Jon?
Flags: needinfo?(jcoppeard)
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
Tracking 53+ for this sec critical issue.
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?
Flags: needinfo?(jcoppeard) → needinfo?(nicolas.b.pierron)
Depends on: 1319888
(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.
Flags: needinfo?(nicolas.b.pierron)
Posted patch patch1315856 (obsolete) — Splinter Review
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?
Flags: needinfo?(jdemooij)
Assignee

Comment 11

3 years ago
(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.
Assignee

Comment 12

3 years ago
[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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Component: JavaScript Engine → JavaScript Engine: JIT
Assignee

Comment 13

3 years ago
Posted patch PatchSplinter Review
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.
Flags: needinfo?(jdemooij)
Attachment #8815267 - Flags: review?(jcoppeard)
Assignee

Updated

3 years ago
Attachment #8815234 - Attachment is obsolete: true
Comment on attachment 8815267 [details] [diff] [review]
Patch

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

Yes that is confusing!
Attachment #8815267 - Flags: review?(jcoppeard) → review+
Assignee

Comment 15

3 years ago
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.
Assignee

Comment 16

3 years ago
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.
Attachment #8815267 - Flags: sec-approval?
Track 51+ as sec-critical.
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.
Attachment #8815267 - Flags: sec-approval? → sec-approval+
Priority: -- → P1
Assignee

Comment 20

3 years ago
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.
Attachment #8815267 - Flags: approval-mozilla-beta?
Attachment #8815267 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c3d4bcee4905
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8815267 [details] [diff] [review]
Patch

Fix a sec-critical. Beta51+ and Aurora52+. Should be in 51 beta 7.
Attachment #8815267 - Flags: approval-mozilla-beta?
Attachment #8815267 - Flags: approval-mozilla-beta+
Attachment #8815267 - Flags: approval-mozilla-aurora?
Attachment #8815267 - Flags: approval-mozilla-aurora+
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [jsbugmon:] → [jsbugmon:][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.