909401 - Assertion failure: payload != dest.typeReg(), at ../jit/x86/MacroAssembler-x86.h:178

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 4887845b1142 (threadsafe build, run with --fuzzing-safe --thread-count=2 --ion-compile-try-catch --ion-regalloc=backtracking --ion-eager --ion-check-range-analysis --ion-parallel-compile=on):


var TZ_PST = -8;
var TZ_DIFF = GetTimezoneOffset();
var PST_DIFF = TZ_DIFF - TZ_PST;
function GetTimezoneOffset() {}
function adjustResultArray(ResultArray) {
    var t = ResultArray[TIME] - PST_DIFF;
    ResultArray[UTC_YEAR] = YearFromTime(t);
}
function TimeInYear( y ) {}
function YearFromTime( t ) {
  var sign = ( t < 0 ) ? -1 : 1;
  var year = ( sign < 0 ) ? 1969 : 1970;
  for ( var timeToTimeZero = t; ;  ) {
    timeToTimeZero -= sign * TimeInYear(year)
          break;
  }
  return ( year );
}
gczeal(4);
evaluate("\
var TIME        = 0;\
var UTC_YEAR    = 1;\
adjustResultArray([]);\
adjustResultArray([946684800000-1]);\
adjustResultArray([]);\
", { noScriptRval : true });

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Trace:


Program received signal SIGSEGV, Segmentation fault.
0x084d4705 in js::ion::MacroAssemblerX86::tagValue (this=0x961e89c, type=JSVAL_TYPE_INT32, payload=..., dest=...) at ../jit/x86/MacroAssembler-x86.h:178
178             JS_ASSERT(payload != dest.typeReg());
(gdb) bt
#0  0x084d4705 in js::ion::MacroAssemblerX86::tagValue (this=0x961e89c, type=JSVAL_TYPE_INT32, payload=..., dest=...) at ../jit/x86/MacroAssembler-x86.h:178
#1  0x084fe5f2 in js::ion::CodeGenerator::visitMaybeToDoubleElement (this=0x961e870, lir=0x95976c0) at js/src/jit/CodeGenerator.cpp:1264
#2  0x08609add in js::ion::LMaybeToDoubleElement::accept (this=0x95976c0, visitor=0x961e870) at ../jit/LIR-Common.h:2834
#3  0x08502ebc in js::ion::CodeGenerator::generateBody (this=0x961e870) at js/src/jit/CodeGenerator.cpp:2705
#4  0x0850f217 in js::ion::CodeGenerator::generate (this=0x961e870) at js/src/jit/CodeGenerator.cpp:5494
#5  0x08542a73 in js::ion::GenerateCode (mir=0x959d850, lir=0x9595f50, maybeMasm=0x0) at js/src/jit/Ion.cpp:1502
#6  0x08542ae4 in js::ion::CompileBackEnd (mir=0x959d850, maybeMasm=0x0) at js/src/jit/Ion.cpp:1521
#7  0x0854333a in js::ion::IonCompile (cx=0x957c830, script=0xf6539280, baselineFrame=0x0, osrPc=0x0, constructing=false, executionMode=js::ion::SequentialExecution)
    at js/src/jit/Ion.cpp:1690
#8  0x08543b56 in js::ion::Compile (cx=0x957c830, script=0xf6539280, osrFrame=0x0, osrPc=0x0, constructing=false, executionMode=js::ion::SequentialExecution) at js/src/jit/Ion.cpp:1848
#9  0x08544313 in js::ion::CanEnter (cx=0x957c830, state=...) at js/src/jit/Ion.cpp:1973
#10 0x080e6e73 in js::RunScript (cx=0x957c830, state=...) at js/src/vm/Interpreter.cpp:421
#11 0x080e73e8 in js::Invoke (cx=0x957c830, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:508
#12 0x080e76f8 in js::Invoke (cx=0x957c830, thisv=..., fval=..., argc=1, argv=0xffffba6c, rval=$jsval(-nan(0xfff8200000000))) at js/src/vm/Interpreter.cpp:539
#13 0x084c653b in js::ion::DoCallFallback (cx=0x957c830, frame=0xffffba9c, stub=0x959d4c0, argc=1, vp=0xffffba5c, res=$jsval(-nan(0xfff8200000000))) at js/src/jit/BaselineIC.cpp:7066
[...]


S-s because this involves GC and it is a low-level JIT assertion.

Comment 3

[Jan de Mooij [:jandem]]

Not a security bug. MaybeToDoubleElement could load an Int32Value (not object/string) with a type tag as payload.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

tagValue will assert if payload == dest.typeReg(). It's actually pretty easy to handle this case by storing the type after (instead of before) storing the payload.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 795541 [details] [diff] [review]
Patch

Review of attachment 795541 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch.
This sounds like a copy & paste error, weird that it only appeared now.

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b484c24235a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4b484c24235a

Comment 8

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 795541 [details] [diff] [review]
Patch

This does break real-world code even without the backtracking allocator, see bug 927977. I'd like to land this on mozilla-beta: the patch has been on Aurora and m-c for over a month without problems and is low risk. I also verified backporting it will fix bug 927977.

[Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
Bug 890524.

> User impact if declined: 
Correctness bugs like bug 927977.

> Testing completed (on m-c, etc.): 
On m-c and Aurora for a month.

> Risk to taking this patch (and alternatives if risky): 
Very low. It's a simple patch that has been on m-c and Aurora for over a month.

> String or IDL/UUID changes made by this patch:
None.

Comment 11

[Alex Keybl [:akeybl]]

"most likely code that does a lot of calculations on arrays, but had to say" -jandem on irc

Given this, and our desire not to regress complex web applications, a+

Comment 12

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-beta/rev/592e9e9d033b