1143847 - Assertion failure: masm.size() - oldSize == 8, at jit/x64/Assembler-x64.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Array.prototype;
oomAfterAllocations(11);
Array.prototype.push.call()

asserts js debug shell on m-c changeset 436686833af0 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: masm.size() - oldSize == 8, at jit/x64/Assembler-x64.cpp.

Configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 436686833af0

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a2f5fa870c8a
user:        Boris Zbarsky
date:        Fri Jul 04 01:24:54 2014 -0400
summary:     Bug 966452 part 1.  Refactor the js_ReportUncaughtException to produce a (message, JSErrorReport*) pair before reporting.  r=waldo and including the fix for bug 1034616 to fix JS tests to deal with this, r=jorendorff.  r=terrence on the AutoStableStringChars bits

Boris, is bug 966452 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x84042, 0x000000010071d99d js-dbg-64-dm-nsprBuild-darwin-436686833af0`js::jit::Assembler::finish(this=<unavailable>) + 893 at Assembler-x64.cpp:211, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010071d99d js-dbg-64-dm-nsprBuild-darwin-436686833af0`js::jit::Assembler::finish(this=<unavailable>) + 893 at Assembler-x64.cpp:211
    frame #1: 0x00000001004334a9 js-dbg-64-dm-nsprBuild-darwin-436686833af0`js::jit::ICStubCompiler::getStubCode() [inlined] js::jit::Linker::Linker(this=0x00007fff5fbfdaf8, masm=0x00007fff5fbfdb18) + 249 at Linker.h:35
    frame #2: 0x000000010043349a js-dbg-64-dm-nsprBuild-darwin-436686833af0`js::jit::ICStubCompiler::getStubCode() [inlined] js::jit::Linker::Linker(this=0x00007fff5fbfdaf8, masm=0x00007fff5fbfdb18) at Linker.h:36
    frame #3: 0x000000010043349a js-dbg-64-dm-nsprBuild-darwin-436686833af0`js::jit::ICStubCompiler::getStubCode(this=0x00007fff5fbfe6e0) + 234 at BaselineIC.cpp:659
    frame #4: 0x0000000100453199 js-dbg-64-dm-nsprBuild-darwin-436686833af0`js::jit::ICGetPropNativeCompiler::getStub(this=0x00007fff5fbfe6e0, space=0x0000000101c3d450) + 345 at BaselineIC.cpp:7159
(lldb)

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Boris, is bug 966452 a likely regressor?

It's possible, since it changed allocation patterns a bit on exception.  As in, the bug might have been preexisting but need a different arg to oomAfterAllocations...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Fwiw, masm.size() == 2, and oldSize == 240.  

We're doing js::jit::TryAttachNativeGetPropStub where the name is "call".

We're in Assembler::finish, we do this:

  masm.ud2();

and this clears the masm buffer due to oom, with this stack:

#0  mozilla::VectorBase<unsigned char, 256ul, js::SystemAllocPolicy, mozilla::Vector<unsigned char, 256ul, js::SystemAllocPolicy> >::clear (this=0x7fff5fbfd050) at Vector.h:995
#1  0x00000001009e63ec in js::jit::AssemblerBuffer::oomDetected (this=0x7fff5fbfd050) at AssemblerBuffer-x86-shared.h:172
#2  0x00000001009e59cb in js::jit::AssemblerBuffer::ensureSpace (this=0x7fff5fbfd050, space=16) at AssemblerBuffer-x86-shared.h:80
#3  0x0000000100a32dc9 in js::jit::X86Encoding::BaseAssembler::X86InstructionFormatter::twoByteOp (this=0x7fff5fbfd050, opcode=js::jit::X86Encoding::OP2_UD2) at BaseAssembler-x86-shared.h:4520
#4  0x00000001009bdadd in js::jit::X86Encoding::BaseAssembler::ud2 (this=0x7fff5fbfd048) at BaseAssembler-x86-shared.h:3591
#5  0x00000001009768c2 in js::jit::Assembler::finish (this=0x7fff5fbfcd48) at Assembler-x64.cpp:210

Then twoByteOp() presses on after its ensureSpace() call failed, pushes two more bytes into the buffer, we unwind, masm.oom() is now true, but nothing checks it and instead we fail the assert.

This seems pretty bad to me: we're silently spitting out bogus code, no?

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Not doing reviews right now from comment #3)
> This seems pretty bad to me: we're silently spitting out bogus code, no?

We call masm.finish() in the Linker constructor. Then in Linker::newCode we do:

  if (masm.oom())
      return fail(cx);

I think this is just a bogus assert, we should probably change it to MOZ_ASSERT_IF(!oom(), ...); Annoying, but better than requiring OOM checks for every masm method.

Will try to repro.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

Only assert if we're not OOM. Not the prettiest patch but straight-forward.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Then in Linker::newCode we do:

OK.  So this is not a security issue then?

Comment 7

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd8149e51d07

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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