1254106 - Assertion failure: offset_ == offset (offset fits in 31 bits), at js/src/jit/Label.h:56

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision b6acf4d4fc20 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-simulator=arm --enable-debug, run with --arm-hwcap=vfp):

var summary = 'Do not overflow 64K boundary in treeDepth';
var N = 100 * 1000;
var a = new Array(N);
for (var i = 0; i != N; ++i) {
    a[i] = summary;
}
var str = a.join('|'); // str is 0|1|2|3|...|<printed value of N -1>
var re = new RegExp(str);
re.exec(N - 1);


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x08461bb8 in bind (offset=<optimized out>, this=0xffff8c44) at js/src/jit/Label.h:56
#0  0x08461bb8 in bind (offset=<optimized out>, this=0xffff8c44) at js/src/jit/Label.h:56
#1  js::jit::Assembler::bind (this=this@entry=0xffff9e80, label=label@entry=0xffff8c44, boff=boff@entry=...) at js/src/jit/arm/Assembler-arm.cpp:2799
#2  0x088a1210 in js::irregexp::NativeRegExpMacroAssembler::CheckBacktrackStackLimit (this=this@entry=0xffff9e64) at js/src/irregexp/NativeRegExpMacroAssembler.cpp:1113
#3  0x088a9139 in js::irregexp::NativeRegExpMacroAssembler::PushBacktrack (this=0xffff9e64, label=0xffff8d1c) at js/src/irregexp/NativeRegExpMacroAssembler.cpp:1044
#4  0x088c0e7c in js::irregexp::Trace::Flush (this=this@entry=0xffff8f24, compiler=compiler@entry=0xffff9a10, successor=successor@entry=0xf1a25990) at js/src/irregexp/RegExpEngine.cpp:2805
#5  0x088c1283 in js::irregexp::RegExpNode::LimitVersions (this=this@entry=0xf1a25990, compiler=compiler@entry=0xffff9a10, trace=trace@entry=0xffff8f24) at js/src/irregexp/RegExpEngine.cpp:4710
#6  0x088c18ce in LimitVersions (trace=0xffff8f24, compiler=0xffff9a10, this=0xf1a25990) at js/src/irregexp/RegExpEngine.cpp:4642
#7  js::irregexp::ActionNode::Emit (this=0xf1a25990, compiler=0xffff9a10, trace=0xffff8f24) at js/src/irregexp/RegExpEngine.cpp:4531
#8  0x088c272b in js::irregexp::TextNode::Emit (this=0xf1548358, compiler=0xffff9a10, trace=0xffff8fd0) at js/src/irregexp/RegExpEngine.cpp:4007
#9  0x088b53b8 in js::irregexp::ChoiceNode::EmitOutOfLineContinuation (this=this@entry=0xf1a259c8, compiler=compiler@entry=0xffff9a10, trace=trace@entry=0xffff9110, alternative=..., alt_gen=alt_gen@entry=0xf0d315c0, preload_characters=preload_characters@entry=4, next_expects_preload=next_expects_preload@entry=true) at js/src/irregexp/RegExpEngine.cpp:4507
#10 0x088c31f5 in js::irregexp::ChoiceNode::Emit (this=0xf1a259c8, compiler=0xffff9a10, trace=0xffff93e4) at js/src/irregexp/RegExpEngine.cpp:4478
#11 0x088c17e4 in js::irregexp::ActionNode::Emit (this=0xf12d3c90, compiler=0xffff9a10, trace=0xffff9490) at js/src/irregexp/RegExpEngine.cpp:4579
#12 0x088b548d in js::irregexp::ChoiceNode::EmitOutOfLineContinuation (this=this@entry=0xf12d3cd8, compiler=compiler@entry=0xffff9a10, trace=trace@entry=0xffff95d0, alternative=..., alt_gen=alt_gen@entry=0xffff9640, preload_characters=preload_characters@entry=4, next_expects_preload=next_expects_preload@entry=false) at js/src/irregexp/RegExpEngine.cpp:4523
#13 0x088c31f5 in js::irregexp::ChoiceNode::Emit (this=0xf12d3cd8, compiler=0xffff9a10, trace=0xffff98c4) at js/src/irregexp/RegExpEngine.cpp:4478
#14 0x088b439b in js::irregexp::RegExpCompiler::Assemble (this=this@entry=0xffff9a10, cx=cx@entry=0xf7a76020, assembler=assembler@entry=0xffff9e64, start=start@entry=0xf12d3cd8, capture_count=0) at js/src/irregexp/RegExpEngine.cpp:1726
#15 0x088bf165 in js::irregexp::CompilePattern (cx=cx@entry=0xf7a76020, shared=shared@entry=0xf7a9b240, data=data@entry=0xffffa6c0, sample=sample@entry=..., is_global=is_global@entry=false, ignore_case=ignore_case@entry=false, is_ascii=true, match_only=match_only@entry=false, force_bytecode=force_bytecode@entry=false, sticky=sticky@entry=false, unicode=unicode@entry=false) at js/src/irregexp/RegExpEngine.cpp:1883
#16 0x08738c6d in js::RegExpShared::compile (this=this@entry=0xf7a9b240, cx=cx@entry=0xf7a76020, pattern=pattern@entry=..., input=input@entry=..., mode=mode@entry=js::RegExpShared::Normal, sticky=sticky@entry=false, force=force@entry=js::RegExpShared::DontForceByteCode) at js/src/vm/RegExpObject.cpp:561
#17 0x0873900d in js::RegExpShared::compile (this=0xf7a9b240, cx=0xf7a76020, input=..., mode=js::RegExpShared::Normal, sticky=false, force=js::RegExpShared::DontForceByteCode) at js/src/vm/RegExpObject.cpp:530
#18 0x087391c1 in js::RegExpShared::compileIfNecessary (this=<optimized out>, this@entry=0xf7a9b240, cx=<optimized out>, cx@entry=0xf7a76020, input=input@entry=..., mode=<optimized out>, mode@entry=js::RegExpShared::Normal, sticky=<optimized out>, sticky@entry=false, force=<optimized out>, force@entry=js::RegExpShared::DontForceByteCode) at js/src/vm/RegExpObject.cpp:583
#19 0x087392bb in js::RegExpShared::execute (this=this@entry=0xf7a9b240, cx=cx@entry=0xf7a76020, input=input@entry=..., start=start@entry=0, sticky=sticky@entry=false, matches=matches@entry=0xffffad60, endIndex=endIndex@entry=0x0) at js/src/vm/RegExpObject.cpp:597
#20 0x089955e3 in ExecuteRegExpImpl (cx=cx@entry=0xf7a76020, res=res@entry=0xf7a8b9d0, re=..., input=input@entry=..., searchIndex=0, sticky=sticky@entry=false, matches=matches@entry=0xffffad60, endIndex=endIndex@entry=0x0) at js/src/builtin/RegExp.cpp:107
#21 0x08995982 in ExecuteRegExp (cx=cx@entry=0xf7a76020, regexp=..., string=string@entry=..., lastIndex=<optimized out>, lastIndex@entry=0, sticky=sticky@entry=false, matches=matches@entry=0xffffad60, endIndex=endIndex@entry=0x0, staticsUpdate=staticsUpdate@entry=js::UpdateRegExpStatics) at js/src/builtin/RegExp.cpp:868
#22 0x08998db6 in RegExpMatcherImpl (cx=cx@entry=0xf7a76020, regexp=..., regexp@entry=..., string=string@entry=..., lastIndex=0, sticky=sticky@entry=false, staticsUpdate=staticsUpdate@entry=js::UpdateRegExpStatics, rval=rval@entry=...) at js/src/builtin/RegExp.cpp:888
#23 0x08999054 in js::RegExpMatcher (cx=0xf7a76020, argc=4, vp=0xf4125168) at js/src/builtin/RegExp.cpp:924
#24 0x086a42aa in js::CallJSNative (cx=0xf7a76020, native=0x8998f00 <js::RegExpMatcher(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#52 main (argc=3, argv=0xffffcbf4, envp=0xffffcc04) at js/src/shell/js.cpp:7250
eax	0x0	0
ebx	0x9855438	159732792
ecx	0xf7e3a88c	-136075124
edx	0x0	0
esi	0x400062b8	1073767096
edi	0xffff8c44	-29628
ebp	0xffff8c08	4294937608
esp	0xffff8bb0	4294937520
eip	0x8461bb8 <js::jit::Assembler::bind(js::jit::Label*, js::jit::BufferOffset)+632>
=> 0x8461bb8 <js::jit::Assembler::bind(js::jit::Label*, js::jit::BufferOffset)+632>:	movl   $0x38,0x0
   0x8461bc2 <js::jit::Assembler::bind(js::jit::Label*, js::jit::BufferOffset)+642>:	call   0x80fcb10 <abort()>


I'm not sure what's going on here but an offset not having the expected size/width doesn't sound nice to me, marking s-s.

Comment 1

[Eric Faust [:efaust]]

This was filed 10 days ago and we never got a bisection....Chris, can you kick jsbugmon?

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Taking on the bisection, the rejection of invalid configure options (--enable-arm-simulator vs --enable-simulator=arm) causes some problems with compiling older ARM versions (which I've since added a last known working rev), so I will do this manually.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5f33eff8767e
user:        Jan de Mooij
date:        Mon Feb 22 11:21:54 2016 +0100
summary:     Bug 1234408 - Fix ARM assembler to bind labels on OOM to silence ~Label assert. r=bbouvier

Jan, is bug 1234408 a likely regressor?

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #3)
> The first good revision is:

Sigh~ this may be a wrong bisection after all... :(

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

It is another case of "a label offset is too long because code we're generating is too large". The generated regexp has 100k tests of the form A | B | ..., which seems enough to generate out-of-bounds labels. The fix would be to abandon regexp compilation whenever a label target is too far away, but I am not sure we can generalize this properly, and where this happens in particular.

Comment 6

[Eric Faust [:efaust]]

Hannes, Benjamin mentioned that maybe you would make sense to look into this, since it involves the RegExp engine? If so, can we at least get an idea whether we should be worried about this for 47?

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Patch

The assembler oom() is true. In that case we shouldn't try to use the buffer anymore. This code does the same as a little bit above when label is used.
We just forget to do it when label is unused.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8736334 [details] [diff] [review]
Patch

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

Is it possible to move the |if (oom())| in the do-while loop to the top of the function? bind() doesn't emit any instructions so oom() shouldn't change from false -> true while we're inside the loop..

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: 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?
Could be every ARM release ever.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
It could easily get backported. Not sure if needed. At the moment I assume this bug isn't too bad. We throw away the generated code, since we had an oom. So shouldn't really be a security issue. But I'm not 100% sure.

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

Comment 10

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

This needs a security rating before it can have sec-approval to be checked in since only sec-criticals and sec-highs need sec-approval.

None of the comments nor the initial report are clear on the security implications of the bug. Based on "So shouldn't really be a security issue", I'm not sure if this is even a security bug.

Comment 11

[Hannes Verschore [:h4writer]]

Since we think this is not exploitable (though not 100% sure), made it sec-low.

Comment 12

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

Comment on attachment 8738919 [details] [diff] [review]
Patch

Cancelling sec-approval?. Just check it in.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f9c628820e2

Comment 14

[Eric Faust [:efaust]]

Given that this has been around forever, and we marked it sec-low. I'm going to say we shouldn't bother uplifting. WONTFIX 47.