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

RESOLVED FIXED in Firefox 39

Status

()

--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gkw, Assigned: jandem)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla39
x86_64
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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?
Flags: needinfo?(bzbarsky)
(Reporter)

Updated

4 years ago
Whiteboard: [fuzzblocker] → \
(Reporter)

Updated

4 years ago
Whiteboard: \
(Reporter)

Comment 1

4 years ago
Created attachment 8578225 [details]
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)
> 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:

  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?
Group: core-security
Flags: needinfo?(jdemooij)
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
Created attachment 8578503 [details] [diff] [review]
Patch

Only assert if we're not OOM. Not the prettiest patch but straight-forward.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8578503 - Flags: review?(sunfish)
> Then in Linker::newCode we do:

OK.  So this is not a security issue then?
(Assignee)

Updated

4 years ago
Group: core-security

Updated

4 years ago
Attachment #8578503 - Flags: review?(sunfish) → review+
https://hg.mozilla.org/mozilla-central/rev/fd8149e51d07
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.