1643888 - Assertion failure: Missing guard allowed non-number to hit ensureDoubleRegister, at js/src/jit/MacroAssembler.cpp:1970

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect, P2)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200605-dadc7312128e (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --disable-oom-functions --blinterp-warmup-threshold=1):

str = function(s) { try { return s.toString(); } catch(exc) {} }
str(undefined);
str(false);
function g() {
    for (let i = 0; i < 2; -i) {
        var y = this * this;
        str(y >>> y);
    }
}
g();

Backtrace:

received signal SIGTRAP, Trace/breakpoint trap.
0x5da30d7b in ?? ()
#0  0x5da30d7b in ?? ()
#1  0xf685e088 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
eax	0x7ff80000	2146959360
ebx	0x7ff80000	2146959360
ecx	0x0	0
edx	0x0	0
esi	0x0	0
edi	0xf6875840	-158902208
ebp	0xf6860040	4135977024
esp	0xffffa9e0	4294945248
eip	0x5da30d7b	1570966907
=> 0x5da30d7b:	cvttsd2si %xmm0,%eax
   0x5da30d7f:	cmp    $0x1,%eax

Filing as s-s because this is a JIT assertion. Note that this bug is 32-bit only.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jan de Mooij [:jandem]]

I think this is a regression from bug 1641789. The code looks like this:

  AutoScratchFloatRegister floatReg(this);
  allocator.ensureDoubleRegister(masm, inputId, floatReg);

That's wrong when inputId is Value allocated on the stack, because AutoScratchFloatRegister can push a double on the stack without informing the CacheIR register allocator, resulting in the wrong offset being used.

This is too error-prone, maybe it's finally time to properly support FloatRegisters in CacheIR. I'll investigate.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)

This is too error-prone, maybe it's finally time to properly support FloatRegisters in CacheIR. I'll investigate.

I looked into this yesterday. It's doable and does make things nicer, but it also adds some complexity especially for Ion ICs. For now it's probably best to land a spot fix. Post-Warp we can simplify Ion ICs more and that will make it easier to do this properly.

I'll mark this sec-moderate because all code paths in ensureDoubleRegister that load from the stack will either hit the assumeUnreachable on non-number values (a crash even in release builds) or end up converting an int32 value to double.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1643888 - Support using AutoScratchFloatRegister with ensureDoubleRegister. r?anba!

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1643888 - Add a test. r?anba!

Depends on D78863

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/2048e8b7abddc46601ff802ab29f37d4b377f5a9

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2048e8b7abdd

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 9155215 [details]
Bug 1643888 - Support using AutoScratchFloatRegister with ensureDoubleRegister. r?anba!

Beta/Release Uplift Approval Request

• User impact if declined: Crashes, potential security issues.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Pretty local fix, fixes the reported test case.
• String changes made/needed: None

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 9155215 [details]
Bug 1643888 - Support using AutoScratchFloatRegister with ensureDoubleRegister. r?anba!

approved for 78.0b6

Comment 10

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b753aa76a080

Comment 11

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200611093454-10ad7868f3ca.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24986ae41f0c
Add a test. r=anba

Comment 14

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/24986ae41f0c