Closed
Bug 1220915
Opened 9 years ago
Closed 9 years ago
Crash [@ js::CompartmentChecker::check]
Categories
(Core :: JavaScript Engine: JIT, defect)
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)
Reporter | ||
Comment 1•9 years ago
|
||
(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)
Reporter | ||
Comment 2•9 years ago
|
||
(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)
Reporter | ||
Comment 3•9 years ago
|
||
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.
Reporter | ||
Updated•9 years ago
|
status-firefox44:
affected → ---
status-firefox45:
--- → affected
Comment 4•9 years ago
|
||
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
Comment 5•9 years ago
|
||
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?
Comment 6•9 years ago
|
||
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.
Comment 7•9 years ago
|
||
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; > }
Comment 8•9 years ago
|
||
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).
Reporter | ||
Comment 9•9 years ago
|
||
(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.
Comment 10•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
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.
Comment 12•9 years ago
|
||
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);
Comment 13•9 years ago
|
||
The duplicated bug is Bug 939157.
Comment 14•9 years ago
|
||
Oops, I missed that comment about bug 939157. I've now filed bug bug 1221299 to track this same issue.
Reporter | ||
Comment 15•9 years ago
|
||
(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.
Reporter | ||
Comment 16•9 years ago
|
||
I confirm that the patch in bug 939157 comment 1 fixes this issue.
Comment 17•9 years ago
|
||
(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.)
Reporter | ||
Comment 18•9 years ago
|
||
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.
Description
•