Write 32-bit bitflags unsigned so bit 31 does not crash - #181
Pix3lPirat3 wants to merge 2 commits into
Conversation
Building a bitflags value with |= is signed in JS, so a flag on bit 31 makes the accumulator negative and the underlying u32 writer throws "value out of range". Coerce the value to unsigned (>>> 0) before writing on both the interpreted and compiled paths. Fixes the update_abilities AbilitySet write crash (bedrock-protocol#781). Adds a bit-31 round-trip test on both paths.
rom1504
left a comment
rom1504
left a comment
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
The existing 505 tests pass, but an additional signed-underlying-type control exposes a regression in both engines. See the inline finding. Skills used: prismarine-protocol-data-review checked read/write compatibility in compiled and interpreted codecs; prismarine-review compared the failure with the pre-change behavior and current discussion.
| if (value[key]) val |= f[key] | ||
| } | ||
| // keep 32-bit values unsigned: |= is signed in JS, so bit 31 makes val negative and the writer rejects it | ||
| if (!big) val = val >>> 0 |
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
Please preserve signed underlying integer types when fixing the unsigned bit-31 case. bitflags supports arbitrary integer types: for ['bitflags', { type: 'i32', flags: { top: 31 }, shift: true }], reading ffffffff returns { _value: -1, top: true }. Writing that decoded value now converts it to 4294967295 here and throws from the i32 writer; the same roundtrip succeeds before this change. I reproduced this in both engines, and signed i8/i16/li32 roundtrips regress too. Keep the unsigned u32/lu32 repair while respecting the selected underlying type, including the compiled write/size code, and cover a signed roundtrip alongside the new unsigned test.
Skills used: prismarine-protocol-data-review checked both codec engines and the retained signed-type contract; prismarine-review verified this is introduced by the new coercion.
2 participants
Problem
A 32-bit
bitflagswhose top flag is bit 31 crashes on write. The value is built with|=, which is signed in JS, so setting bit 31 makes it negative, and thelu32/u32writer then throws:Both the interpreted and compiled write/sizeOf paths are affected. Downstream this is the root cause of PrismarineJS/bedrock-protocol#781 (the
update_abilitiesAbilitySet crash), where the ability bitset sets a high bit.Change
Coerce the assembled bitflags value to unsigned (
val = val >>> 0) before handing it to the underlying numeric writer, for non-bigflag sets, on all three sites: interpreted write + sizeOf (src/datatypes/utils.js) and the compiled write + sizeOf codegen (src/datatypes/compiler-utils.js).big(BigInt) flag sets are unchanged.Testing
test/misc.js: a 32-bit bitflags with bit 31 set round-trips on both the interpreted and compiled paths (writes0x80000001, reads the flags back).