901518 - ARM: Assertion failure: Tag == MEM, at ../ion/arm/Assembler-arm.h:1102 or Crash on Heap with possibly invalid free

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 482b9d04974a (run with --fuzzing-safe --ion-eager):


function foo() {
  var n = 4294967295;
  for (var i = 0; i < 20; i++)
    n = null & "arguments";
  n == 29;
}
foo();

Comment 1

[Christian Holler (:decoder)]

Crash trace from an opt build:

Program received signal SIGSEGV, Segmentation fault.
0xb6ccd718 in ?? ()
(gdb) bt
#0  0xb6ccd718 in ?? ()
#1  0x001116e8 in js_free (p=<optimized out>) at ./dist/include/js/Utility.h:168
#2  release (ptr=<optimized out>) at ./dist/include/js/Utility.h:377
#3  ~Scoped (this=0xbeffe58c, __in_chrg=<optimized out>) at ./dist/include/mozilla/Scoped.h:93
#4  ~ScopedJSFreePtr (this=0xbeffe58c, __in_chrg=<optimized out>) at ./dist/include/js/Utility.h:379
#5  ~ScopedThreadSafeStringInspector (this=0xbeffe588, __in_chrg=<optimized out>) at ../vm/String.h:821
#6  js::StringToNumber (cx=0xbeffe580, str=0xffffff81, result=0xffffff82) at js/src/jsnum.cpp:1452
#7  0xb6ccd00c in ?? ()
#8  0xb6ccd00c in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) x /i $pc
=> 0xb6ccd718:  vstr    d0, [r12]
(gdb) info reg r12
r12            0xe00000ec       3758096620


S-s and sec-critical due to crash with possibly invalid free. Needinfo from Marty :)

Comment 2

[Marty Rosenberg [:mjrosenb]]

diff --git a/js/src/ion/arm/MacroAssembler-arm.h b/js/src/ion/arm/MacroAssembler-arm.h
--- a/js/src/ion/arm/MacroAssembler-arm.h
+++ b/js/src/ion/arm/MacroAssembler-arm.h
@@ -1194,17 +1194,17 @@ class MacroAssemblerARMCompat : public M
     }
     void storeDouble(FloatRegister src, BaseIndex addr) {
         // Harder cases not handled yet.
         JS_ASSERT(addr.offset == 0);
         uint32_t scale = Imm32::ShiftOf(addr.scale).value;
         ma_vstr(src, addr.base, addr.index, scale);
     }
     void moveDouble(FloatRegister src, FloatRegister dest) {
-        ma_vstr(src, Operand(dest));
+        ma_vmov(src, dest);
     }

     void storeFloat(FloatRegister src, Address addr) {
         ma_vstr(VFPRegister(src).singleOverlay(), Operand(addr));
     }
     void storeFloat(FloatRegister src, BaseIndex addr) {
         // Harder cases not handled yet.
         JS_ASSERT(addr.offset == 0);

I've verified that this patch makes the assertion go away, but based on the names of the functions on the call stack, I'm not sure this is the correct fix.

Comment 3

[Shu-yu Guo [:shu]]

I don't really understand the backtrace. Why are we jumping from free into ??

I'm not sure I have anything productive to add - I don't know enough about ARM to not cargo cult additions to the ARM masm. It's very plausible I got things wrong.

Comment 4

[Marty Rosenberg [:mjrosenb]]

#0  0x003791f8 in js::ion::Operand::disp (this=0x7effe744) at ../../src/ion/arm/Assembler-arm.h:1102
#1  0x0056c030 in js::ion::MacroAssemblerARM::ma_vdtr (this=0x849f10, ls=js::ion::IsStore, addr=..., rt=..., cc=js::ion::Assembler::Always)
    at /home/mjrosenb/src/central/central/js/src/ion/arm/MacroAssembler-arm.cpp:1434
#2  0x0056c508 in js::ion::MacroAssemblerARM::ma_vstr (this=0x849f10, src=..., addr=..., cc=js::ion::Assembler::Always)
    at /home/mjrosenb/src/central/central/js/src/ion/arm/MacroAssembler-arm.cpp:1504
#3  0x003fe620 in js::ion::MacroAssemblerARMCompat::moveDouble (this=0x849f10, src=..., dest=...) at ../../src/ion/arm/MacroAssembler-arm.h:1202
#4  0x003feab8 in js::ion::MacroAssembler::storeCallFloatResult (this=0x849f10, reg=...) at ../../src/ion/IonMacroAssembler.h:298
#5  0x0040c0f2 in js::ion::CodeGeneratorShared::storeFloatResultTo (this=0x849ed8, reg=...) at ../../src/ion/shared/CodeGenerator-shared.h:316
#6  0x0040c330 in js::ion::StoreFloatRegisterTo::generate (this=0x83501c, codegen=0x849ed8) at ../../src/ion/shared/CodeGenerator-shared.h:551
#7  0x00437d92 in js::ion::CodeGeneratorShared::visitOutOfLineCallVM<js::ion::ArgSeq<js::ion::ArgSeq<void, void>, js::ion::Register>, js::ion::StoreFloatRegisterTo> (this=0x849ed8, ool=0x834ff8) at ../../src/ion/shared/CodeGenerator-shared.h:636
#8  0x00437a86 in js::ion::OutOfLineCallVM<js::ion::ArgSeq<js::ion::ArgSeq<void, void>, js::ion::Register>, js::ion::StoreFloatRegisterTo>::accept (
    this=0x834ff8, codegen=0x849ed8) at ../../src/ion/shared/CodeGenerator-shared.h:606
#9  0x00437a46 in js::ion::OutOfLineCodeBase<js::ion::CodeGeneratorShared>::generate (this=0x834ff8, codegen=0x849ed8)
    at ../../src/ion/shared/CodeGenerator-shared.h:434
#10 0x00607a74 in js::ion::CodeGeneratorShared::generateOutOfLineCode (this=0x849ed8)
    at /home/mjrosenb/src/central/central/js/src/ion/shared/CodeGenerator-shared.cpp:99
#11 0x00559d14 in js::ion::CodeGeneratorARM::generateOutOfLineCode (this=0x849ed8)
    at /home/mjrosenb/src/central/central/js/src/ion/arm/CodeGenerator-arm.cpp:154
#12 0x0041fb18 in js::ion::CodeGenerator::generate (this=0x849ed8) at /home/mjrosenb/src/central/central/js/src/ion/CodeGenerator.cpp:5446
#13 0x00440a18 in js::ion::GenerateCode (mir=0x82ff58, lir=0x832960, maybeMasm=0x0) at /home/mjrosenb/src/central/central/js/src/ion/Ion.cpp:1267
#14 0x00440a7c in js::ion::CompileBackEnd (mir=0x82ff58, maybeMasm=0x0) at /home/mjrosenb/src/central/central/js/src/ion/Ion.cpp:1286
#15 0x00440ede in js::ion::IonCompile (cx=0x7a9e68, script=0x7692a180, baselineFrame=0x0, osrPc=0x0, constructing=false, 
    executionMode=js::ion::SequentialExecution) at /home/mjrosenb/src/central/central/js/src/ion/Ion.cpp:1449
#16 0x004415fc in js::ion::Compile (cx=0x7a9e68, script=..., osrFrame=0x0, osrPc=0x0, constructing=false, executionMode=js::ion::SequentialExecution)
    at /home/mjrosenb/src/central/central/js/src/ion/Ion.cpp:1607
#17 0x00441c4e in js::ion::CanEnter (cx=0x7a9e68, state=...) at /home/mjrosenb/src/central/central/js/src/ion/Ion.cpp:1732
#18 0x0009d522 in js::RunScript (cx=0x7a9e68, state=...) at /home/mjrosenb/src/central/central/js/src/vm/Interpreter.cpp:418
#19 0x0009d9e8 in js::Invoke (cx=0x7a9e68, args=..., construct=js::NO_CONSTRUCT) at /home/mjrosenb/src/central/central/js/src/vm/Interpreter.cpp:505
#20 0x0009dbee in js::Invoke (cx=0x7a9e68, thisv=..., fval=..., argc=0, argv=0x7effef00, rval=...)
    at /home/mjrosenb/src/central/central/js/src/vm/Interpreter.cpp:536
#21 0x003e3204 in js::ion::DoCallFallback (cx=0x7a9e68, frame=0x7effef20, stub=0x7a8cf0, argc=0, vp=0x7effeef0, res=...)
    at /home/mjrosenb/src/central/central/js/src/ion/BaselineIC.cpp:7051

A more useful backtrace.

Comment 5

[Marty Rosenberg [:mjrosenb]]

Attachment: doubleMemory-r0.patch

Looks like we just tried to call store when we wanted move.

Comment 6

[Marty Rosenberg [:mjrosenb]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/e19118c912a1

Comment 7

[Marty Rosenberg [:mjrosenb]]

Based on when shu's patch landed, this most likely affects aurora.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Alex Keybl [:akeybl]]

Unclear which branches this impacts. Let's figure that out.

Comment 10

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

(In reply to Marty Rosenberg [:mjrosenb] from comment #7)
> Based on when shu's patch landed, this most likely affects aurora.

Which bug is the regressing bug?

Comment 11

[Marty Rosenberg [:mjrosenb]]

changeset:   141148:699c21af878d
user:        Shu-yu Guo <[e-mail]>
date:        Fri Aug 02 08:24:57 2013 -0700
summary:     Bug 895950 - Support calling VM functions with double out parameters. (r=nbp)

Comment 12

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

(In reply to Alex Keybl [:akeybl] from comment #9)
> Unclear which branches this impacts. Let's figure that out.

Bug 895950 landed only for Firefox 25, so only aurora is affected.

Comment 13

[Matt Wobensmith [:mwobensmith][:matt:]]

Thank you, Gary!

Comment 14

[Alex Keybl [:akeybl]]

This is ready for an uplift nomination in that case :)

Comment 15

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 785964 [details] [diff] [review]
doubleMemory-r0.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  Bug 895950
User impact if declined: crashes
Testing completed (on m-c, etc.): plenty of burn in on m-c
Risk to taking this patch (and alternatives if risky): this is as close to trivial as is possible.  we could also back out shu's patch, but I think it adds a feature we want.
String or IDL/UUID changes made by this patch:

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fe08b2ee300b