Closed Bug 1143847 Opened 5 years ago Closed 5 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox39 --- fixed


(Reporter: gkw, Assigned: jandem)



(Keywords: assertion, regression, testcase)


(2 files)


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/ -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:
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?
Flags: needinfo?(bzbarsky)
Whiteboard: [fuzzblocker] → \
Whiteboard: \
Attached file 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 = '', 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
> 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...
Flags: needinfo?(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:


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?
Group: core-security
Flags: needinfo?(jdemooij)
(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.
Attached patch PatchSplinter Review
Only assert if we're not OOM. Not the prettiest patch but straight-forward.
Assignee: nobody → jdemooij
Flags: needinfo?(jdemooij)
Attachment #8578503 - Flags: review?(sunfish)
> Then in Linker::newCode we do:

OK.  So this is not a security issue then?
Group: core-security
Attachment #8578503 - Flags: review?(sunfish) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.