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

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

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);

Comment 1

[Christian Holler (:decoder)]

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.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 3

[Brian Hackett [Laid off!]]

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();

Comment 4

[Brian Hackett [Laid off!]]

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

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 6

[Nicolas B. Pierron [:nbp]]

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

Comment 7

[Douglas Crosher [:dougc]]

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

Comment 8

[Brian Hackett [Laid off!]]

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

[Daniel Veditz [:dveditz]]

Comment on attachment 8434254 [details] [diff] [review]
patch

sec-approval+ = dveditz

Comment 10

[Daniel Veditz [: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.

Comment 11

[Daniel Veditz [:dveditz]]

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)

Comment 12

[Sylvestre Ledru [:Sylvestre]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0bb2357664

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2a0bb2357664

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c28fb44d72a9
https://hg.mozilla.org/releases/mozilla-beta/rev/400d802dc432

Do we want this on esr24 based on comment 12?

Comment 16

[Daniel Veditz [:dveditz]]

Sounds like it.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 8434254 [details] [diff] [review]
patch

approval-mozilla-esr24+ == dveditz

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/5509f025a233
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/13402cda531a
https://hg.mozilla.org/releases/mozilla-esr24/rev/4b216735dd0f

Comment 19

[Kamil Jozwiak [:kjozwiak]]

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.

Comment 20

[Brian Hackett [Laid off!]]

That behavior looks expected.

Comment 21

[Kamil Jozwiak [:kjozwiak]]

Thanks Brian, much appreciated. Marking this as verified.

Comment 22

[Sean Stangl [:sstangl]]

Can we land the testcase?

Comment 23

[Christian Holler (:decoder)]

Sounds like this has been fixed on all supported branches for a while, so I assume yes.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

Yes, the testcase should go in.