Fix uint8 overflow in array literal skipIndexesId and sourcePositions#2321
Fix uint8 overflow in array literal skipIndexesId and sourcePositions#2321Finomosec wants to merge 14 commits intomozilla:masterfrom
Conversation
* Set `global` as part of initialisation. * Add a test that `global` and `globalThis` are the same thing.
* Fix debugger eval for Script * Fix debugger eval scope for top-level scripts
This is to avoid repeatedly parsing the replacement template in case of multiple matches.
…sourcePositions When a script contains more than 255 object/array literals, the literalIds table grows beyond 255. The skipIndexesId (used for sparse array literals) and sourcePositions (used for spread in sparse arrays) were encoded as uint8, causing an IllegalStateException (Kit.codeBug) when the values exceeded 255. Changed addUint8 to addUint16 in CodeGenerator.visitArrayLiteral and updated the corresponding reads in Interpreter (DoLiteralNewArray, DoSpread) to use getIndex (uint16) instead of single-byte reads. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
I think this is potentially very helpful as I have been trying to run more standard benchmarks and we've run into some of these types of overflows. However I think that your actual change is buried behind a lot of other commits in this PR -- can you try to rebase your change against the current master branch? You should end up with just one commit that's different when you compare in the GitHub UI. Thanks! |
| rootProject.name=rhino-root | ||
| group=org.mozilla | ||
| version=1.9.1 | ||
| version=1.9.1-PATCHED |
There was a problem hiding this comment.
This needs to be reverted
| * When a script contains more than 255 object/array literals, the literalIds | ||
| * table grows beyond 255, and skipIndexesId (encoded as uint8) overflows. | ||
| */ | ||
| public class ArrayLiteralOverflowTest { |
There was a problem hiding this comment.
This test should be modified to use Utils.runWithAllModes.
|
This looks correct to me. I've put a couple of small comments, but they should be pretty easy to change. |
|
I'm happy to take this but someone (or something) needs to fix the PR so it doesn't have a copy of a whole bunch of other PRs in it. TBH I'm not sure how it got this way but I might suggest just creating a new PR and cherry-pick the one commit that actually changes something. |
|
as @Finomosec seems to be from germany, i tried to reach out to him via linkedin - maybe we can align. If not i will extract a simple PR from this one... 🤞 |
8 participants
Summary
CodeGenerator.visitArrayLiteralencodesskipIndexesIdandsourcePositionsas uint8 viaaddUint8, which overflows when a script contains more than 255 object/array literals (causingliteralIdsto exceed 255 entries)addUint8toaddUint16for these values inCodeGeneratorand updated corresponding reads inInterpreter(DoLiteralNewArray,DoSpread) to usegetIndex(uint16)bytecodeSpanentry forIcode_LITERAL_NEW_ARRAYto account for the new 2-byte encodingoptimizationLevel = -1)Reproduction
A script with 260+ object literals followed by a sparse array literal (e.g.
[1, , 3]) triggersIllegalStateException(fromKit.codeBug()inaddUint8) during compilation in interpreter mode.Test plan
ArrayLiteralOverflowTestwith 4 tests covering both interpreted and compiled modes, with and without spread operators