1669597 - Crash [@ ??] with SIGTRAP

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 7ba9ec4d39f3 (debug build, run with --fuzzing-safe --ion-offthread-compile=off):

var actual = '';
test();
function test() {
    function f87() {
        var b78 = 0
        for (var i60 = 0; i60 < 10; ++i60)
            b78 += +f87(+actual) 
    }
    actual = f87();
}

Backtrace:

Thread 1 "js" received signal SIGTRAP, Trace/breakpoint trap.
#0  0x00000b5c683a22c8 in ?? ()
#1  0x0000ffffffffcc20 in ?? ()
(gdb) x /i $pc
=> 0xb5c683a22c8:       brk     #0x0

Reproduces on aarch64 only. Marking as fuzzblocker due to frequency.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Unable to reproduce bug using the following builds:

mozilla-central 20201006161706-19bc84a9ed83
mozilla-central 20201006161706-19bc84a9ed83
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 3

[Iain Ireland [:iain]]

Here's a reduced testcase:

var str = '';

function f87() {
    while (true) {
        f87(+str)
    }
}
f87();

We crash in the checkStackAlignment assertion at the beginning of the CallKnown for f87, while checking sp (the actual stack pointer, not the pseudo stack pointer). That register was last modified in the CheckUnsafeCallWithABI check here, which is generated as part of the callWithABI in guardStringToInt32. Somewhat unusually, the push(ReturnReg) in that code modifies sp (here), but the pop(ReturnReg) does not.

I think that part is actually fine. If I understand correctly, the invariant we maintain on ARM64 is just that the stack pointer on ARM64 is below the pseudo stack pointer. The precise value doesn't seem to matter (?). A few instructions after we check the alignment of sp, the value is clobbered without ever being read.

I'm not sure if there's a bug here, or just an overly strict assertion. I don't know why this started failing now. If I had to guess, there might be something going on in guardStringToInt32; it's doing some gymnastics with the stack pointer, and it's possible that something is going wrong with our stack adjustment tracking there.

jandem: You added guardStringToInt32 recently. Does anything stand out to you here?

Comment 4

[Jan de Mooij [:jandem]]

I'm pretty sure the problem is that we reserve less than a word, sizeof(int32_t), on the stack. If I change this to sizeof(uintptr_t) we don't hit the breakpoint. I'll look into this in more detail tomorrow and see if we can assert this at compile time in callWithABI at least.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1669597 - Use word-alignment to avoid misaligning the stack pointer. r?iain!

Also adds an assertion to callWithABI. This caught a similar issue with the callVM
trampoline on x64. This now matches the ARM64 and MIPS64 implementations.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Iain Ireland [:iain] from comment #3)

I'm not sure if there's a bug here, or just an overly strict assertion.

As far as I can tell it's just a debug assertion, but it's still worth fixing.

Comment 7

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/6c44f42ee22d5f2299cf1da0a24ebb3ca10656f3

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/6c44f42ee22d