Closed Bug 1220915 Opened 9 years ago Closed 9 years ago

Crash [@ js::CompartmentChecker::check]

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED DUPLICATE of bug 939157
Tracking Status
firefox45 --- affected

People

(Reporter: gkw, Unassigned)

References

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(2 files)

// Adapted from randomly chosen test: js/src/tests/ecma_6/Generators/return-finally.js
f = function*() {
    try {} finally {
        yield;
    }
}
g = f();
g.next();
g.next();

crashes js ARM-simulator debug shell on m-c changeset 451a18579143 with --fuzzing-safe --no-threads --ion-eager at js::CompartmentChecker::check

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-simulator=arm --disable-debug --disable-threadsafe --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--disable-debug --enable-more-deterministic --32 --enable-simulator=arm" -r 451a18579143

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/00dac1d05d60
user:        Tooru Fujisawa
date:        Sat Oct 17 23:30:20 2015 +0900
summary:     Bug 1202134 - Save return value onto the stack before executing finally block. r=jandem

Jan, is bug 1202134 a likely regressor?

Setting s-s since weird memory address 0xffffff82 seems to be getting accessed, though this seems to only occur for ARM-simulator shells compiled on Mac for now, let us get some eyes on this first.
Flags: needinfo?(jdemooij)
Attached file stack
(lldb) bt 5
* thread #1: tid = 0x4d5497, 0x002da91a js-32-dm-armSim-darwin-451a18579143`js::CompartmentChecker::check(js::AbstractFramePtr) [inlined] JSObject::compartment(this=0xffffff82) const at jsobj.h:159, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xffffff82)
  * frame #0: 0x002da91a js-32-dm-armSim-darwin-451a18579143`js::CompartmentChecker::check(js::AbstractFramePtr) [inlined] JSObject::compartment(this=0xffffff82) const at jsobj.h:159
    frame #1: 0x002da91a js-32-dm-armSim-darwin-451a18579143`js::CompartmentChecker::check(js::AbstractFramePtr) [inlined] js::CompartmentChecker::check(obj=0xffffff82) + 4 at jscntxtinlines.h:81
    frame #2: 0x002da916 js-32-dm-armSim-darwin-451a18579143`js::CompartmentChecker::check(this=0xffffff82, frame=(ptr_ = 40893802)) + 86 at jscntxt.cpp:1215
    frame #3: 0x00454fca js-32-dm-armSim-darwin-451a18579143`js::ScopeIter::ScopeIter(JSContext*, js::AbstractFramePtr, unsigned char*) [inlined] void js::assertSameCompartment<js::AbstractFramePtr>(js::ExclusiveContext*, js::AbstractFramePtr const&) + 298 at jscntxtinlines.h:162
    frame #4: 0x00454f9a js-32-dm-armSim-darwin-451a18579143`js::ScopeIter::ScopeIter(this=0xbfffe3a8, cx=<unavailable>, frame=(ptr_ = 40893802), pc=<unavailable>) + 250 at ScopeObject.cpp:1367
(lldb)
(lldb) dis -p
js-32-dm-armSim-darwin-451a18579143`js::CompartmentChecker::check:
->  0x2da91a <+90>: movl   (%eax), %eax
    0x2da91c <+92>: movl   0x8(%eax), %eax
    0x2da91f <+95>: testl  %eax, %eax
    0x2da921 <+97>: je     0x2da94a                  ; <+138> at jscntxt.cpp:1216
(lldb) register read $eax
     eax = 0xffffff82
(lldb)
I merely reduced the following file while running it with a 32-bit ARM-simulator build on Mac:

http://hg.mozilla.org/mozilla-central/file/451a18579143/js/src/tests/ecma_6/Generators/return-finally.js

This could have been prevented if bug 1155473 was fixed.
it's inside js::jit::HandleException, but the code shouldn't throw expcetion, and if I build it with --enable-debug or --disable-optimize, it doesn't hit the assertion nor the exception.
https://dxr.mozilla.org/mozilla-central/rev/451a185791433bce1a6a894c27f3da60a3119431/js/src/jit/JitFrames.cpp#741
https://dxr.mozilla.org/mozilla-central/rev/451a185791433bce1a6a894c27f3da60a3119431/js/src/jit/JitFrames.cpp#899
in JSOP_RESUME for 2nd next() call, it jumps to noExprStack, and stack is not restored, it seems to be a reason of the exception.

https://dxr.mozilla.org/mozilla-central/rev/9f69202d82752e093a653a8f15b0274e347db33a/js/src/jit/BaselineCompiler.cpp#4005
>     masm.branchTestNull(Assembler::Equal, exprStackSlot, &noExprStack);

GeneratorObject::suspend is called before it, and genObj->setExpressionStack is called with 3-length array, so it shouldn't be null.

https://dxr.mozilla.org/mozilla-central/rev/9f69202d82752e093a653a8f15b0274e347db33a/js/src/vm/GeneratorObject.cpp#84
>        genObj->setExpressionStack(*stack);

maybe generator object is corrupted with some reason, or it's reading from wrong value?
here's the disassembled code for |masm.branchTestNull(Assembler::Equal, exprStackSlot, &noExprStack);|

  0x01627750  e590c034       ldr ip, [r0, #+52]
  0x01627754  e30fcf87       movw ip, #65415
  0x01627758  e34fcfff       movt ip, #65535
  0x0162775c  e15c000c       cmp ip, ip
  0x01627760  0a00004c       beq +312 -> 0x1627898

Looks like register is not allocated correctly.
Keywords: sec-high
something goes wrong with following lines:

https://dxr.mozilla.org/mozilla-central/rev/9f69202d82752e093a653a8f15b0274e347db33a/js/src/jit/arm/MacroAssembler-arm.cpp#338
>     Imm32 negImm = imm;
>     Register negDest;
>     ALUOp negOp = ALUNeg(op, dest, &negImm, &negDest);
>     Imm8 negImm8 = Imm8(negImm.value);

without --disable-optimize (crash case)
  op: 01400000
  dest: 00000010
  imm: ffffff87
  neg: 00000079
  negDest: 00000010
  negOp: 01600000
  negInvalid: 00000001

with --disable-optimize (not-crash case)
  op: 01400000
  dest: 00000010
  imm: ffffff87
  neg: 00000079
  negDest: 00000010
  negOp: 01600000
  negInvalid: 00000000

there, negInvalid is true for crash case, but other values are same.

So, following condition matches only for not-crash case.

https://dxr.mozilla.org/mozilla-central/rev/9f69202d82752e093a653a8f15b0274e347db33a/js/src/jit/arm/MacroAssembler-arm.cpp#346
>     if (negOp != OpInvalid && !negImm8.invalid) {
>         as_alu(negDest, src1, negImm8, negOp, s, c);
>         return;
>     }
maybe we're hitting compiler bug...?

Attached file is disassembled code for Imm8::EncodeImm.

https://dxr.mozilla.org/mozilla-central/rev/9f69202d82752e093a653a8f15b0274e347db33a/js/src/jit/arm/Assembler-arm.h#649
>     static datastore::Imm8mData EncodeImm(uint32_t imm) {
>         // An encodable integer has a maximum of 8 contiguous set bits,
>         // with an optional wrapped left rotation to even bit positions.
>         for (int rot = 0; rot < 16; rot++) {
>             uint32_t rotimm = mozilla::RotateLeft(imm, rot*2);
>             if (rotimm <= 0xFF)
>                 return datastore::Imm8mData(rotimm, rot);
>         }
>         return datastore::Imm8mData();
>     }

There, it should check |rotimm <= 0xFF| with |rotimm == imm| case first (rot == 0), but in attached code, the %edx is rolled before first comparison.

I'm using Xcode 7.0.1, Apple LLVM version 7.0.0 (clang-700.0.72).
(In reply to Tooru Fujisawa [:arai] from comment #8)
> I'm using Xcode 7.0.1, Apple LLVM version 7.0.0 (clang-700.0.72).

I'm on:

$ clang --version
Apple LLVM version 7.0.0 (clang-700.1.76)
Target: x86_64-apple-darwin15.0.0
Thread model: posix

using Xcode 7.1 on Mac OS X 10.11 El Capitan.
This is a bug in mozilla::RotateLeft. It does:

  return (aValue << aShift) | (aValue >> (sizeof(T) * CHAR_BIT - aShift));

When aShift is 0, the right shift shifts by the bit width of the integer, which is undefined behavior. I'll file a new bug for this issue.
Flags: needinfo?(jdemooij)
Thanks, if I modified RotateLeft/RotateRight to return aValue when |aShift == 0|, the crash disappears :)

Another concern is comment #6 here, if the code is the result of the bug in RotateLeft, it doesn't matter, but if not, the generated code path could be hit by another (unknown) case.
ah, but that case is covered by following assertion, so I guess it's safe if everything is passed on debug build.

https://dxr.mozilla.org/mozilla-central/rev/59a6ad6a921f4809dfc37d943d765300c65721e5/js/src/jit/arm/MacroAssembler-arm.cpp#421
>    const Register& scratch = ScratchRegister;
>    MOZ_ASSERT(src1 != scratch);
The duplicated bug is Bug 939157.
Oops, I missed that comment about bug 939157. I've now filed bug bug 1221299 to track this same issue.
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> crashes js ARM-simulator debug shell on m-c changeset 451a18579143 with

Goofed up here. I meant it crashes *opt* shells.
I confirm that the patch in bug 939157 comment 1 fixes this issue.
(In reply to Tooru Fujisawa [:arai] from comment #8)
> Attached file is disassembled code for Imm8::EncodeImm.
> ...
> There, it should check |rotimm <= 0xFF| with |rotimm == imm| case first (rot
> == 0), but in attached code, the %edx is rolled before first comparison.

Great detective work arai! Thanks.

(And sstangl/sunfish/Waldo for fixing this of course.)
Opening up because bug 939157 is open and has a landed patch.
Group: javascript-core-security
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: