Closed Bug 1238003 Opened 4 years ago Closed 4 years ago

Assertion failure: ins->regexp()->type() == MIRType_Object, at c:/Users/mozilla/debug-build/mozilla-central/js/src/jit/Lowering.cpp:2164

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox43 --- unaffected
firefox44 --- unaffected
firefox45 --- unaffected
firefox46 --- fixed
firefox-esr38 --- unaffected
firefox-esr45 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.5 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- affected

People

(Reporter: cbook, Assigned: arai)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion)

Attachments

(4 files)

Attached file bughunter crash stack
Found via bughunter on http://www.charlotteobserver.com/ and reproduced on a windows 7 trunk debug build based on todays tip.

Assertion failure: ins->regexp()->type() == MIRType_Object, at c:/Users/mozilla/debug-build/mozilla-central/js/src/jit/Lowering.cpp:2164

Steps to reproduce:
--> Load http://www.charlotteobserver.com
--> Assertion failure 

filing as s-s based on previous experience with this MIR bugs but feel free to unhide if not
jandem, another one for you i guess
Flags: needinfo?(jdemooij)
arai is this from bug 1207922? :)
Flags: needinfo?(jdemooij) → needinfo?(arai.unmht)
Thanks.
yes, it should be bug 1207922.
give me 30 minutes to rebuild and test it.
MRegExpTester is created in following code, and it does not create when `callInfo.getArg(0)->type() != MIRType_Object` is true.

https://dxr.mozilla.org/mozilla-central/rev/0f363ae95dc90d593394ef464aa500804c824962/js/src/jit/MCallOptimize.cpp#1901
>     if (callInfo.getArg(0)->type() != MIRType_Object)
>         return InliningStatus_NotInlined;
> 
>     TemporaryTypeSet* arg0Types = callInfo.getArg(0)->resultTypeSet();
>     const Class* clasp = arg0Types ? arg0Types->getKnownClass(constraints()) : nullptr;
>     if (clasp != &RegExpObject::class_)
>         return InliningStatus_NotInlined;
> 
>     // string
>     if (callInfo.getArg(1)->mightBeType(MIRType_Object))
>         return InliningStatus_NotInlined;
> 
>     // lastIndex: Only inline if it's Int32.
>     if (callInfo.getArg(2)->type() != MIRType_Int32)
>         return InliningStatus_NotInlined;
> 
>     // sticky
>     if (callInfo.getArg(3)->type() != MIRType_Boolean)
>         return InliningStatus_NotInlined;
> 
>     JSContext* cx = GetJitContext()->cx;
>     if (!cx->compartment()->jitCompartment()->ensureRegExpTesterStubExists(cx)) {
>         oom();
>         return InliningStatus_Error;
>     }
> 
>     callInfo.setImplicitlyUsedUnchecked();
> 
>     MInstruction* tester = MRegExpTester::New(alloc(), callInfo.getArg(0), callInfo.getArg(1),
>                                               callInfo.getArg(2), callInfo.getArg(3));

However, the value of `ins->regexp()->type()` becomes MIRType_Value in the assertion failure.

Can the value of `type()` be modified between here and Lowering ?
Flags: needinfo?(jdemooij)
here's the minimal case

function f(str) {
   if (typeof str === 'string')
      str = new RegExp(str);
   for (var i = 0; i < 2000; i++)
      str.test('foo');
}
f("abc");
looks like both resultType_ and producer_ are modified between MRegExpTester::New and LIRGenerator::visitRegExpTester
(so, modified producer_ is used at last)

resultType_ is modified with following call:

* thread #5: tid = 0xcb1fee, 0x0000000100468955 js`js::jit::MDefinition::setResultType(this=0x0000000106c69440, type=MIRType_Value) + 21 at MIR.h:819, name = 'JS Helper', stop reason = watchpoint 1
  * frame #0: 0x0000000100468955 js`js::jit::MDefinition::setResultType(this=0x0000000106c69440, type=MIRType_Value) + 21 at MIR.h:819
    frame #1: 0x0000000100348662 js`js::jit::MPhi::specialize(this=0x0000000106c69440, type=MIRType_Value) + 34 at MIR.h:6954
    frame #2: 0x0000000100326d85 js`(anonymous namespace)::TypeAnalyzer::respecialize(this=0x0000000105009468, phi=0x0000000106c69440, type=MIRType_Value) + 69 at IonAnalysis.cpp:1300
    frame #3: 0x0000000100326cf5 js`(anonymous namespace)::TypeAnalyzer::propagateSpecialization(this=0x0000000105009468, phi=0x0000000106c649a0) + 645 at IonAnalysis.cpp:1342
    frame #4: 0x0000000100325318 js`(anonymous namespace)::TypeAnalyzer::specializePhis(this=0x0000000105009468) + 584 at IonAnalysis.cpp:1388
    frame #5: 0x00000001002e31c3 js`(anonymous namespace)::TypeAnalyzer::analyze(this=0x0000000105009468) + 51 at IonAnalysis.cpp:1804
    frame #6: 0x00000001002db21d js`js::jit::ApplyTypeInformation(mir=0x0000000106c61270, graph=0x0000000106c61040) + 45 at IonAnalysis.cpp:1818
    frame #7: 0x00000001002d5d8b js`js::jit::OptimizeMIR(mir=0x0000000106c61270) + 2299 at Ion.cpp:1634
    frame #8: 0x00000001002dd4fb js`js::jit::CompileBackEnd(mir=0x0000000106c61270) + 107 at Ion.cpp:2011
    frame #9: 0x00000001009083f8 js`js::HelperThread::handleIonWorkload(this=0x000000010366c600) + 984 at HelperThreads.cpp:1270
    frame #10: 0x0000000100907cc5 js`js::HelperThread::threadLoop(this=0x000000010366c600) + 709 at HelperThreads.cpp:1587
    frame #11: 0x0000000100905a2c js`js::HelperThread::ThreadMain(arg=0x000000010366c600) + 44 at HelperThreads.cpp:1198
    frame #12: 0x000000010321e3d1 libnspr4.dylib`_pt_root(arg=0x000000010365c530) + 449 at ptthread.c:212
    frame #13: 0x00007fff9046b05a libsystem_pthread.dylib`_pthread_body + 131
    frame #14: 0x00007fff9046afd7 libsystem_pthread.dylib`_pthread_start + 176
    frame #15: 0x00007fff904683ed libsystem_pthread.dylib`thread_start + 13

and producer_ is modified with following call:

* thread #5: tid = 0xcb1fee, 0x000000010046864a js`js::jit::MUse::setProducerUnchecked(this=0x0000000106c79f98, producer=0x0000000106c649a0) + 314 at MIR.h:169, name = 'JS Helper', stop reason = watchpoint 2
  * frame #0: 0x000000010046864a js`js::jit::MUse::setProducerUnchecked(this=0x0000000106c79f98, producer=0x0000000106c649a0) + 314 at MIR.h:169
    frame #1: 0x0000000100439181 js`js::jit::MDefinition::justReplaceAllUsesWith(this=0x0000000106c69440, dom=0x0000000106c649a0) + 305 at MIR.cpp:611
    frame #2: 0x000000010055c74d js`ReplaceAllUsesWith(from=0x0000000106c69440, to=0x0000000106c649a0) + 317 at ValueNumbering.cpp:150
    frame #3: 0x000000010055c35d js`js::jit::ValueNumberer::visitDefinition(this=0x0000000105009b38, def=0x0000000106c69440) + 941 at ValueNumbering.cpp:771
    frame #4: 0x000000010055d23f js`js::jit::ValueNumberer::visitBlock(this=0x0000000105009b38, block=0x0000000106c67368, dominatorRoot=0x0000000106c63c28) + 479 at ValueNumbering.cpp:963
    frame #5: 0x000000010055d540 js`js::jit::ValueNumberer::visitDominatorTree(this=0x0000000105009b38, dominatorRoot=0x0000000106c63c28) + 688 at ValueNumbering.cpp:1008
    frame #6: 0x000000010055d77d js`js::jit::ValueNumberer::visitGraph(this=0x0000000105009b38) + 189 at ValueNumbering.cpp:1046
    frame #7: 0x000000010055db3b js`js::jit::ValueNumberer::run(this=0x0000000105009b38, updateAliasAnalysis=UpdateAliasAnalysis) + 91 at ValueNumbering.cpp:1117
    frame #8: 0x00000001002d6305 js`js::jit::OptimizeMIR(mir=0x0000000106c61270) + 3701 at Ion.cpp:1699
    frame #9: 0x00000001002dd4fb js`js::jit::CompileBackEnd(mir=0x0000000106c61270) + 107 at Ion.cpp:2011
    frame #10: 0x00000001009083f8 js`js::HelperThread::handleIonWorkload(this=0x000000010366c600) + 984 at HelperThreads.cpp:1270
    frame #11: 0x0000000100907cc5 js`js::HelperThread::threadLoop(this=0x000000010366c600) + 709 at HelperThreads.cpp:1587
    frame #12: 0x0000000100905a2c js`js::HelperThread::ThreadMain(arg=0x000000010366c600) + 44 at HelperThreads.cpp:1198
    frame #13: 0x000000010321e3d1 libnspr4.dylib`_pt_root(arg=0x000000010365c530) + 449 at ptthread.c:212
    frame #14: 0x00007fff9046b05a libsystem_pthread.dylib`_pthread_body + 131
    frame #15: 0x00007fff9046afd7 libsystem_pthread.dylib`_pthread_start + 176
    frame #16: 0x00007fff904683ed libsystem_pthread.dylib`thread_start + 13


Am I missing something?
maybe I should add some code to avoid those replacements, or make it work even if the replacement happens?
so, the assertion failure disappears when I add `Mix3Policy<ObjectPolicy<0>, StringPolicy<1>, IntPolicy<2> >::Data` to MRegExpTester.
Is it a kind of correct fix?
which policy I should use for Boolean value?
Prepared 3 patches, adds Policy, uses Policy, and a testcase.
Assignee: nobody → arai.unmht
Flags: needinfo?(arai.unmht)
Attachment #8706005 - Flags: review?(jdemooij)
exec also has same issue, in almost same testcase.
Attachment #8706006 - Flags: review?(jdemooij)
currently the testcase covers 1st argument, the ObjectPolicy only.
will investigate when/if policy for other arguments could be used, but I guess this testcase will be sufficient for now.
Attachment #8706007 - Flags: review?(jdemooij)
Attachment #8706005 - Flags: review?(jdemooij) → review+
Attachment #8706006 - Flags: review?(jdemooij) → review+
Comment on attachment 8706007 [details] [diff] [review]
Part 3: Add test for Policy in RegExpMatcher and RegExpTester.

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

Thanks for fixing this so quickly!
Attachment #8706007 - Flags: review?(jdemooij) → review+
Comment on attachment 8706005 [details] [diff] [review]
Part 1: Add BooleanPolicy.

Thank you for reviewing :)

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
I'm not sure how to exploit this flaw tho, the flaw is the dereference of wrong pointer, by treating JS::Value as js::RegExpObject*

> Do comments in the patch, the check-in comment, or tests included in the patch
> paint a bulls-eye on the security problem?
the testcase (Part 3) describes how to hit the assertion failure.
might be better to land Part 1 and 2 first.

> Which older supported branches are affected by this flaw?
firefox 46 (current nightly only)

> If not all supported branches, which bug introduced the flaw?
bug 1207922

> Do you have backports for the affected branches? If not, how different, hard to
> create, and risky will they be?
-

> How likely is this patch to cause regressions; how much testing does it need?
Very low.  this adds extra checks that is already used in many places.
Attachment #8706005 - Flags: sec-approval?
Comment on attachment 8706005 [details] [diff] [review]
Part 1: Add BooleanPolicy.

Bugs that only affect trunk can land without sec-approval.
Flags: needinfo?(jdemooij)
Attachment #8706005 - Flags: sec-approval?
should I land Part 3 (testcase) as well?
For a nightly-only issue, no reason not to.
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.