1236476 - Assertion failure: message, at js/src/jscntxt.cpp:818 with OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

oomTest(() => {
    offThreadCompileScript(`
      "use asm";
      return assertEq;
    `);
    runOffThreadScript();
});



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000008c3e01 in js::CallErrorReporter (cx=<optimized out>, message=<optimized out>, reportp=<optimized out>) at js/src/jscntxt.cpp:818
#0  0x00000000008c3e01 in js::CallErrorReporter (cx=<optimized out>, message=<optimized out>, reportp=<optimized out>) at js/src/jscntxt.cpp:818
#1  0x0000000000bdae63 in js::frontend::CompileError::throwError (this=<optimized out>, cx=<optimized out>) at js/src/frontend/TokenStream.cpp:583
#2  0x0000000000a36a84 in js::GlobalHelperThreadState::finishParseTask (this=<optimized out>, maybecx=maybecx@entry=0x7ffff6907800, rt=rt@entry=0x7ffff695d000, token=token@entry=0x7ffff31d5ac0) at js/src/vm/HelperThreads.cpp:1064
#3  0x00000000008c4cde in JS::FinishOffThreadScript (maybecx=maybecx@entry=0x7ffff6907800, rt=rt@entry=0x7ffff695d000, token=token@entry=0x7ffff31d5ac0) at js/src/jsapi.cpp:4111
#4  0x0000000000488773 in runOffThreadScript (cx=0x7ffff6907800, argc=<optimized out>, vp=0x7fffffffc338) at js/src/shell/js.cpp:3670
#5  0x00007ffff7ff7d28 in ?? ()
#6  0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff6907800	140737330051072
rcx	0x7ffff6ca53cd	140737333842893
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffc150	140737488339280
rsp	0x7fffffffc150	140737488339280
r8	0x7ffff7fe0780	140737354008448
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffffbf10	140737488338704
r11	0x7ffff6c27960	140737333328224
r12	0x1	1
r13	0x7fffffffc1b0	140737488339376
r14	0x7fffffffc1d0	140737488339408
r15	0x7ffff6907808	140737330051080
rip	0x8c3e01 <js::CallErrorReporter(JSContext*, char const*, JSErrorReport*)+97>
=> 0x8c3e01 <js::CallErrorReporter(JSContext*, char const*, JSErrorReport*)+97>:	movl   $0x332,0x0
   0x8c3e0c <js::CallErrorReporter(JSContext*, char const*, JSErrorReport*)+108>:	callq  0x4a4a90 <abort()>

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Not specific to asm.js, I think? 

- when encountering an error while parsing off main thread, we call cx->addPendingException, which is allocating a new CompileError https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/TokenStream.cpp#620
- then we expand the error message in this call to ExpandErrorArgumentsVA https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/TokenStream.cpp#650
- at the end of ExpandErrorArgumentsVA, if there's a memory allocation failure, we explicitly free the error "message" variable, which is what's happening here
- later, when calling CallErrorReporter with this CompileError's message, it asserts because the message is empty

Should the ParseTask have a flag outOfMemory (there is overRecursed already)? Or should we just ReportOutOfMemory if there's no message allocated? The former sounds more general...

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

cc'ing people who might know what to do here (see comment 1 for analysis)

Comment 3

[Luke Wagner [:luke]]

Agreed this seems unrelated to asm.js.  This looks related to bug 1232676 where the AUtoEnterOOMUnsafeRegion was added to ExclusiveContext::addPendingCompileError().  Jan: do you know why we have to crash here instead of simply making addPendingCompileError() fallible as bbouvier suggested?  Also, ideally I think we'd only add finished CompileErrors to the cx instead of adding them before they are initialized.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #3)
> This looks related to bug 1232676
> where the AUtoEnterOOMUnsafeRegion was added to
> ExclusiveContext::addPendingCompileError().  Jan: do you know why we have to
> crash here instead of simply making addPendingCompileError() fallible as
> bbouvier suggested?

There used to be a MOZ_CRASH there, I just replaced it with AutoEnterOOMUnsafeRegion because the fuzzers know how to handle those. Making addPendingCompileError fallible is fine, I think.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

The issue is actually not in addPendingCompileError: it is that ExpandErrorArgumentsVA fails with oom but never reports it, so the crafted error is malformed. Patch incoming.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: MozReview Request: Bug 1236476: Report out of memory in ExpandErrorArgumentsVA; r=jandem

Review commit: https://reviewboard.mozilla.org/r/29569/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29569/

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Previous patch adds an outOfMemory flag to the parse task, that is set the same way that the overrecursed flag.

I've tried in another patch to make ExpandErrorArgumentsVA less of a mess by using UniquePtrs, but it's a mess as in one case, you'd have to have a vector of UniquePtrs...

Comment 8

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/de72e2291ae8
user:        Jan de Mooij
date:        Wed Dec 09 22:55:50 2015 -0500
summary:     Bug 1225396 part 3 - Make %GeneratorPrototype% inherit from %IteratorPrototype%. r=jorendorff

This iteration took 0.459 seconds to run.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8704103 [details]
MozReview Request: Bug 1236476: Report out of memory in ExpandErrorArgumentsVA; r=jandem

https://reviewboard.mozilla.org/r/29569/#review26373

Thanks for fixing this! I've one question below, let me know what you think.

::: js/src/jscntxt.cpp:602
(Diff revision 1)
> +                    ReportOutOfMemory(cx);

The cx->pod_malloc call is supposed to report OOM on the context when it fails. That's not happening here because ExclusiveContext::onOutOfMemory does:

    if (!isJSContext())
        return nullptr;
    return runtime_->onOutOfMemory(allocFunc, nbytes, reallocPtr, asJSContext());

JSRuntime::onOutOfMemory will call ReportOutOfMemory, if we have a JSContext. It seems wrong to call ReportOutOfMemory here as well.

Should we, instead of reporting OOM here, change ExclusiveContext::onOutOfMemory to do addPendingOutOfMemory()?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> Should we, instead of reporting OOM here, change
> ExclusiveContext::onOutOfMemory to do addPendingOutOfMemory()?

It's a bit tricky because some callers of ExclusiveContext::pod_malloc and friends may call recoverFromOutOfMemory, or rely on these calls not reporting anything if it's an ExclusiveContext and handle the nullptr case.

I think it makes sense to do this though, and to also handle the ExclusiveContext case in ExclusiveContext::recoverFromOutOfMemory...

Comment 11

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

Assuming related to bug 1225396 as per comment 8.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8704103 [details]
MozReview Request: Bug 1236476: Report out of memory in ExpandErrorArgumentsVA; r=jandem

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29569/diff/1-2/

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jan de Mooij [:jandem] from comment #10)
> (In reply to Jan de Mooij [:jandem] from comment #9)
> > Should we, instead of reporting OOM here, change
> > ExclusiveContext::onOutOfMemory to do addPendingOutOfMemory()?
> 
> It's a bit tricky because some callers of ExclusiveContext::pod_malloc and
> friends may call recoverFromOutOfMemory, or rely on these calls not
> reporting anything if it's an ExclusiveContext and handle the nullptr case.
> 
> I think it makes sense to do this though, and to also handle the
> ExclusiveContext case in ExclusiveContext::recoverFromOutOfMemory...

Agreed: updated the patch to do so. I will send the patch to try to make sure we don't have something strange showing up.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8704103 [details]
MozReview Request: Bug 1236476: Report out of memory in ExpandErrorArgumentsVA; r=jandem

https://reviewboard.mozilla.org/r/29569/#review26941

Thanks!

::: js/src/jscntxt.cpp:949
(Diff revision 2)
> +            task->outOfMemory = false;

Can we do `task->outOfMemory = false` unconditionally?

::: js/src/vm/HelperThreads.cpp:1355
(Diff revision 2)
> -    if (!error || !helperThread()->parseTask()->errors.append(error))
> +           helperThread()->parseTask()->errors.append(*error);

I think this will leak the CompileError if the append() fails.

Also, can we use cx->new_ (to make sure the OOM is reported!) and return a pointer?

```
frontend::CompileError* addPendingCompileError() {
    frontend::CompileError* error = new_<frontend::CompileError>();
    if (!error)
        return nullptr;
    if (!errors.append(*error)) {
        js_delete(error);
        return nullptr;
    }
    return error;
}
```

Bonus points for using UniquePtr :)

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8704103 [details]
MozReview Request: Bug 1236476: Report out of memory in ExpandErrorArgumentsVA; r=jandem

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29569/diff/2-3/

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/115da8bc01ad

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/115da8bc01ad