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)
Tracking
()
VERIFIED
FIXED
mozilla61
People
(Reporter: decoder, Assigned: jandem)
Details
(5 keywords, Whiteboard: [jsbugmon:update][adv-main60+])
Crash Data
Attachments
(2 files)
15.66 KB,
patch
|
tcampbell
:
review+
jandem
:
feedback+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
936 bytes,
patch
|
tcampbell
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
Very interesting! This might be our mystery TI topcrash. Will debug now.
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 2•6 years ago
|
||
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.
Assignee | ||
Comment 3•6 years ago
|
||
I also want to add an AutoNoTypeInferenceSweeping class (it should contain an AutoCheckCannotGC + do some extra checks).
Updated•6 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 4•6 years ago
|
||
JSBugMon: Bisection requested, failed due to error (try manually).
Comment 5•6 years ago
|
||
I assume that this is a latent issue exposed by the poisoning patch. Any idea how far back it goes?
status-firefox59:
--- → ?
status-firefox60:
--- → ?
status-firefox-esr52:
--- → ?
tracking-firefox61:
--- → +
tracking-firefox-esr52:
--- → ?
Reporter | ||
Updated•6 years ago
|
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
Reporter | ||
Comment 6•6 years ago
|
||
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.
Updated•6 years ago
|
Assignee: nobody → jdemooij
tracking-firefox60:
--- → +
Assignee | ||
Comment 7•6 years ago
|
||
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)
Updated•6 years ago
|
Priority: -- → P1
Assignee | ||
Comment 8•6 years ago
|
||
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 9•6 years ago
|
||
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 10•6 years ago
|
||
Comment on attachment 8965289 [details] [diff] [review] Patch Review of attachment 8965289 [details] [diff] [review]: ----------------------------------------------------------------- (Reproducing a bugzilla issue)
Attachment #8965289 -
Flags: review+
Updated•6 years ago
|
Attachment #8965289 -
Flags: review+
Assignee | ||
Comment 11•6 years ago
|
||
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 12•6 years ago
|
||
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+
Assignee | ||
Comment 13•6 years ago
|
||
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.
Assignee | ||
Comment 14•6 years ago
|
||
I'll wait a few days to make sure this doesn't blow up on Nightly.
Flags: needinfo?(jdemooij)
Comment 15•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a466495618d0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 16•6 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Status: RESOLVED → VERIFIED
Crash Signature: [@ js::ReportMagicWordFailure]
[@ js::ConstraintTypeSet::addType] → [@ js::ReportMagicWordFailure]
[@ js::ConstraintTypeSet::addType]
Updated•6 years ago
|
Group: javascript-core-security → core-security-release
Assignee | ||
Comment 17•6 years ago
|
||
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]
Assignee | ||
Comment 18•6 years ago
|
||
Flags: needinfo?(jdemooij)
Attachment #8970120 -
Flags: review?(tcampbell)
Updated•6 years ago
|
Updated•6 years ago
|
Attachment #8970120 -
Flags: review?(tcampbell) → review+
Assignee | ||
Comment 19•6 years ago
|
||
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 20•6 years ago
|
||
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+
Comment 21•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/faa0174ca76e
Updated•6 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main60+]
Updated•6 years ago
|
Crash Signature: [@ js::ReportMagicWordFailure]
[@ js::ConstraintTypeSet::addType] → [@ js::ReportMagicWordFailure]
[@ js::ConstraintTypeSet::addType]
Comment 22•6 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx60
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•