Assertion failure: hasScript(), at js/src/jsfun.h:459 with GC

VERIFIED FIXED in Firefox -esr45



2 years ago
2 years ago


(Reporter: decoder, Assigned: jandem)


(Blocks 1 bug, 5 keywords)

Dependency tree / graph

Firefox Tracking Flags

(firefox-esr4551+ fixed, firefox50 wontfix, firefox51+ fixed, firefox52+ fixed, firefox53+ verified)


(Whiteboard: [jsbugmon:update][post-critsmash-triage][adv-main51+][adv-esr45.7+])


(1 attachment, 1 obsolete attachment)



2 years ago
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):

  function TestCase() {}
function f(x) {}
for (var i=0; i<100; i++) {
    f(new TestCase());
for (var i=0; i<10; i-- ) {}


 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.

Comment 1

2 years ago
Is jsbugmon not working?
Flags: needinfo?(jdemooij)
Priority: -- → P1


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

Comment 2

2 years ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
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.

Comment 3

2 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.

Comment 4

2 years ago
Posted patch Patch (obsolete) — Splinter Review
Assignee: nobody → jdemooij
Flags: needinfo?(jdemooij)
Attachment #8824489 - Flags: review?(hv1989)

Comment 5

2 years ago
This may be more difficult to trigger without bug 1318627, but the patch is trivial so we should just land this everywhere.
Can you request sec-approval since the issue has a sec-high rating? Thanks.
Flags: needinfo?(jdemooij)

Comment 7

2 years ago
Comment on attachment 8824489 [details] [diff] [review]

[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?

> Which older supported branches are affected by this flaw?

> 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 on attachment 8824489 [details] [diff] [review]

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+
Comment on attachment 8824489 [details] [diff] [review]

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+

Comment 10

2 years ago

(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.

Comment 11

2 years ago
Posted patch PatchSplinter Review
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 on attachment 8825348 [details] [diff] [review]

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+
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: javascript-core-security → core-security-release
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-critsmash-triage]
Whiteboard: [jsbugmon:update][post-critsmash-triage] → [jsbugmon:update][post-critsmash-triage][adv-main51+][adv-esr45.7+]


2 years ago

Comment 15

2 years ago
JSBugMon: This bug has been automatically verified fixed.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.