1693328 - Sparse array in spread causes assertion violation in Spidermonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[lukas.bernhard]

Attachment: program_20210217005125_7DE75A57-6431-EEBA-8E9C-C02840A01D55_deterministic_129.js

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:85.0) Gecko/20100101 Firefox/85.0

Steps to reproduce:

Invoking the js shell with the following command
obj-x86_64-pc-linux-gnu/dist/bin/js --fuzzing-safe program_20210217005125_7DE75A57-6431-EEBA-8E9C-C02840A01D55_deterministic_129.js crashes the shell.

Actual results:

The shell crashed with the following assertion violation:
Assertion failure: !aobj->isIndexed(), at js/src/vm/Interpreter.cpp:4955

As far as I understand, the .js creates a sparse array, sets the length to 2 and then
invokes a spread operation when calling Object(...v2). The js::SpreadCallOperation validates the length to be reasonably small (it is, since we set it to 2). However, the array is still sparse, failing the assert.

A git bisect identifies the the first bad commit as:
de1bca0b4f3a9c55f3ca14ab6028e0196803bf07 is the first bad commit
commit de1bca0b4f3a9c55f3ca14ab6028e0196803bf07
Author: Iain Ireland <[e-mail]>
Date: Fri Jan 8 23:34:45 2021 +0000

This commit crashes with:
Assertion failure: !aobj->isIndexed(), at js/src/vm/Interpreter.cpp:5045
./evaluator.sh: line 9: 3608145 Segmentation fault (core dumped) obj-x86_64-pc-linux-gnu/dist/bin/js program_20210217005125_7DE75A57-6431-EEBA-8E9C-C02840A01D55_deterministic_129.js

Comment 1

[:Gijs (he/him)]

The commit in comment #0 is from bug 1674143 - Iain, could you take a look?

Comment 2

[Iain Ireland [:iain]]

Thanks for the report!

Bug 1674143 is a red herring; that patch expanded the set of spread calls that we would attempt to optimize, but something like the following would have triggered the same assertion prior to the patch:

function main(...v2) {
    v2[16777217] = "a";
    v2.length = 2;
    const v5 = Object(...v2);
}
main(0,0);

Fortunately, this is not a security problem. It's just an overly strict assertion.

SM objects contain two arrays of data: the elements array, which contains integer-indexed properties (like o[1]), and the slots array, which mostly contains named properties (like o.x). v2[16777217] is over the cap for how large we are willing to make the elements array, so instead we store it in the slots array. We mark the indexed property flag so that we know something strange is going on. Independently, we track the length of v2 (16777218) and the densely initialized length of the elements array (2). We also track the fact that the elements array itself is still densely packed.

When we set v2.length to 2, we delete any elements with index >= 2. This removes v2[16777217]. The length of v2 is now 2. The densely initialized length of v2 is still 2. The elements array of v2 is still densely packed. When we do a spread call, this means that all of the elements of v2 are stored contiguously in memory, and we can copy the arguments directly out of the elements array instead of having to use the iterator protocol.

The condition I just described is the one that baseline's spread call stubs depend on. We verify it using IsPackedArray. However, the assertion in SpreadCallOperation is stricter: it asserts that the object has never had any indexed properties. We can safely replace that assertion with MOZ_ASSERT(IsPackedArray(aobj)), and everything will work.

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1693328: Fix overly strict assertion in SpreadCallOperation r=jandem

Note: after we remove the indexed property, the array is in dictionary mode, which causes the shape guard in the OptimizeSpreadCall IC to fail repeatedly. We end up attaching new stubs until we transition to megamorphic, then again until we transition to generic. I added the mode check in tryAttachArray to keep it slightly more reasonable.

We could also consider making isFirstStub available in the IRGenerator base class, and using that instead. That might be a good first bug for Outreachy applicants.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f01f49b282a5
Fix overly strict assertion in SpreadCallOperation r=jandem

Comment 5

[Dorel Luca [:dluca]]

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