Assertion failure for unexpected range for value in LInt32ToIntPtr in SpiderMonkey debug builds
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
People
(Reporter: 2628388509, Assigned: jandem)
References
(Blocks 1 open bug, Regression)
Details
(Keywords: regression, sec-audit, Whiteboard: [adv-main115-])
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
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•1 year ago
|
||
Nice find! Thanks for the report!
This might be an OOB read; marking as security-sensitive for now while I investigate.
Comment 2•1 year ago
|
||
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?
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 3•1 year ago
|
||
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.
Updated•1 year ago
|
![]() |
||
Comment 5•1 year ago
|
||
Remove an invalid debug assertion. r=iain
https://hg.mozilla.org/integration/autoland/rev/07da12b6f71b2cb3ce2bd9b1206823f53966474d
https://hg.mozilla.org/mozilla-central/rev/07da12b6f71b
Reporter | ||
Comment 6•1 year ago
|
||
Thanks for fix. Can this bug be assigned a CVE? I am a PHD majoring in computer security. CVE is important for me.
Updated•1 year ago
|
Assignee | ||
Comment 7•1 year ago
|
||
(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.
Updated•1 year ago
|
Description
•