1830107 - Assertion failure for unexpected range for value in LInt32ToIntPtr in SpiderMonkey debug builds

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[2628388509]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/111.0.0.0 Safari/537.36

Steps to reproduce:

==========test.js==============
function read(a, index) {
for (var i = 0; i < 1; i++) {
a[index + 10];
print(a[index])
}
}

var a = new Int8Array(11);
read(a, 0);
read(a, 0);

for (var i = 0; i > -10; --i) {
read(a, i);
}
==========end of test.js==============
Compile spidermonkey in debug mode.
Run arguments:
./js --no-threads --baseline-warmup-threshold=2 --ion-warmup-threshold=10 test.js

Actual results:

SpiderMonkey Debug build crashes. Assertion failure happened.
LInt32ToIntPtr: unexpected range for value.

Expected results:

SpiderMonkey should print three "1" and nine "undefined".
The problem may be in jit/CodeGenerator.cpp. VisitInt32ToIntPtr verifies non-negative assumption at runtime, if assumption failure, Ion should bailout instead of crashing.

Comment 1

[Iain Ireland [:iain]]

Nice find! Thanks for the report!

This might be an OOB read; marking as security-sensitive for now while I investigate.

Comment 2

[Iain Ireland [:iain]]

After digging into this a bit more, I think it's actually safe, but I'll let Jan take a look before opening it back up.

We have two accesses to an Int8Array, a[index + 10] and a[index]. We convert both index and index + 10 from Int32 to IntPtr using Int32ToIntPtr. Both accesses initially have their own bounds check, but EliminateRedundantChecks coalesces them into a single bounds check. This means that the only use of the second Int32ToIntPtr is a LoadUnboxedScalar. When lowering it, this code observes that the only use expects a bounds-checked index and assumes (correctly!) that we have eliminated a bounds check, allowing us to mark the node as non-negative.

However, the Int32ToIndex is hoisted by LICM and executes before the bounds check. I don't think this is a problem in practice, because the bounds check will still execute before we do the load, but it means that our debug assertion is incorrect: the index is negative at the point of conversion.

Jan, you know this code better than I do. Thoughts on the correct fix?

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1830107 - Remove an invalid debug assertion. r?iain!

In debug builds, LInt32ToIntPtr checks that the int32 input isn't negative if
that's what we determined earlier. However this check is invalid when we hoist the
conversion before a bounds check because in this case the uses will never see a
negative value but the conversion can still see one.

It seems best to remove this check because if we have an index >= INT32_MAX we'd
typically crash immediately anyway, and this makes the code for debug vs non-debug
more similar.

Comment 4

[2628388509]

Thanks for fix. Can this bug be assigned a CVE?

Comment 5

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

Remove an invalid debug assertion. r=iain
https://hg.mozilla.org/integration/autoland/rev/07da12b6f71b2cb3ce2bd9b1206823f53966474d
https://hg.mozilla.org/mozilla-central/rev/07da12b6f71b

Comment 6

[2628388509]

Thanks for fix. Can this bug be assigned a CVE? I am a PHD majoring in computer security. CVE is important for me.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to 2628388509 from comment #6)

Thanks for fix. Can this bug be assigned a CVE? I am a PHD majoring in computer security. CVE is important for me.

Unfortunately this only affects debug builds so isn't a security bug. I'll open it up.

Comment 8

[2628388509]

All right. Thanks!

Comment 9

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons