Closed Bug 1447989 Opened 6 years ago Closed 6 years ago

Crash [@ js::ReportMagicWordFailure] or Crash [@ js::ConstraintTypeSet::addType] with GC

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 - unaffected
firefox59 --- wontfix
firefox60 + verified
firefox61 + verified

People

(Reporter: decoder, Assigned: jandem)

Details

(5 keywords, Whiteboard: [jsbugmon:update][adv-main60+])

Crash Data

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 7771df14ea18+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --ion-eager --ion-offthread-compile=off):

var set = () => summary >>> 0xffffffff;
gczeal(17, 1);
var result = false;
set(result[0]);


Backtrace:

received signal SIGSEGV, Segmentation fault.
MOZ_CrashPrintf (aLine=aLine@entry=2579, aFormat=aFormat@entry=0xe96238 "Got 0x%lx expected magic word 0x%lx") at mfbt/Assertions.cpp:63
#0  MOZ_CrashPrintf (aLine=aLine@entry=2579, aFormat=aFormat@entry=0xe96238 "Got 0x%lx expected magic word 0x%lx") at mfbt/Assertions.cpp:63
#1  0x00000000009eef1d in js::ReportMagicWordFailure (actual=<optimized out>, expected=expected@entry=11647069175327152089) at js/src/vm/TypeInference.cpp:2578
#2  0x0000000000a02a3f in js::TypeConstraint::checkMagic (this=0x7ffff49da0a0) at js/src/vm/TypeInference.h:586
#3  js::TypeConstraint::next (this=0x7ffff49da0a0) at js/src/vm/TypeInference.h:591
#4  js::ConstraintTypeSet::addType (this=0x7ffff5f55ca0, cx=0x7ffff5f15000, type=...) at js/src/vm/TypeInference.cpp:717
#5  0x0000000000a08a46 in js::TypeMonitorResult (cx=cx@entry=0x7ffff5f15000, script=<optimized out>, pc=<optimized out>, types=0x7ffff5f55ca0, type=...) at js/src/vm/TypeInference.cpp:3337
#6  0x00000000005c8f72 in js::TypeScript::Monitor (cx=cx@entry=0x7ffff5f15000, script=<optimized out>, pc=pc@entry=0x7ffff49f8140 "7\347\001", types=types@entry=0x7ffff5f55ca0, rval=...) at js/src/vm/TypeInference-inl.h:607
#7  0x00000000005b59ec in js::jit::DoGetElemFallback (cx=0x7ffff5f15000, frame=0x7fffffffcc88, stub_=0x7ffff49fa1c0, lhs=..., rhs=..., res=...) at js/src/jit/BaselineIC.cpp:615
#8  0x00002ffc609184e4 in ?? ()
#9  0x0000000000000000 in ?? ()
rax	0x1c37d20	29588768
rbx	0x7ffff5f55ca0	140737319885984
rcx	0x7fffffc2	2147483586
rdx	0x1c37ddd	29588957
rsi	0x0	0
rdi	0x1c37da0	29588896
rbp	0x7ffff5f15000	140737319620608
rsp	0x7fffffffc720	140737488340768
r8	0x0	0
r9	0x3d	61
r10	0x0	0
r11	0xfffffffffffffff1	-15
r12	0x7ffff49da0a0	140737297359008
r13	0x3	3
r14	0xa1a2b3b4c5c6d7d9	-6799674898382399527
r15	0x0	0
rip	0x42f7ac <MOZ_CrashPrintf(int, char const*, ...)+243>
=> 0x42f7ac <MOZ_CrashPrintf(int, char const*, ...)+243>:	movl   $0x0,0x0
   0x42f7b7 <MOZ_CrashPrintf(int, char const*, ...)+254>:	ud2



Marking s-s because this looks like a type inference problem involving GC, which is potentially dangerous.
Very interesting! This might be our mystery TI topcrash. Will debug now.
Flags: needinfo?(jdemooij)
So the poisoning added in bug 1446348 is what's making us crash here. We can hold a TypeConstraint*/ConstraintTypeSet* pointer live across a call that can trigger sweeping in some way, like here:

https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/js/src/vm/TypeInference.cpp#714-718

I think we should mark these pointers as GC pointers and mark the sweep functions as GC functions, and then use the static analysis.
I also want to add an AutoNoTypeInferenceSweeping class (it should contain an AutoCheckCannotGC + do some extra checks).
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, failed due to error (try manually).
I assume that this is a latent issue exposed by the poisoning patch. Any idea how far back it goes?
Crash Signature: [@ js::ReportMagicWordFailure] → [@ js::ReportMagicWordFailure] [@ js::ConstraintTypeSet::addType]
Summary: Crash [@ js::ReportMagicWordFailure] with GC → Crash [@ js::ReportMagicWordFailure] or Crash [@ js::ConstraintTypeSet::addType] with GC
This is an automated crash issue comment:

Summary: Crash [@ js::ConstraintTypeSet::addType]
Build version: mozilla-central revision 9cb650de48f9+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --without-intl-api --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --target=i686-pc-linux-gnu
Runtime options: --fuzzing-safe --ion-eager --ion-offthread-compile=off

Testcase:

gczeal(17, 1);
count = 0;
for (var {
        length: x,
        [x - 1 + count]: y = "psych"
    } in "foo") {
    count++;
}

Backtrace:

==15534==ERROR: AddressSanitizer: SEGV on unknown address 0x6f6f6f6f (pc 0x09f9b3d4 bp 0xffa7b1a8 sp 0xffa7b0c0 T0)
==15534==The signal is caused by a READ memory access.
    #0 0x9f9b3d3 in js::ConstraintTypeSet::addType(JSContext*, js::TypeSet::Type) js/src/vm/TypeInference.cpp:716:25
    #1 0x9fc36ab in js::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, js::StackTypeSet*, js::TypeSet::Type) js/src/vm/TypeInference.cpp:3337:12
    #2 0x88d5ba9 in js::TypeScript::Monitor(JSContext*, JSScript*, unsigned char*, js::StackTypeSet*, JS::Value const&) js/src/vm/TypeInference-inl.h:607:9
    #3 0x8834daa in js::jit::DoGetElemFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICGetElem_Fallback*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:615:9
    #4 0xeac159e2  (<unknown module>)


Just wanted to show this nice opt-crash with some poison pattern. Probably the same bug as the one in comment 0.
Attached patch PatchSplinter Review
The fix for this bug is a one-liner, but this patch also adds (release) asserts to help catch similar bugs. I also added some asserts for a few other potential issues I noticed.

decoder, this patch should apply to current m-c tip (rev 1b258f938525). It would be great if we could fuzz this for a few hours to make sure the new asserts don't reveal any obvious issues.
Attachment #8965289 - Flags: feedback?(choller)
Priority: -- → P1
Comment on attachment 8965289 [details] [diff] [review]
Patch

decoder said he didn't find anything.

On to the next station!
Flags: needinfo?(jdemooij)
Attachment #8965289 - Flags: review?(tcampbell)
Attachment #8965289 - Flags: feedback?(choller)
Attachment #8965289 - Flags: feedback+
Comment on attachment 8965289 [details] [diff] [review]
Patch

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

Making the lazy sweeping more explicit seem like a good trend.
Attachment #8965289 - Flags: review?(tcampbell) → review+
Comment on attachment 8965289 [details] [diff] [review]
Patch

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

(Reproducing a bugzilla issue)
Attachment #8965289 - Flags: review+
Attachment #8965289 - Flags: review+
Comment on attachment 8965289 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
I'm not sure. It's not trivial but might be 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?
All.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch adds some extra asserts. We could uplift a more minimal fix to beta and ESR52, should be very low risk.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.
Attachment #8965289 - Flags: sec-approval?
Comment on attachment 8965289 [details] [diff] [review]
Patch

sec-approval+ for trunk.
Is there any particular value in uplifting an assert focused patch to Beta and ESR52?
Attachment #8965289 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a466495618d0468b0fd43f9c10fed8ae8112b6d9

(In reply to Al Billings [:abillings] from comment #12)
> Is there any particular value in uplifting an assert focused patch to Beta
> and ESR52?

The patch also fixes the bug reported here. I think we should uplift the minimal fix to Beta and ESR52, without the assert infrastructure.
I'll wait a few days to make sure this doesn't blow up on Nightly.
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/a466495618d0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
JSBugMon: This bug has been automatically verified fixed.
Status: RESOLVED → VERIFIED
Crash Signature: [@ js::ReportMagicWordFailure] [@ js::ConstraintTypeSet::addType] → [@ js::ReportMagicWordFailure] [@ js::ConstraintTypeSet::addType]
Group: javascript-core-security → core-security-release
ESR52 is not affected, this is from bug 1363054.

I'll post a minimal fix for beta now.

I also have some ideas on how to redesign TI sweeping that would have made these bugs obvious.
Crash Signature: [@ js::ReportMagicWordFailure] [@ js::ConstraintTypeSet::addType] → [@ js::ReportMagicWordFailure] [@ js::ConstraintTypeSet::addType]
Attached patch Patch for betaSplinter Review
Flags: needinfo?(jdemooij)
Attachment #8970120 - Flags: review?(tcampbell)
Attachment #8970120 - Flags: review?(tcampbell) → review+
Comment on attachment 8970120 [details] [diff] [review]
Patch for beta

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1363054.
[User impact if declined]: Crashes, security issues.
[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?]: Very low risk.
[Why is the change risky/not risky?]: It's just a one-liner and has been on Nightly for a week at least.
[String changes made/needed]: None.
Attachment #8970120 - Flags: approval-mozilla-beta?
Comment on attachment 8970120 [details] [diff] [review]
Patch for beta

Fix for a sec-high crash, looks like this was verified in nightly.
OK to uplift for beta 16.
Attachment #8970120 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main60+]
Crash Signature: [@ js::ReportMagicWordFailure] [@ js::ConstraintTypeSet::addType] → [@ js::ReportMagicWordFailure] [@ js::ConstraintTypeSet::addType]
JSBugMon: This bug has been automatically verified fixed on Fx60
Group: core-security-release
Keywords: bugmon
You need to log in before you can comment on or make changes to this bug.