Closed Bug 1019684 Opened 5 years ago Closed 5 years ago

Assertion failure: isInRange(offset), at jit/arm/Assembler-arm.h:989 or Crash [@ js::jit::Simulator::instructionDecode] with RegExp

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

ARM
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox29 --- wontfix
firefox30 + wontfix
firefox31 + verified
firefox32 + verified
firefox33 + verified
firefox-esr24 31+ verified
b2g-v1.2 --- wontfix
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: decoder, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update][adv-main31+][adv-esr24.7+])

Crash Data

Attachments

(1 file)

The following testcase asserts on mozilla-central revision f28005b84ed0 (run with --fuzzing-safe):


var SECTION = "expression-015";
var TITLE   = "Function Calls";
var N = 100 * 1000;
var a = new Array(N);
for (var i = 0; i != N; ++i) {
  a[i] = SECTION;
}
var str = a.join('|');
var re = new RegExp(str);
re.exec(N - 1);
Tested on an ARM simulator build.

Backtrace for assertion:

Program received signal SIGSEGV, Segmentation fault.
0x083f55d8 in js::jit::BOffImm::BOffImm (this=0xffff896c, offset=-33554672) at  js/src/jit/arm/Assembler-arm.h:989
989             JS_ASSERT(isInRange(offset));
#0  0x083f55d8 in js::jit::BOffImm::BOffImm (this=0xffff896c, offset=-33554672) at  js/src/jit/arm/Assembler-arm.h:989
#1  0x083e6edd in diffB<js::jit::BOffImm> (other=..., this=0xffff8968) at  js/src/jit/shared/IonAssemblerBuffer.h:36
#2  js::jit::Assembler::as_b (this=0xffff9904, l=0xa9dc6c0, c=3758096384, isPatchable=false) at  js/src/jit/arm/Assembler-arm.cpp:1853
#3  0x081378a0 in jump (label=0xa9dc6c0, this=0xffff9904) at  js/src/jit/arm/MacroAssembler-arm.h:711
#4  js::irregexp::NativeRegExpMacroAssembler::JumpOrBacktrack (this=0xffff98e8, to=0xa9dc6c0) at  js/src/irregexp/NativeRegExpMacroAssembler.cpp:1117
#5  0x08154db2 in js::irregexp::ChoiceNode::EmitOutOfLineContinuation (this=this@entry=0x9d19370, compiler=0xffff94a4, trace=trace@entry=0xffff8b78, alternative=..., alt_gen=alt_gen@entry=0xa9dc6b8, preload_characters=preload_characters@entry=2, next_expects_preload=true) at  js/src/irregexp/RegExpEngine.cpp:4189
#6  0x08177dad in js::irregexp::ChoiceNode::Emit (this=0x9d19370, compiler=0xffff94a4, trace=0xffff8e84) at  js/src/irregexp/RegExpEngine.cpp:4151
#7  0x08175bfe in js::irregexp::ActionNode::Emit (this=0xa3c4a70, compiler=0xffff94a4, trace=0xffff8f34) at  js/src/irregexp/RegExpEngine.cpp:4226


Crash trace:

Program received signal SIGSEGV, Segmentation fault.
js::jit::Simulator::instructionDecode (this=this@entry=0x9209c10, instr=instr@entry=0xf16c6588) at  js/src/jit/arm/Simulator-arm.cpp:4013
4013        if (instr->conditionField() == kSpecialCondition) {
#0  js::jit::Simulator::instructionDecode (this=this@entry=0x9209c10, instr=instr@entry=0xf16c6588) at  js/src/jit/arm/Simulator-arm.cpp:4013
#1  0x0834a7bc in js::jit::Simulator::execute<false> (this=this@entry=0x9209c10) at  js/src/jit/arm/Simulator-arm.cpp:4069
#2  0x0833c940 in js::jit::Simulator::callInternal (this=this@entry=0x9209c10, entry=entry@entry=0xf352b008 "^m\206\352\200E\006\343lE", <incomplete sequence \343>) at  js/src/jit/arm/Simulator-arm.cpp:4151
#3  0x0833ca06 in js::jit::Simulator::call (this=0x9209c10, entry=0xf352b008 "^m\206\352\200E\006\343lE", <incomplete sequence \343>, argument_count=1) at  js/src/jit/arm/Simulator-arm.cpp:4232
#4  0x080fece6 in js::irregexp::ExecuteCode (cx=0x920a590, codeBlock=0xf663c4e8, chars=0xf6640028 u"99999", start=0, length=5, matches=0xffffbdd4) at  js/src/irregexp/RegExpEngine.cpp:1665
#5  0x0844d9a7 in js::RegExpShared::execute (this=0x92da410, cx=0x920a590, chars=0xf6640028 u"99999", length=5, lastIndex=0xffffbc20, matches=...) at  js/src/vm/RegExpObject.cpp:757
#6  0x0856493e in ExecuteRegExpImpl (lastIndex=0xffffbc20, length=<optimized out>, chars=<optimized out>, input=..., re=..., res=0x920a6b0, cx=0x920a590, matches=...) at  js/src/builtin/RegExp.cpp:112
#7  js::ExecuteRegExp (cx=0x920a590, regexp=(JSObject * const) 0xf663e060 [object RegExp], string="99999", matches=..., staticsUpdate=js::UpdateRegExpStatics) at  js/src/builtin/RegExp.cpp:602
edi     0xf16c6588      -244554360
=> 0x8339053 <js::jit::Simulator::instructionDecode(js::jit::SimInstruction*)+51>:      mov    (%edi),%edx


Marked s-s and sec-critical based on the traces. Could be an irregexp regression, needinfo? on Brian for that.
Crash Signature: [@ js::jit::Simulator::instructionDecode]
Flags: needinfo?(bhackett1024)
Keywords: crash, sec-critical
Attached patch patchSplinter Review
This is an old bug in the ARM assembler.  If we generate branches that are out of range we just bust on this assert in debug builds, or do the wrong thing and then crash in release builds.  Since the ARM assembler is going to be replaced soon it doesn't make much sense to do a proper fix, so this patch just turns the assert into a release mode safe crash.  This patch also fixes some tests for out of range branches which then used the wrong method to signal that the assembler is bailing out.
Assignee: nobody → bhackett1024
Attachment #8434254 - Flags: review?(mrosenberg)
Flags: needinfo?(bhackett1024)
Ion has script size limits which should make this bug very hard to hit there, but baseline doesn't really have limits on the scripts it will compile.  The following testcase hits this assert when running with --baseline-eager:

var str = "while (x) {";
for (var i = 0; i < 1000000; i++)
    str += "delete x;\n";
str += "}";

var f = new Function(str);
f();
Whiteboard: [jsbugmon:update]
Attachment #8434254 - Flags: review?(mrosenberg) → review+
Comment on attachment 8434254 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily.

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 branches with the baseline compiler, probably.

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

Old bug in the ARM assembler.

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

Trivial.

How likely is this patch to cause regressions; how much testing does it need?

Not at all.

[Approval Request Comment]
User impact if declined: Potentially exploitable crashes
Fix Landed on Version: none yet
Risk to taking this patch (and alternatives if risky): none
Attachment #8434254 - Flags: sec-approval?
Attachment #8434254 - Flags: approval-mozilla-esr24?
Attachment #8434254 - Flags: approval-mozilla-beta?
Attachment #8434254 - Flags: approval-mozilla-aurora?
We're doing a build #2 of the FF30 release candidate for another last-minute sec landing so if this is a low risk uplift, we could take as a ride-along.
(In reply to Brian Hackett (:bhackett) from comment #2)
> Since the ARM assembler is going to be replaced soon it doesn't make much sense to do a proper fix

Since when, we have plan for the Assembler buffer but not for the assembler AFAIK.  The Assembler x64 would be a reimplementation as they differ too much, and the current version will stick.  Don't we already have a working case for that when the branch is produced by as_BranchPool?  Could we just move that down within as_b?
(In reply to Lukas Blakk [:lsblakk] from comment #5)
> We're doing a build #2 of the FF30 release candidate for another last-minute
> sec landing so if this is a low risk uplift, we could take as a ride-along.

Yes, please take this one if you can. The significant change is that it now crashes cooperatively rather than being a potential exploit.
(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> (In reply to Brian Hackett (:bhackett) from comment #2)
> > Since the ARM assembler is going to be replaced soon it doesn't make much sense to do a proper fix
> 
> Since when, we have plan for the Assembler buffer but not for the assembler
> AFAIK.  The Assembler x64 would be a reimplementation as they differ too
> much, and the current version will stick.  Don't we already have a working
> case for that when the branch is produced by as_BranchPool?  Could we just
> move that down within as_b?

I don't really know what the plans are here, I'm just going by what Marty told me on IRC.  Most of the code related to this is in IonAssemblerBuffer anyways.  I looked into doing a better fix where we bail out of compilation instead of safe crashing, but that requires changing the behavior / signature of several other methods and is riskier.
Comment on attachment 8434254 [details] [diff] [review]
patch

sec-approval+ = dveditz
Attachment #8434254 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(dveditz)
Do we really need to land this fix on ESR-24? We don't ship a Fennec ESR so who is using ESR-24 on ARM?

Fixing this on the various b2g branches would be good, of course.
I don't think we need to rush this into Firefox 30. Would of course be nice to land on the b2g-30 branch, but otherwise mozilla-central and aurora are fine (or beta too if you don't land until next week)
Attachment #8434254 - Flags: approval-mozilla-esr24?
I would take this patch for 31 (as this is going to be the next ESR).

(In reply to Daniel Veditz [:dveditz] from comment #10)
> who is using ESR-24 on ARM?
Debian packages
https://hg.mozilla.org/mozilla-central/rev/2a0bb2357664
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Attachment #8434254 - Flags: approval-mozilla-beta?
Attachment #8434254 - Flags: approval-mozilla-beta+
Attachment #8434254 - Flags: approval-mozilla-aurora?
Attachment #8434254 - Flags: approval-mozilla-aurora+
Sounds like it.
Flags: needinfo?(dveditz)
Comment on attachment 8434254 [details] [diff] [review]
patch

approval-mozilla-esr24+ == dveditz
Attachment #8434254 - Flags: approval-mozilla-esr24+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main31+][adv-esr24.7+]
I reproduced the original issue using m-c revision f28005b84ed0 with the flags outlined in comment #0 & comment #1:

Assertion failure: isInRange(offset), at /home/kjozwiak/mozilla/mozilla-central/js/src/jit/arm/Assembler-arm.h:989
Segmentation fault (core dumped)

I went through the following verification:

firefox33 [Revision ID: 835e22069c1a tip]
- ran the poc code from comment #0 without receiving the assert or any other error messages

firefox32 [Revision ID: 244a3dc5bf49 tip]
- When running the poc code from comment #0, received the following assert:

Assertion failure: [unhandlable oom] BOffImm, at /home/kjozwiak/mozilla/mozilla-aurora/js/src/jscntxt.cpp:1425
Hit MOZ_CRASH() at /home/kjozwiak/mozilla/mozilla-aurora/js/src/jscntxt.cpp:1426
Segmentation fault (core dumped)

firefox31 [Revision ID: 6befadcaa685]
- When I ran through the poc code from comment #0, received the following error message:

poc.js:9:4 InternalError: regular expression too complex

firefox-esr24 [Revision ID: 31b0c1ff3c0b tip]
- When I ran through the poc code from comment #0, received the following error message:

poc.js:9:0 InternalError: regular expression too complex

On all the different branches, I used the same steps:
- Built using: ../configure --enable-debug --disable-optimize --enable-arm-simulator
- Copied poc code from comment #0 into poc.js
- Ran ./js --fuzzing-safe poc.js

Brian, would you be able to take a look at the assert I'm receiving in fx32 and the error messages that I'm receiving in fx31 and in fxESR24? Let me know if you need anything else and I'll try providing as much information as possible.
Flags: needinfo?(bhackett1024)
That behavior looks expected.
Flags: needinfo?(bhackett1024)
Thanks Brian, much appreciated. Marking this as verified.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa!]
Can we land the testcase?
Flags: needinfo?(choller)
Sounds like this has been fixed on all supported branches for a while, so I assume yes.
Flags: needinfo?(choller)
Yes, the testcase should go in.
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.