avm2: Treat hasnext2/nextname/nextvalue index as u32 - #24166
avm2: Treat hasnext2/nextname/nextvalue index as u32#24166SuchAFuriousDeath wants to merge 2 commits into
Conversation
37e208d to
91dfe81
Compare
kjarosh
left a comment
There was a problem hiding this comment.
LGTM, but would be nice if @Lord-McSweeney or @adrian17 took a look as well
Lord-McSweeney
left a comment
There was a problem hiding this comment.
From its source, avmplus always coerces the index passed to nextname, nextvalue, and hasnext(2) to an int. Is there some evidence suggesting otherwise?
|
Does the test fail without changes from this PR? |
Yes |
The game not working as well as the test. |
kjarosh
left a comment
There was a problem hiding this comment.
As mentioned on Discord,
ArrayStorage::Sparse::get_next_enumerant returns key + 1 as the cookie
This is the root cause, not the fact that we stop iteration for negative int values. Sadly, those indices are untested in ruffle that's why the tests pass here.
Description
Fixes #11181.
The
hasnext2,nextname, andnextvalueopcodes coerced their enumeration cookie toi32and treated negative values as end-of-iteration.ArrayStorage::Sparse::get_next_enumerantreturnskey + 1as the cookie, soas soon as the enumeration reached a sparse index
>= 2^31, the cookie wrapped to a negativei32and iteration terminated. Callers ended up withnullin the middle of the enumerated sequence and every key past thatpoint was lost.
Boxhead the Nightmare (issue #11181) writes a SharedObject whose
objectsfield is a sparseArraykeyed by 32-bithashTagvalues. Restoring that save into Ruffle dropped every key>= 2^31;FDataFields/set objects()then walked the array and hit
TypeError #1009 accessing field: hashTag, aborting boot.This restores the pre-e7ee3d3e1 (#18958)
u32semantics, which match avmplus.op_has_nextis left as-is — the legacyhasnextopcode is not covered by the new test and modern ASC only emitshasnext2. Someone can flipit in a follow-up.
Checklist