1248101 - Crash [@ Balloc] with OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 576a6dcde5b6 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe min.js):

oomTest(function() 1e300)


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
Balloc (state=<optimized out>, k=1) at js/src/dtoa.c:580
#0  Balloc (state=<optimized out>, k=1) at js/src/dtoa.c:580
#1  0x0000000000d20d03 in mult (state=state@entry=0x7ffff69250b0, a=a@entry=0x7ffff3c89e80, b=b@entry=0x7ffff3c89ee0) at js/src/dtoa.c:836
#2  0x0000000000d21883 in pow5mult (state=state@entry=0x7ffff69250b0, b=0x7ffff3c89e80, b@entry=0x7ffff6987820, k=37, k@entry=300) at js/src/dtoa.c:947
#3  0x0000000000d22326 in _strtod (se=0x7fffffffc180, s00=<optimized out>, state=0x7ffff69250b0) at js/src/dtoa.c:1953
#4  js_strtod_harder (state=0x7ffff69250b0, s00=<optimized out>, se=se@entry=0x7fffffffc180, err=err@entry=0x7fffffffc17c) at js/src/jsdtoa.cpp:71
#5  0x000000000094c7fa in js_strtod<char16_t> (cx=0x7ffff6907000, begin=begin@entry=0x7ffff6997926 u"1e300)⬠翿", end=<optimized out>, dEnd=dEnd@entry=0x7fffffffc2a0, d=d@entry=0x7fffffffc270) at js/src/jsnum.cpp:1789
#6  0x0000000000bff3fd in js::frontend::TokenStream::getTokenInternal (this=0x7fffffffcd70, ttp=0x7fffffffc370, modifier=modifier@entry=js::frontend::Token::Operand) at js/src/frontend/TokenStream.cpp:1298
#7  0x00000000004cd5ab in js::frontend::TokenStream::getToken (this=this@entry=0x7fffffffcd70, ttp=ttp@entry=0x7fffffffc370, modifier=js::frontend::Token::Operand) at js/src/frontend/TokenStream.h:552
#8  0x00000000005010e5 in js::frontend::Parser<js::frontend::FullParseHandler>::functionArgsAndBodyGeneric (this=this@entry=0x7fffffffcd40, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, pn=0x7ffff69cd020, fun=fun@entry=..., kind=js::frontend::Statement) at js/src/frontend/Parser.cpp:3168
#9  0x00000000004dab03 in js::frontend::Parser<js::frontend::FullParseHandler>::standaloneLazyFunction (this=this@entry=0x7fffffffcd40, fun=fun@entry=..., strict=<optimized out>, generatorKind=js::NotGenerator) at js/src/frontend/Parser.cpp:3111
#10 0x0000000000bdf1d9 in js::frontend::CompileLazyFunction (cx=cx@entry=0x7ffff6907000, lazy=lazy@entry=..., chars=<optimized out>, length=8) at js/src/frontend/BytecodeCompiler.cpp:799
#11 0x00000000008ebb57 in JSFunction::createScriptForLazilyInterpretedFunction (cx=0x7ffff6907000, fun=fun@entry=...) at js/src/jsfun.cpp:1479
#12 0x000000000049cb01 in JSFunction::getOrCreateScript (this=<optimized out>, cx=<optimized out>) at js/src/jsfun.h:413
#13 0x0000000000aa741b in js::Invoke (cx=cx@entry=0x7ffff6907000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:478
#14 0x0000000000aa7f1c in js::Invoke (cx=cx@entry=0x7ffff6907000, thisv=..., fval=..., argc=argc@entry=0, argv=argv@entry=0x0, rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:527
#15 0x00000000008a2a54 in JS_CallFunction (cx=cx@entry=0x7ffff6907000, obj=..., fun=..., fun@entry=..., args=..., rval=..., rval@entry=...) at js/src/jsapi.cpp:2856
#16 0x0000000000a33d59 in OOMTest (cx=0x7ffff6907000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/TestingFunctions.cpp:1208
[...]
#29 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7058
rax	0x0	0
rbx	0x1	1
rcx	0x1	1
rdx	0x7ffff69250b8	140737330172088
rsi	0x1	1
rdi	0x7ffff69250b0	140737330172080
rbp	0x7fffffffbf90	140737488338832
rsp	0x7fffffffbf70	140737488338800
r8	0x1	1
r9	0x271	625
r10	0x7ffff3c89e9c	140737283399324
r11	0x0	0
r12	0x2	2
r13	0x1c2cc0c	29543436
r14	0x1c2cc10	29543440
r15	0x1	1
rip	0xd20c4e <Balloc(DtoaState*, int)+110>
=> 0xd20c4e <Balloc(DtoaState*, int)+110>:	mov    %ebx,0x8(%rax)
   0xd20c51 <Balloc(DtoaState*, int)+113>:	mov    %r12d,0xc(%rax)

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160210154833" and the hash "b1a7043faedd2d1562c22fe8ad10b6356383c5f7".
The "bad" changeset has the timestamp "20160210181434" and the hash "579b314c61d300698a75e241d00c1c15facd2b56".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b1a7043faedd2d1562c22fe8ad10b6356383c5f7&tochange=579b314c61d300698a75e241d00c1c15facd2b56

Comment 2

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

Guessing this might be related to bug 1245737 as per comment 1. Nick, is this possible?

Comment 3

[Nicholas Nethercote [inactive]]

Definitely possible. I'll take a look on Monday.

Comment 4

[Nicholas Nethercote [inactive]]

The diagnosis is simple. Until recently, dtoa.c pre-allocated a ~2 KiB chunk of memory that in practice was enough for any conversions that occur in JS code. Bug 1245737 changed that so that dtoa.c instead allocated memory on demand. I made this change because in practice the amount of memory needed on demand is measured in the 10s of bytes.

However, this change isn't OOM-safe. The chunk pre-allocation occurred in newdtoa(), which checks for allocation failure. The on-demand allocations happen in Balloc(), which itself calls a malloc-like function provided by the embedding, and there is no error-handling if any of the Balloc() calls in dtoa.c fail.

Here are the docs for the malloc you have to provide:

> * #define MALLOC your_malloc, where your_malloc(n) acts like malloc(n)
> *      if memory is available and otherwise does something you deem
> *      appropriate.  If MALLOC is undefined, malloc will be invoked
> *      directly -- and assumed always to succeed.

The malloc we provide is dtoa_malloc(), which is a trivial wrapper for js_malloc() that does not check for failure.

The easy fix here is to simply abort. The allocation sizes here are tiny, measured in dozens of bytes, so if we truly OOM then things are in a dire state.

But that goes against the usual SpiderMonkey OOM-handling approach. The alternative is to add error-handling paths to dtoa.c, which doesn't look like much fun at all :(

It's worth noting here also that js_strtod_harder() has an |err| parameter that allows for errors to be reported, but it's always set to zero. Which is probably a good thing because not all the call sites actually check |err|. If we go with the simple infallible alloc approach then we can just get rid of that parameter.

Comment 5

[Nicholas Nethercote [inactive]]

BTW, it's nice that the fuzzers check for OOMs in this way!

Comment 6

[Nicholas Nethercote [inactive]]

jorendorff: do you think making dtoa_malloc() infallible is reasonable?

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> But that goes against the usual SpiderMonkey OOM-handling approach. The
> alternative is to add error-handling paths to dtoa.c, which doesn't look
> like much fun at all :(

For small/unlikely OOMs and/or when OOM handling is really complicated, we sometimes crash on OOM these days. It's important to use AutoEnterOOMUnsafeRegion though, because the fuzzers know to ignore those crashes:

See for instance https://dxr.mozilla.org/mozilla-central/rev/e355cacefc881ba360d412853b57e8e060e966f4/js/src/gc/Marking.cpp#825-827

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> 
> For small/unlikely OOMs and/or when OOM handling is really complicated, we
> sometimes crash on OOM these days. It's important to use
> AutoEnterOOMUnsafeRegion though, because the fuzzers know to ignore those
> crashes:

Thank you, that's very helpful. I'll put up a patch using AutoEnterOOMUnsafeRegion tomorrow.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Make dtoa_malloc infallible

I'm happy to hear suggestions for a better location for the test.

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8719597 [details] [diff] [review]
Make dtoa_malloc infallible

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

Yeah, there ought to be a better place, but it doesn't seem worth worrying over.

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e98b77de3d8f0efc8444298126fee719529e25ed
Bug 1248101 - Make dtoa_malloc infallible. r=sfink.

Comment 12

[Carsten Book [:Tomcat]]

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