Crash [@ js::jit::Assembler::bind] involving oomAfterAllocations

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine: JIT
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gkw, Assigned: jolesen)

Tracking

(Blocks: 2 bugs, {crash, regression, testcase})

Trunk
mozilla44
x86_64
Mac OS X
crash, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 affected, firefox44 fixed)

Details

(Whiteboard: [jsbugmon:], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
function f(code) {
    g(code)
}
function g(code) {
    eval(code)
    h()
}
function h() {
    dis.z
}
f()
f()
f("oomAfterAllocations(2)")

crashes js debug 32-bit ARM-simulator shell on m-c changeset aa5f8d47a0ba with --fuzzing-safe --no-threads --ion-eager at js::jit::Assembler::bind.

Debug configure options:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-arm-simulator --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect is running.
Flags: needinfo?
(Reporter)

Comment 1

3 years ago
Created attachment 8560800 [details]
stack

(lldb) bt 5
* thread #1: tid = 0xa69a2, 0x00602b63 js-dbg-32-dm-nsprBuild-armSim-darwin-aa5f8d47a0ba`js::jit::Assembler::bind(js::jit::RepatchLabel*) [inlined] js::jit::Instruction::encode(this=0x0000000c) const at Assembler-arm.h:1840, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xc)
  * frame #0: 0x00602b63 js-dbg-32-dm-nsprBuild-armSim-darwin-aa5f8d47a0ba`js::jit::Assembler::bind(js::jit::RepatchLabel*) [inlined] js::jit::Instruction::encode(this=0x0000000c) const at Assembler-arm.h:1840
    frame #1: 0x00602b63 js-dbg-32-dm-nsprBuild-armSim-darwin-aa5f8d47a0ba`js::jit::Assembler::bind(this=0x0066b1ee, label=<unavailable>) + 99 at Assembler-arm.cpp:2522
    frame #2: 0x004b93c7 js-dbg-32-dm-nsprBuild-armSim-darwin-aa5f8d47a0ba`GenerateReadSlot(JSContext*, js::jit::IonScript*, js::jit::MacroAssembler&, js::jit::IonCache::StubAttacher&, JSObject*, js::NativeObject*, js::Shape*, js::jit::Register, js::jit::TypedOrValueRegister, js::jit::Label*) [inlined] js::jit::IonCache::StubAttacher::jumpRejoin(js::jit::MacroAssembler&) + 1447 at IonCaches.cpp:214
    frame #3: 0x004b9397 js-dbg-32-dm-nsprBuild-armSim-darwin-aa5f8d47a0ba`GenerateReadSlot(cx=<unavailable>, ion=<unavailable>, masm=<unavailable>, attacher=<unavailable>, obj=<unavailable>, holder=<unavailable>, shape=<unavailable>, object=<unavailable>, output=<unavailable>, failures=<unavailable>) + 1399 at IonCaches.cpp:874
    frame #4: 0x004b8141 js-dbg-32-dm-nsprBuild-armSim-darwin-aa5f8d47a0ba`js::jit::GetPropertyIC::tryAttachNative(JSContext*, JS::Handle<JSScript*>, js::jit::IonScript*, JS::Handle<JSObject*>, JS::Handle<js::PropertyName*>, void*, bool*) [inlined] js::jit::GetPropertyIC::object(this=0x01a0fad0, this=0x01a275b0, this=0x01a275b0) const + 929 at IonCaches.h:590
(lldb)
Flags: needinfo?
(Reporter)

Comment 2

3 years ago
autoBisect didn't turn up anything useful (it was crashing/asserting at various places in the past), so setting needinfo? from Jan and/or Marty to help move this forward.
Flags: needinfo?(mrosenberg)
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Marty can you look at this? Based on comment 0 I assume it's ARM-only.
Flags: needinfo?(jdemooij)
(Reporter)

Updated

3 years ago
Summary: Crash [@ js::jit::Assembler::bind] → Crash [@ js::jit::Assembler::bind] involving oomAfterAllocations
(Reporter)

Comment 5

2 years ago
Bouncing back to Jan - Marty is no longer paid staff with Mozilla.
Flags: needinfo?(marty.rosenberg) → needinfo?(jdemooij)
(Reporter)

Comment 6

2 years ago
This still occurs as of m-c rev caf25344f73e.
Hm this is an OOM in the ARM assembler buffer :( Doug, do you know how this is supposed to work?
Flags: needinfo?(jdemooij) → needinfo?(dtc-moz)
(In reply to Jan de Mooij [:jandem] from comment #7)
> Hm this is an OOM in the ARM assembler buffer :( Doug, do you know how this
> is supposed to work?

The ARM backend pool allocation consistency check failures were corrected and it no longer flags an oom due to these. The test case causes an allocation failure in initWithAllocator() and then later seems so be picking up a bad pointer dealing with labels. There is a comment at the allocation site "We need to wait until an AutoJitContextAlloc is created by the MacroAssembler before allocating any space.".

There is an oom flag in the assembler and this is set in many cases upon an allocation failure, and execution continues without the allocation. This looks like a foot-gun. Every use of these objects would need to be protected, and internal consistency checks adapted for the oom cases. Code dealing with labels seems to be a recurring problem for the ARM backend and OOMs. Dealing with these by throwing an exception seems more appropriate, or checking a 'success' flag and exiting the path on a failure.

There have been other OOM bugs addressed by simply crashing more gracefully when the allocation fails, see 'CrashAtUnhandlableOOM()', but this seems contrary to the strategy of flagging the OOM and continuing. If this is acceptable then perhaps this could be used to workaround this current issue too but calling this from initWithAllocator(). Or perhaps initWithAllocator() could return a 'success' flag and the caller could abort compilation if there is a failure. Perhaps the oom flag could be removed entirely and CrashAtUnhandlableOOM could be called on all these unhandled OOM cases?
Flags: needinfo?(dtc-moz) → needinfo?(jdemooij)
Blocks: 912928
Gary, do you have a newer test for this? The one in comment 0 doesn't repro anymore..
Flags: needinfo?(jdemooij) → needinfo?(gary)
(Reporter)

Comment 10

2 years ago
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/6ffa14c65354
user:        Jon Coppeard
date:        Fri May 22 18:52:38 2015 +0100
summary:     Bug 1155618 - Add better support for testing OOM behaviour r=terrence

Bug 1155618 probably caused the issue to go away. I guess we can mark this WFM and file a new bug should the issue occur again.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(gary)
Resolution: --- → WORKSFORME
This issue is still active, just replace the oomAfterAllocations argument by 3 or 4 instead of 2 to reproduce the issue again.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Reporter)

Comment 12

2 years ago
Thanks :decoder! Re-setting needinfo? for Jan.
Flags: needinfo?(jdemooij)
(Assignee)

Updated

2 years ago
Assignee: nobody → jolesen
(Assignee)

Comment 13

2 years ago
Created attachment 8670257 [details] [diff] [review]
ARM assembler: Check oom() before using editSrc().

The editSrc() function returns a pointer into the assembly buffer. This
may not be a valid pointer if the assembly buffer ran out of memory.

Harden AssemblerBuffer::getInst() by adding assertions. Since invalid
uses of getInst() may be security bugs, use MOZ_RELEASE_ASSERT for the
range check.
Attachment #8670257 - Flags: review?(jdemooij)
(Assignee)

Comment 14

2 years ago
The included test case is not good, and should be omitted.

- dis is not available in a release build, and js dies with a reference error.
- The magic number in oomAfterAllocations(3) makes for a fragile test.
Comment on attachment 8670257 [details] [diff] [review]
ARM assembler: Check oom() before using editSrc().

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

::: js/src/jit-test/tests/ion/bug1130672.js
@@ +9,5 @@
> +        dis.z
> +}
> +f()
> +f()
> +f("oomAfterAllocations(3)")

oomAfterAllocations is only defined in debug builds. You could add something like

if (typeof oomAfterAllocations !== 'function')
    quit();

::: js/src/jit/shared/IonAssemblerBuffer.h
@@ +285,5 @@
>          return getInst(off);
>      }
>  
> +    // Get a pointer to the instruction at offset |off| which must be within the bounds of the
> +    // buffer. Use |getInstOrNull()| if |off| may be unassigned.

Nit: comments should fit within 80 columns (for code it's 99 columns).
Attachment #8670257 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 16

2 years ago
Created attachment 8670764 [details] [diff] [review]
ARM assembler: Check oom() before using editSrc().

Updated patch: Removed test case per comment above. Reflowed comments to 80 col.
(Assignee)

Updated

2 years ago
Attachment #8670257 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 17

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8181c8fd433e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8181c8fd433e
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Fixed by Jakob. Thanks!
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.