Closed
Bug 1437450
Opened 4 years ago
Closed 4 years ago
Assertion failure: startOfUninitialized <= nfixed, at js/src/jit/MacroAssembler.cpp:1163
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla60
People
(Reporter: decoder, Assigned: jandem)
Details
(5 keywords, Whiteboard: [jsbugmon:update][adv-main59+][adv-esr52.7+])
Attachments
(1 file)
986 bytes,
patch
|
nbp
:
review+
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-release+
RyanVM
:
approval-mozilla-esr52+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 2b7d42d527af (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --disable-oom-functions --ion-eager): function testBug507425() { var r = /x/; for (var i = 0; i < 3; i++) r.lastIndex = 0; var s = ';'; try { for (i = 0; i < 80; testBug507425(s)) s = s; } catch (exc) { r.DateTimeFormat = 0; } } testBug507425(); Backtrace: received signal SIGSEGV, Segmentation fault. #0 0x0000000000824aa5 in js::jit::MacroAssembler::initGCSlots (this=this@entry=0x7ffff5fbe048, obj=obj@entry=..., temp=..., temp@entry=..., templateObj=templateObj@entry=0x7ffff5899080, initContents=initContents@entry=true) at js/src/jit/MacroAssembler.cpp:1163 #1 0x0000000000824e77 in js::jit::MacroAssembler::initGCThing (this=0x7ffff5fbe048, obj=..., temp=..., templateObj=templateObj@entry=0x7ffff5899080, initContents=initContents@entry=false, convertDoubleElements=convertDoubleElements@entry=false) at js/src/jit/MacroAssembler.cpp:1257 #2 0x000000000082a12f in js::jit::MacroAssembler::createGCObject (this=<optimized out>, obj=..., obj@entry=..., temp=..., temp@entry=..., templateObj=templateObj@entry=0x7ffff5899080, initialHeap=initialHeap@entry=js::gc::DefaultHeap, fail=fail@entry=0x7ffff5fceee8, initContents=true, convertDoubleElements=false) at js/src/jit/MacroAssembler.cpp:921 #3 0x000000000068b435 in js::jit::CodeGenerator::visitRegExp (this=0x7ffff5fbe000, lir=0x7ffff5fb6fa8) at js/src/jit/CodeGenerator.cpp:1239 #4 0x00000000006cbcef in js::jit::CodeGenerator::generateBody (this=this@entry=0x7ffff5fbe000) at js/src/jit/CodeGenerator.cpp:5568 #5 0x00000000006cc815 in js::jit::CodeGenerator::generate (this=this@entry=0x7ffff5fbe000) at js/src/jit/CodeGenerator.cpp:9748 #6 0x00000000006f83a6 in js::jit::GenerateCode (mir=mir@entry=0x7ffff5fc4270, lir=0x7ffff5fb6230) at js/src/jit/Ion.cpp:1966 #7 0x0000000000751949 in js::jit::CompileBackEnd (mir=mir@entry=0x7ffff5fc4270) at js/src/jit/Ion.cpp:1988 #8 0x0000000000bb8db8 in js::HelperThread::handleIonWorkload (this=this@entry=0x7ffff5f28098, locked=...) at js/src/vm/HelperThreads.cpp:1838 [...] #14 0x00007ffff6c383dd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109 rax 0x0 0 rbx 0x1 1 rcx 0x7ffff6c282ad 140737333330605 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7ffff5cfcb90 140737317424016 rsp 0x7ffff5cfcb20 140737317423904 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff5cfe700 140737317431040 r10 0x58 88 r11 0x7ffff6b9e7a0 140737332766624 r12 0x3 3 r13 0x4 4 r14 0x7ffff5899080 140737312821376 r15 0xfff9800000000000 -1829587348619264 rip 0x824aa5 <js::jit::MacroAssembler::initGCSlots(js::jit::Register, js::jit::Register, js::NativeObject*, bool)+997> => 0x824aa5 <js::jit::MacroAssembler::initGCSlots(js::jit::Register, js::jit::Register, js::NativeObject*, bool)+997>: movl $0x0,0x0 0x824ab0 <js::jit::MacroAssembler::initGCSlots(js::jit::Register, js::jit::Register, js::NativeObject*, bool)+1008>: ud2 Marking s-s because violating this assertion looks bad.
Comment 1•4 years ago
|
||
I've looked at this only because it happened to come up while fuzzing nursery strings. The crashing stack is nice and short. The main thread is reporting over-recursion from within jit code. I haven't looked any further than that.
Updated•4 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 2•4 years ago
|
||
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/76b0524a77e4 user: Jan de Mooij date: Sun Jul 09 11:46:57 2017 +0200 summary: Bug 1115355 - Optimize RegExpObject allocation in Ion. r=evilpie This iteration took 266.287 seconds to run.
Assignee | ||
Comment 4•4 years ago
|
||
The underlying bug predates bug 1115355. The problem is that the MakeMRegExpHoistable pass looks at all uses of MRegExp (regexp literals) but if the script has a try-catch block it's possible we're missing uses in catch blocks (because Ion skips these). The easiest fix is to check hasTryBlock, similar to what we do in EliminateDeadResumePointOperands. This is pretty bad because it lets you modify the template RegExpObject; I think we should backport this patch to beta/ESR. FWIW, the following test fails with --ion-eager --no-threads. I'm pretty sure this failed before bug 1115355 landed. function f() { var arr = []; for (var i = 0; i < 50; i++) { try { var re = /foo/; if (i % 5 === 0) throw 1; } catch (e) { arr.push(re); } } for (var i = 0; i < arr.length; i++) { for (var j = 0; j < arr.length; j++) { assertEq(arr[i] === arr[j], i === j); } } } f();
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8950554 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 5•4 years ago
|
||
Let's treat this as sec-high at least.
status-firefox58:
--- → affected
status-firefox59:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox59:
--- → ?
tracking-firefox60:
--- → ?
tracking-firefox-esr52:
--- → ?
Keywords: sec-high
Comment 6•4 years ago
|
||
Comment on attachment 8950554 [details] [diff] [review] Patch Review of attachment 8950554 [details] [diff] [review]: ----------------------------------------------------------------- This patch fixes the try-catch issue, but I will note that IsRegExpHoistable does not handle the implictlyUsed / removedUses flags, which might cause similar issues with polymorphic callsites. One alternative solution would be to allocate a proper RegExp object when we bailout instead of re-using the template object after the bailout.
Attachment #8950554 -
Flags: review?(nicolas.b.pierron) → review+
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 7•4 years ago
|
||
Comment on attachment 8950554 [details] [diff] [review] Patch [Security approval request comment] > How easily could an exploit be constructed based on the patch? With some effort it's possible probably. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. (There's a comment, but the code change says it all anyway.) > 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? Should apply or be easy to backport. > How likely is this patch to cause regressions; how much testing does it need? Unlikely. Maybe a perf regression but I don't expect it.
Attachment #8950554 -
Flags: sec-approval?
Comment 8•4 years ago
|
||
Comment on attachment 8950554 [details] [diff] [review] Patch sec-approval+ for trunk. Please make and nominate Beta and ESR52 patches as well, to land after trunk.
Attachment #8950554 -
Flags: sec-approval? → sec-approval+
Updated•4 years ago
|
Component: JavaScript Engine → JavaScript Engine: JIT
Priority: -- → P1
This can still make the Beta 14 build tomorrow if you can land it and uplift to beta tonight. Nicolas, maybe you can help with that but I'll also follow up in email with jandem.
Flags: needinfo?(nicolas.b.pierron)
Assignee | ||
Comment 11•4 years ago
|
||
I wanted to land this yesterday but the tree was closed for most of the day. And now it's also closed :/
Flags: needinfo?(nicolas.b.pierron)
Assignee | ||
Comment 12•4 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/61b461277369e0cec89c79d8526a82c575818e94
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 13•4 years ago
|
||
Comment on attachment 8950554 [details] [diff] [review] Patch Approval Request Comment [Feature/Bug causing the regression]: Old bug. [User impact if declined]: Security issues, crashes. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: On inbound. [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 just disables an optimization in some cases. It might affect performance in some cases but I don't expect it to be serious. [String changes made/needed]: None.
Attachment #8950554 -
Flags: approval-mozilla-beta?
Comment 14•4 years ago
|
||
Comment on attachment 8950554 [details] [diff] [review] Patch m-b was merged to m-r today, so approving for m-r and the Fennec b15 relbranch. Jan, does this need an ESR52 approval request as well?
Flags: needinfo?(jdemooij)
Attachment #8950554 -
Flags: approval-mozilla-release+
Attachment #8950554 -
Flags: approval-mozilla-beta?
Attachment #8950554 -
Flags: approval-mozilla-beta+
Assignee | ||
Comment 15•4 years ago
|
||
Comment on attachment 8950554 [details] [diff] [review] Patch Yeah this patch is so small and safe that we should just take it on ESR52 IMO. [Approval Request Comment] User impact if declined: Security issues. Fix Landed on Version: 60, backport to 59. Risk to taking this patch (and alternatives if risky): Very low risk. String or UUID changes made by this patch: None.
Flags: needinfo?(jdemooij)
Attachment #8950554 -
Flags: approval-mozilla-esr52?
Comment 16•4 years ago
|
||
Comment on attachment 8950554 [details] [diff] [review] Patch Fixes a sec-high. Approved for ESR 52.7.0.
Attachment #8950554 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
![]() |
||
Comment 17•4 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/61b461277369
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•4 years ago
|
Status: RESOLVED → VERIFIED
Comment 18•4 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Comment 19•4 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/36f58000ec41 (FIREFOX_59b_RELBRANCH) https://hg.mozilla.org/releases/mozilla-release/rev/806696d49430
Comment 20•4 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/16b8073d5c30
Updated•4 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main59+][adv-esr52.7+]
Updated•4 years ago
|
Comment 21•4 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx59
Updated•4 years ago
|
Group: javascript-core-security → core-security-release
Updated•4 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•