Closed
Bug 1328834
Opened 7 years ago
Closed 7 years ago
Assertion failure: hasScript(), at js/src/jsfun.h:459 with GC
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
People
(Reporter: decoder, Assigned: jandem)
Details
(4 keywords, Whiteboard: [jsbugmon:update][post-critsmash-triage][adv-main51+][adv-esr45.7+])
Attachments
(1 file, 1 obsolete file)
1.03 KB,
patch
|
jandem
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 31ffcb82ced8 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --ion-offthread-compile=off): evaluate(` function TestCase() {} `); gczeal(15,10); function f(x) {} for (var i=0; i<100; i++) { f(new TestCase()); } relazifyFunctions(); relazifyFunctions(); for (var i=0; i<10; i-- ) {} Backtrace: received signal SIGSEGV, Segmentation fault. JSFunction::hasUncompiledScript (this=this@entry=0x7ffff36ab700) at js/src/jsfun.h:459 #0 JSFunction::hasUncompiledScript (this=this@entry=0x7ffff36ab700) at js/src/jsfun.h:459 #1 JSFunction::nonLazyScript (this=this@entry=0x7ffff36ab700) at js/src/jsfun.h:464 #2 0x000000000061aec8 in js::jit::IonBuilder::createThisScriptedSingleton (this=this@entry=0x7ffff33aa278, target=0x7ffff36ab700, callee=callee@entry=0x7ffff33ae8d8) at js/src/jit/IonBuilder.cpp:4739 #3 0x000000000061e84b in js::jit::IonBuilder::createThis (this=this@entry=0x7ffff33aa278, target=target@entry=0x7ffff36ab700, callee=0x7ffff33ae8d8, newTarget=0x7ffff33ae8d8) at js/src/jit/IonBuilder.cpp:4848 #4 0x000000000062cdc7 in js::jit::IonBuilder::makeCallHelper (this=this@entry=0x7ffff33aa278, target=0x7ffff36ab700, callInfo=...) at js/src/jit/IonBuilder.cpp:5306 #5 0x000000000062cf08 in js::jit::IonBuilder::makeCall (this=this@entry=0x7ffff33aa278, target=<optimized out>, callInfo=...) at js/src/jit/IonBuilder.cpp:5354 #6 0x000000000064a6ec in js::jit::IonBuilder::jsop_call (this=this@entry=0x7ffff33aa278, argc=<optimized out>, constructing=<optimized out>) at js/src/jit/IonBuilder.cpp:5168 #7 0x000000000064ef47 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7ffff33aa278, op=op@entry=JSOP_NEW) at js/src/jit/IonBuilder.cpp:1995 #8 0x00000000006504f8 in js::jit::IonBuilder::visitBlock (this=this@entry=0x7ffff33aa278, cfgblock=cfgblock@entry=0x7ffff699a170, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1512 #9 0x000000000064623b in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff33aa278) at js/src/jit/IonBuilder.cpp:1433 #10 0x0000000000647149 in js::jit::IonBuilder::build (this=this@entry=0x7ffff33aa278) at js/src/jit/IonBuilder.cpp:842 #11 0x000000000065c997 in js::jit::IonCompile (cx=cx@entry=0x7ffff695f000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x7fffffffc658, osrPc=<optimized out>, recompile=<optimized out>, optimizationLevel=optimizationLevel@entry=js::jit::OptimizationLevel::Normal) at js/src/jit/Ion.cpp:2280 #12 0x000000000065d4d2 in js::jit::Compile (cx=cx@entry=0x7ffff695f000, script=script@entry=..., osrFrame=osrFrame@entry=0x7fffffffc658, osrPc=osrPc@entry=0x7ffff33f4e97 "ず", forceRecompile=<optimized out>) at js/src/jit/Ion.cpp:2533 #13 0x000000000065df50 in BaselineCanEnterAtBranch (pc=0x7ffff33f4e97 "ず", osrFrame=0x7fffffffc658, script=..., cx=<optimized out>) at js/src/jit/Ion.cpp:2724 #14 js::jit::IonCompileScriptForBaseline (cx=0x7ffff695f000, frame=frame@entry=0x7fffffffc658, pc=pc@entry=0x7ffff33f4e97 "ず") at js/src/jit/Ion.cpp:2782 #15 0x0000000000e99262 in js::jit::DoWarmUpCounterFallbackOSR (cx=0x7ffff695f000, frame=0x7fffffffc658, stub=0x7ffff33bd890, infoPtr=0x7fffffffc630) at js/src/jit/BaselineIC.cpp:144 #16 0x00007ffff7e43344 in ?? () [...] #27 0x0000000000000000 in ?? () rax 0x2052520 33891616 rbx 0x7ffff33aa278 140737274094200 rcx 0x11a5a60 18504288 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7fffffffbbf0 140737488337904 rsp 0x7fffffffbbf0 140737488337904 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff7fe4740 140737354024768 r10 0x58 88 r11 0x7ffff6b9f750 140737332770640 r12 0x7ffff3698080 140737277165696 r13 0x7ffff36ab700 140737277245184 r14 0x7ffff368c100 140737277116672 r15 0x7ffff36ab700 140737277245184 rip 0x463451 <JSFunction::nonLazyScript() const+65> => 0x463451 <JSFunction::nonLazyScript() const+65>: movl $0x0,0x0 0x46345c <JSFunction::nonLazyScript() const+76>: ud2 This assertion is known to be problematic for security and there's GC involved, so marking s-s.
Updated•7 years ago
|
Priority: -- → P1
Updated•7 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 2•7 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/853a0dfc4d5f user: Jan de Mooij date: Thu Dec 08 14:26:36 2016 -1000 summary: Bug 1318627 - Remove Zone::active and related infrastructure. r=jonco, f=bhackett This iteration took 266.176 seconds to run.
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Fuzzing Team from comment #2) > summary: Bug 1318627 - Remove Zone::active and related infrastructure. > r=jonco, f=bhackett That change probably exposed this bug, because we can now discard type information in more cases, but the underlying bug is likely older.
Assignee | ||
Comment 4•7 years ago
|
||
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8824489 -
Flags: review?(hv1989)
Assignee | ||
Comment 5•7 years ago
|
||
This may be more difficult to trigger without bug 1318627, but the patch is trivial so we should just land this everywhere.
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox-esr45:
--- → affected
tracking-firefox51:
--- → ?
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
Keywords: sec-high
Comment 6•7 years ago
|
||
Can you request sec-approval since the issue has a sec-high rating? Thanks.
Flags: needinfo?(jdemooij)
Updated•7 years ago
|
tracking-firefox-esr45:
--- → ?
Assignee | ||
Comment 7•7 years ago
|
||
Comment on attachment 8824489 [details] [diff] [review] Patch [Security approval request comment] > How easily could an exploit be constructed based on the patch? Hard to say. Probably not trivial but might cause some weird memory corruption. > 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? Should apply or be trivial to backport. > How likely is this patch to cause regressions; how much testing does it need? Very unlikely. It just adds two target->hasScript() checks.
Flags: needinfo?(jdemooij)
Attachment #8824489 -
Flags: sec-approval?
Comment 8•7 years ago
|
||
Comment on attachment 8824489 [details] [diff] [review] Patch Review of attachment 8824489 [details] [diff] [review]: ----------------------------------------------------------------- Should these new conditions not be the first things that get tested in those corresponding functions? Before I.e. getSingletonPrototype(target) since that will add constraints, which we would bailout on, even if we took the more global working code?
Attachment #8824489 -
Flags: review?(hv1989) → review+
Updated•7 years ago
|
Comment 9•7 years ago
|
||
Comment on attachment 8824489 [details] [diff] [review] Patch sec-approval+ for trunk. I see release management has marked it tracking + for branches and this is a higher rated issue so I'd suggest nominating branch patches as well.
Attachment #8824489 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 10•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a2e4e4e4f0e65c17f10658dc1f7ee67ff515b74 (In reply to Hannes Verschore [:h4writer] from comment #8) > Should these new conditions not be the first things that get tested in those > corresponding functions? Having a lazy function there is rare, but fair enough. I moved the checks to the top of the function. When I did that I noticed createThisScriptedBaseline already checked target->hasScript(), so that means we only need the one in createThisScriptedSingleton.
Assignee | ||
Comment 11•7 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: Old bug. [User impact if declined]: Potential crashes or security issues. [Is this code covered by automated tests?]: Yes although this edge case probably isn't. It does fix the testcase the fuzzers found. We can check it in later. [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 just adds an extra check for an edge case, 2-liner. [String changes made/needed]: None.
Attachment #8824489 -
Attachment is obsolete: true
Attachment #8825348 -
Flags: review+
Attachment #8825348 -
Flags: approval-mozilla-esr45?
Attachment #8825348 -
Flags: approval-mozilla-beta?
Attachment #8825348 -
Flags: approval-mozilla-aurora?
Comment 12•7 years ago
|
||
Comment on attachment 8825348 [details] [diff] [review] Patch fix security-sensitive assertion in js jit in all branches
Attachment #8825348 -
Flags: approval-mozilla-esr45?
Attachment #8825348 -
Flags: approval-mozilla-esr45+
Attachment #8825348 -
Flags: approval-mozilla-beta?
Attachment #8825348 -
Flags: approval-mozilla-beta+
Attachment #8825348 -
Flags: approval-mozilla-aurora?
Attachment #8825348 -
Flags: approval-mozilla-aurora+
Comment 13•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/142e00af3966 https://hg.mozilla.org/releases/mozilla-beta/rev/d60102a285f0 https://hg.mozilla.org/releases/mozilla-esr45/rev/0521b0e4707c
https://hg.mozilla.org/mozilla-central/rev/5a2e4e4e4f0e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Updated•7 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-critsmash-triage]
Updated•7 years ago
|
Whiteboard: [jsbugmon:update][post-critsmash-triage] → [jsbugmon:update][post-critsmash-triage][adv-main51+][adv-esr45.7+]
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Comment 15•7 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•