1811803 - Crash [@ ??] in JIT code

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20230122-4ffae7ad22de (debug build, run with --fuzzing-safe --ion-offthread-compile=off --fast-warmup):

a = {}
for (b = 0; b < 100; ++b)
  a['x' + b] = 'x' + b;

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x3f342dcf in ?? ()
#1  0x3f3007f3 in ?? ()
#2  0x5867b14f in js::jit::EnterBaselineInterpreterAtBranch(JSContext*, js::InterpreterFrame*, unsigned char*) ()
#3  0x57c7b382 in Interpret(JSContext*, js::RunState&) ()
[...]
#11 0x57b04a5b in Shell(JSContext*, js::cli::OptionParser*) ()
#12 0x57afcec9 in main ()
eax	0xffffff86	-122
ebx	0xf5d00530	-170916560
ecx	0x1eb0a9	2011305
edx	0x1eae199e	514726302
esi	0xdeadbeef	-559038737
edi	0x1eae199e	514726302
ebp	0xffffaf00	4294946560
esp	0xffffaee8	4294946536
eip	0x3f342dcf	1060384207
=> 0x3f342dcf:	testl  $0x8,(%edi)
   0x3f342dd5:	je     0x3f342dfe

Reproduces on 32-bit only. Marking s-s because this is a non-zero crash in JIT code. Also marking as fuzzblocker due to frequency.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Jan de Mooij [:jandem]]

NI Doug because this is likely from the megamorphic SetElem changes (note that you need a 32-bit build).

Comment 4

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20230123094444-1af6fa02884e.
The bug appears to have been introduced in the following build range:

Start: 849b9d9a8b6f197d20a2f66addd0e3fc1e54f6d4 (20230119163652)
End: f2fbf518572b21b732795f6edeb10a3209799757 (20230120212103)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=849b9d9a8b6f197d20a2f66addd0e3fc1e54f6d4&tochange=f2fbf518572b21b732795f6edeb10a3209799757

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Based on comment #4, this bug contains a bisection range found by bugmon. However, the Regressed by field is still not filled.

:dthayer, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit auto_nag documentation.

Comment 6

[Donal Meehan [:dmeehan]]

:dthayer As mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=858032#c37, there is a spike of crashes in Nightly since Friday 2023-01-20.
Do we know which patch in Comment 4 introduced this?

Comment 7

[Alex Thayer [:alexical] (she/her)]

I assigned myself because it does indeed look like the megamorphic SetElem changes, but I can't get this to reproduce on either 32 bit Linux or Windows. Jan (or anyone), any tips?

Comment 8

[Christian Holler (:decoder)]

This reproduces right away for me using a 32-bit linux JS shell build fetched from our TaskCluster CI:

python -mfuzzfetch --target js --cpu x86 --debug -n js32
LD_LIBRARY_PATH=js32/dist/bin js32/dist/bin/js --fuzzing-safe --ion-offthread-compile=off --fast-warmup test.js

You can install fuzzfetch via pip, it is just a script that makes it easier to download Mozilla TC builds. I did this on 64-bit Linux.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Doug Thayer [:dthayer] (he/him) from comment #7)

I assigned myself because it does indeed look like the megamorphic SetElem changes, but I can't get this to reproduce on either 32 bit Linux or Windows. Jan (or anyone), any tips?

Repros for me on Linux. I use a mozconfig like this to cross-compile the JS shell for 32-bit:

ac_add_options --enable-application=js
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-shell-dbgopt-32
ac_add_options --enable-debug
ac_add_options --enable-optimize
ac_add_options --target=i686-pc-linux

Then run the test like this:

$ obj-shell-dbgopt-32/dist/bin/js --fuzzing-safe --ion-offthread-compile=off --fast-warmup test.js
Segmentation fault (core dumped)

Unfortunately ./mach run eats that "Segmentation fault (core dumped)" line so running the shell manually works better for this. We really need to fix that.

Comment 10

[Jan de Mooij [:jandem]]

On x86 we push the value type/payload registers in emitMegamorphicCachedSetSlot and then we use those registers as scratch. In this case those registers are ecx/edx. However the id uses register edx too (eax/edx I think), so we crash accessing the atom in loadAtomOrSymbolAndHash because edx is now being used as scratch register.

The simplest fix might be to use fixed registers for all operands of LMegamorphicSetElement in lowering on x86 with useBoxFixedAtStart, to force those registers to be disjoint (and always the same registers).

Let me know if you can't repro this locally and I can take a closer look.

Comment 11

[Andrew McCreight [:mccr8]]

I don't know how practical it is to exploit it, but sharing a single register for multiple values sounds bad so I'll mark it sec-high.

Comment 12

[Alex Thayer [:alexical] (she/her)]

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

(In reply to Doug Thayer [:dthayer] (he/him) from comment #7)

I assigned myself because it does indeed look like the megamorphic SetElem changes, but I can't get this to reproduce on either 32 bit Linux or Windows. Jan (or anyone), any tips?

Repros for me on Linux. I use a mozconfig like this to cross-compile the JS shell for 32-bit:

ac_add_options --enable-application=js
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-shell-dbgopt-32
ac_add_options --enable-debug
ac_add_options --enable-optimize
ac_add_options --target=i686-pc-linux

Then run the test like this:

$ obj-shell-dbgopt-32/dist/bin/js --fuzzing-safe --ion-offthread-compile=off --fast-warmup test.js
Segmentation fault (core dumped)

Unfortunately ./mach run eats that "Segmentation fault (core dumped)" line so running the shell manually works better for this. We really need to fix that.

Yeah that looks effectively the same as my mozconfig. I'll just add a test though and see if I can get it to fail on try and pass with the fix you suggest, as it sounds pretty trivial. If that doesn't work out then I'll let you know.

Comment 13

[Donal Meehan [:dmeehan]]

[Tracking Requested - why for this release]:

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1809359

Comment 15

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1811803 - Use useBoxFixedAtStart in LMegemorphicSetElement r?jandem

Comment 16

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

Use useBoxFixedAtStart in LMegemorphicSetElement r=jandem
https://hg.mozilla.org/integration/autoland/rev/a1ca52a54ce80493e975350f1cc00f4eca15ed76
https://hg.mozilla.org/mozilla-central/rev/a1ca52a54ce8

Comment 17

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20230127094652-f75c73066b88.