1234402 - Crash [@ js::irregexp::ChoiceNode::Emit] with OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 388bdc46ba51 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2):

offThreadCompileScript(`
[null, "", ""].forEach(function(locales) {
  try {
    Intl.NumberFormat(locales)
  } catch (e) {}
  oomAfterAllocations(100);
})
`);
runOffThreadScript()



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::irregexp::ChoiceNode::Emit (this=0x7ffff69dacd8, compiler=0x7fffffff8a40, trace=0x7fffffff7dd0) at js/src/irregexp/RegExpEngine.cpp:4191
#0  js::irregexp::ChoiceNode::Emit (this=0x7ffff69dacd8, compiler=0x7fffffff8a40, trace=0x7fffffff7dd0) at js/src/irregexp/RegExpEngine.cpp:4191
#1  0x0000000000bfc932 in js::irregexp::ChoiceNode::EmitOutOfLineContinuation (this=this@entry=0x7ffff69dac38, compiler=compiler@entry=0x7fffffff8a40, trace=trace@entry=0x7fffffff7f90, alternative=..., alt_gen=alt_gen@entry=0x7fffffff8028, preload_characters=preload_characters@entry=4, next_expects_preload=next_expects_preload@entry=true) at js/src/irregexp/RegExpEngine.cpp:4311
#2  0x0000000000c0ef3e in js::irregexp::ChoiceNode::Emit (this=0x7ffff69dac38, compiler=0x7fffffff8a40, trace=<optimized out>) at js/src/irregexp/RegExpEngine.cpp:4282
#3  0x0000000000c0ed47 in js::irregexp::ChoiceNode::Emit (this=0x7ffff69d6418, compiler=0x7fffffff8a40, trace=0x7fffffff8680) at js/src/irregexp/RegExpEngine.cpp:4251
#4  0x0000000000c0c08e in js::irregexp::AssertionNode::Emit (this=0x7ffff69dbee8, compiler=0x7fffffff8a40, trace=0x7fffffff8750) at js/src/irregexp/RegExpEngine.cpp:2986
#5  0x0000000000c0d3ef in js::irregexp::ActionNode::Emit (this=0x7ffff69dbf30, compiler=0x7fffffff8a40, trace=0x7fffffff8840) at js/src/irregexp/RegExpEngine.cpp:4357
#6  0x0000000000bfb68b in js::irregexp::RegExpCompiler::Assemble (this=this@entry=0x7fffffff8a40, cx=cx@entry=0x7ffff6907400, assembler=<optimized out>, start=start@entry=0x7ffff69dbf30, capture_count=<optimized out>) at js/src/irregexp/RegExpEngine.cpp:1606
#7  0x0000000000c0b106 in js::irregexp::CompilePattern (cx=cx@entry=0x7ffff6907400, shared=shared@entry=0x7ffff69d3b80, data=data@entry=0x7fffffff9a50, sample=..., sample@entry=..., is_global=is_global@entry=false, ignore_case=<optimized out>, is_ascii=true, match_only=true, force_bytecode=force_bytecode@entry=false, sticky=false) at js/src/irregexp/RegExpEngine.cpp:1761
#8  0x0000000000aa56c5 in js::RegExpShared::compile (this=this@entry=0x7ffff69d3b80, cx=cx@entry=0x7ffff6907400, pattern=..., pattern@entry=..., input=..., input@entry=..., mode=mode@entry=js::RegExpShared::MatchOnly, force=force@entry=js::RegExpShared::DontForceByteCode) at js/src/vm/RegExpObject.cpp:534
#9  0x0000000000aa7d41 in js::RegExpShared::compile (this=0x7ffff69d3b80, cx=0x7ffff6907400, input=..., mode=js::RegExpShared::MatchOnly, force=js::RegExpShared::DontForceByteCode) at js/src/vm/RegExpObject.cpp:503
#10 0x0000000000aa7ec5 in js::RegExpShared::compileIfNecessary (this=this@entry=0x7ffff69d3b80, cx=cx@entry=0x7ffff6907400, input=..., input@entry=..., mode=mode@entry=js::RegExpShared::MatchOnly, force=force@entry=js::RegExpShared::DontForceByteCode) at js/src/vm/RegExpObject.cpp:556
#11 0x0000000000aa7f2a in js::RegExpShared::execute (this=this@entry=0x7ffff69d3b80, cx=cx@entry=0x7ffff6907400, input=input@entry=..., start=start@entry=0, matches=matches@entry=0x0) at js/src/vm/RegExpObject.cpp:568
#12 0x0000000000cd9238 in ExecuteRegExpImpl (cx=cx@entry=0x7ffff6907400, res=res@entry=0x0, re=..., input=input@entry=..., searchIndex=0, matches=matches@entry=0x0) at js/src/builtin/RegExp.cpp:98
#13 0x0000000000cdbd42 in js::ExecuteRegExp (cx=cx@entry=0x7ffff6907400, regexp=..., regexp@entry=..., string=..., string@entry=..., matches=matches@entry=0x0, staticsUpdate=staticsUpdate@entry=js::DontUpdateRegExpStatics) at js/src/builtin/RegExp.cpp:846
#14 0x0000000000cdc031 in js::regexp_test_no_statics (cx=0x7ffff6907400, argc=<optimized out>, vp=0x7ffff4671540) at js/src/builtin/RegExp.cpp:979
#15 0x0000000000a7e002 in js::CallJSNative (cx=0x7ffff6907400, native=0xcdbf60 <js::regexp_test_no_statics(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#53 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6878
rax	0x7ffff69dda80	140737330928256
rbx	0x7fffffff7dd0	140737488322000
rcx	0x4	4
rdx	0x0	0
rsi	0xa	10
rdi	0x7fffffff7b10	140737488321296
rbp	0x7fffffff7d80	140737488321920
rsp	0x7fffffff7990	140737488320912
r8	0x7fffffff7d1c	140737488321820
r9	0xdfdfffdf	3755999199
r10	0x1	1
r11	0x20	32
r12	0x7ffff69dacd8	140737330916568
r13	0x0	0
r14	0x0	0
r15	0x1	1
rip	0xc0eaf6 <js::irregexp::ChoiceNode::Emit(js::irregexp::RegExpCompiler*, js::irregexp::Trace*)+1094>
=> 0xc0eaf6 <js::irregexp::ChoiceNode::Emit(js::irregexp::RegExpCompiler*, js::irregexp::Trace*)+1094>:	mov    %ecx,0xc(%r14)
   0xc0eafa <js::irregexp::ChoiceNode::Emit(js::irregexp::RegExpCompiler*, js::irregexp::Trace*)+1098>:	mov    (%rbx),%rax

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Crash on OOM in AlternativeGenerationList constructor.

Looks like there are some fallible operations in AlternativeGeneration ctor:
  * Vector::reserve
  * Vector::append
  * js_new

and OOM happens there is not handlable, as there are already some AutoEnterOOMUnsafeRegion in other methods.

So, added |AutoEnterOOMUnsafeRegion oomUnsafe| there and |oomUnsafe.crash(...)| for each operation.


Also, there seems to be some more fallible operations in RegExpEngine.cpp.
It might be better to investigate them and fix them if needed, maybe in separated bugs.

Comment 2

[Fuzzing Team]

JSBugMon: Bisection requested, result:
Due to skipped revisions, the first bad revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a0dd5a83ba36
user:        Jan de Mooij
date:        Thu Jul 24 11:56:43 2014 +0200
summary:     Bug 1031529 part 2 - Remove JS_THREADSAFE #ifdefs everywhere. r=bhackett

changeset:   https://hg.mozilla.org/mozilla-central/rev/6426fef52f51
user:        Jan de Mooij
date:        Thu Jul 24 11:56:45 2014 +0200
summary:     Bug 1031529 part 3 - Step defining JS_THREADSAFE, remove --disable-threadsafe. r=glandium

This iteration took 53.133 seconds to run.

Comment 3

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

This probably hails from the irregexp landing and probably older than the range described in comment 2, making the range inaccurate.

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8701019 [details] [diff] [review]
Crash on OOM in AlternativeGenerationList constructor.

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

Sorry for the delay. Currently on PTO and only saw now.

::: js/src/irregexp/RegExpEngine.cpp
@@ +4238,5 @@
> +        if (!alt_gens_.reserve(count))
> +            oomUnsafe.crash("AlternativeGenerationList reserve");
> +        for (size_t i = 0; i < count && i < kAFew; i++) {
> +            if (!alt_gens_.append(a_few_alt_gens_ + i))
> +                oomUnsafe.crash("AlternativeGenerationList append few");

since we used reserved on alt_gens, this cannot fail.
Please use infallibleAppend for these.

@@ +4245,5 @@
> +            AlternativeGeneration* gen = js_new<AlternativeGeneration>();
> +            if (!gen)
> +                oomUnsafe.crash("AlternativeGenerationList js_new");
> +            if (!alt_gens_.append(gen))
> +                oomUnsafe.crash("AlternativeGenerationList append after js_new");

since we used reserved on alt_gens, this cannot fail.
Please use infallibleAppend for these.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Crash on OOM in AlternativeGenerationList constructor.

Thank you for reviewing :)

changed those 2 append to infallibleAppend, fixed testcase to check helperThreadCount() for --no-threads case, and added allow-oom too.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8701856 [details] [diff] [review]
Crash on OOM in AlternativeGenerationList constructor.

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

Thank you

::: js/src/irregexp/RegExpEngine.cpp
@@ +4238,5 @@
> +        if (!alt_gens_.reserve(count))
> +            oomUnsafe.crash("AlternativeGenerationList reserve");
> +        for (size_t i = 0; i < count && i < kAFew; i++) {
> +            alt_gens_.infallibleAppend(a_few_alt_gens_ + i);
> +        }

nit: no {} around a single line for body

Comment 7

[Jan de Mooij [:jandem]]

Note that the reserve/append calls on this Vector can't fail, because the Vectors in irregexp code use LifoAllocPolicy<Infallible>. I still need to figure out what to do about this for bug 1231224..

Comment 8

[Tooru Fujisawa [:arai]]

thank you for reviewing :)

(In reply to Jan de Mooij [:jandem] from comment #7)
> Note that the reserve/append calls on this Vector can't fail, because the
> Vectors in irregexp code use LifoAllocPolicy<Infallible>. I still need to
> figure out what to do about this for bug 1231224..

So, maybe I should not touch reserve/append code there at all?
does it make sense to use infallibleAppend for it? (looks like there's no infallibleReserve)

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #8)
> So, maybe I should not touch reserve/append code there at all?
> does it make sense to use infallibleAppend for it? (looks like there's no
> infallibleReserve)

I think you can just get rid of the OOM check after reserve(). The infallibleAppend calls are nice though :)

Comment 10

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c4a945741f8438c91330646e12b127da495e07
Bug 1234402 - Crash on OOM in AlternativeGenerationList constructor. r=bbouvier

Comment 11

[Tooru Fujisawa [:arai]]

okay, fixed :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1c4a945741f

Comment 13

[Hannes Verschore [:h4writer]]

Thanks for stepping in Jan. Removing myself from needinfo, since it is fixed.