diff --git a/CLAUDE.md b/CLAUDE.md index ec66061..5ed2f53 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -164,14 +164,6 @@ This is the section to read before assuming a `.proto` will round-trip: drops them. - **Groups are unsupported.** `DataType` has no `GROUP` (10) and `WireType` has no SGROUP (3) / EGROUP (4); both raise `"Unknown wire type"`. -- **A negative `int32` or `enum` decodes differently per interpreter.** The wire - form is a ten-byte sign-extended varint, and `bit64.to_number` is unsigned - (`value[1] * 0x100000000 + value[2]`). On 5.3+ that multiply overflows signed - 64-bit integers and wraps to the correct answer; on 5.1, 5.2 and LuaJIT the - operands are doubles, nothing wraps, and `-1` reads as `1.8446744073709552e19`. - Four of the six CI matrix entries are the second kind. Note that a current - Homebrew `lua` is 5.5, which is **not** in the matrix and does wrap, so this is - invisible locally. Tracked as FL-19. - **`sfixed32` is unsigned in both directions.** Decode returns `4294967295` for the wire bytes `FFFFFFFF` where the reference returns `-1`, and encoding a negative one raises `bad argument #4 to 'char'`. `sfixed64` is unaffected: its @@ -295,6 +287,14 @@ SINT64, FIXED64 and SFIXED64 come back as Int64 **tables**, while INT32, UINT32, ENUM, BOOL and FIXED32 come back as plain numbers. `pb.decode_varint` silently truncates beyond 53 bits. +The 32-bit varint types do not go through `pb.decode_varint`. INT32, UINT32, +ENUM and SINT32 read the full `{high, low}` pair and then discard the high word, +because protobuf truncates a varint to the field's declared width rather than +widening it: `int32` and `uint32` are wire compatible, so the five-byte payload +`0xFFFFFFFF` is `4294967295` read as one and `-1` read as the other. Going +through a plain number instead would make the answer depend on the interpreter's +number model, which is what FL-19 fixed. + ### Vendor Dependencies The `vendor/bitn.lua` file is a vendored copy of the [lua-bitn](https://github.com/finitelabs/lua-bitn) library providing portable bitwise operations. Import as `require("bitn")` (not `vendor.bitn`). diff --git a/src/protobuf/init.lua b/src/protobuf/init.lua index 7c847c3..aeaadbe 100644 --- a/src/protobuf/init.lua +++ b/src/protobuf/init.lua @@ -533,6 +533,21 @@ function pb.decode_length_delimited(buffer, pos) return data, new_pos + length end +--- Reads the low 32 bits of a decoded varint as a signed two's complement value. +--- +--- The reference implementation truncates a varint to the field's declared +--- width rather than widening it, so the five-byte payload `0xFFFFFFFF` is -1 +--- as an int32 even though no sign bit is set at 64 bits. +--- @param pair Int64HighLow The decoded varint as a {high_32, low_32} pair. +--- @return integer value The low word interpreted as a signed 32-bit integer. +local function varint_to_int32(pair) + local low = pair[2] + if low >= 0x80000000 then + return low - 0x100000000 + end + return low +end + --- Decodes one non-length-delimited value off the wire. --- @param protoSchema ProtoSchema The complete proto schema. --- @param fieldType integer The field's data type, which selects among the wire type's readings. @@ -553,14 +568,20 @@ local function decode_scalar(protoSchema, fieldType, wireType, buffer, pos) value = pb.zigzag_decode64(raw) elseif fieldType == protoSchema.DataType.SINT32 then local raw - raw, pos = pb.decode_varint(buffer, pos) - value = pb.zigzag_decode32(raw) + raw, pos = pb.decode_varint64(buffer, pos) + value = pb.zigzag_decode32(raw[2]) elseif fieldType == protoSchema.DataType.BOOL then value, pos = pb.decode_varint(buffer, pos) value = value ~= 0 -- Convert to boolean + elseif fieldType == protoSchema.DataType.UINT32 then + local raw + raw, pos = pb.decode_varint64(buffer, pos) + value = raw[2] else - -- INT32, UINT32, ENUM, etc. - value, pos = pb.decode_varint(buffer, pos) + -- INT32, ENUM + local raw + raw, pos = pb.decode_varint64(buffer, pos) + value = varint_to_int32(raw) end elseif wireType == protoSchema.WireType.FIXED64 then if fieldType == protoSchema.DataType.DOUBLE then diff --git a/test/generated/wire_vectors.lua b/test/generated/wire_vectors.lua index aced7f8..95a7170 100644 --- a/test/generated/wire_vectors.lua +++ b/test/generated/wire_vectors.lua @@ -382,6 +382,54 @@ return { }, }, }, + { + name = "int32 from a five-byte payload", + note = "a uint32 producer's 0xFFFFFFFF read back through a wire-compatible int32", + golden = "\008\255\255\255\255\015", + expected = { + optional_int32 = -1, + }, + }, + { + name = "int32 at the sign boundary", + note = "0x80000000 is the low word's first negative value", + golden = "\008\128\128\128\128\008", + expected = { + optional_int32 = -2147483648, + }, + }, + { + name = "int32 with a populated high word", + note = "bits above the low word are discarded rather than widening the value", + golden = "\008\129\128\128\128\240\255\255\255\255\001", + expected = { + optional_int32 = 1, + }, + }, + { + name = "enum from a five-byte payload", + note = "an enum truncates to 32 bits on the same path as int32", + golden = "\168\001\255\255\255\255\015", + expected = { + optional_nested_enum = -1, + }, + }, + { + name = "uint32 from a ten-byte payload", + note = "truncation is unsigned here, so the same bytes are not int32's -1", + golden = "\024\255\255\255\255\255\255\255\255\255\001", + expected = { + optional_uint32 = 4294967295, + }, + }, + { + name = "sint32 from a ten-byte payload", + note = "the zigzag input is the truncated low word, not a rounded double", + golden = "(\255\255\255\255\255\255\255\255\255\001", + expected = { + optional_sint32 = -2147483648, + }, + }, { name = "oneof scalar arm", note = "ordinary field on the wire", diff --git a/test/known_gaps.lua b/test/known_gaps.lua index ca1fd2a..c0c5538 100644 --- a/test/known_gaps.lua +++ b/test/known_gaps.lua @@ -21,19 +21,13 @@ return { }, -- May pass or fail depending on the interpreter's number model, so neither - -- outcome fails the run. These cannot be strict: FL-19 makes a negative int32 - -- decode correctly on 5.3 and 5.4, where signed 64-bit arithmetic wraps, and - -- incorrectly on 5.1, 5.2 and LuaJIT, where it does not. Demanding a failure - -- would turn 5.3 and 5.4 red for the opposite reason. + -- outcome fails the run, and the result is printed rather than asserted. -- - -- The result is still printed on every run, so the split stays visible rather - -- than becoming a silent exclusion. - version_dependent = { - ["negative scalars | decode"] = "FL-19", - ["negative scalars | encode"] = "FL-19", - ["negative enum | decode"] = "FL-19", - ["negative enum | encode"] = "FL-19", - ["maps with 32-bit keys | decode"] = "FL-19", - ["maps with 32-bit keys | encode"] = "FL-19", - }, + -- Empty since FL-19. Its six entries were the only ones: a negative int32 or + -- enum decoded correctly on 5.3 and 5.4, where signed 64-bit arithmetic wraps, + -- and to 1.8446744073709552e19 on 5.1, 5.2 and LuaJIT, where it does not, so a + -- strict entry would only have moved which half of the matrix was red. The + -- decoder now truncates to 32 bits explicitly and the vectors are asserted on + -- every interpreter. + version_dependent = {}, } diff --git a/tools/gen_wire_vectors b/tools/gen_wire_vectors index 3d1c5a1..d35b48b 100755 --- a/tools/gen_wire_vectors +++ b/tools/gen_wire_vectors @@ -201,6 +201,29 @@ def build_cases(cls): + payload ) + def varint_field_wire(field_name, raw): + """ + Build a varint field carrying `raw` as its full unsigned 64-bit payload. + + The reference's serializer only ever emits the canonical width for the + value it holds, so it cannot produce a 32-bit field whose payload has + bits above the low word: writing `int32 = -1` always yields the ten-byte + sign-extended form and never the five-byte `0xFFFFFFFF` that a `uint32` + producer writes for the same field number. int32 and uint32 are wire + compatible by design, so that encoding reaches a real decoder, and only + an assembled vector can carry it. + """ + field = cls.DESCRIPTOR.fields_by_name[field_name] + payload = bytearray() + remaining = raw & 0xFFFFFFFFFFFFFFFF + while True: + group = remaining & 0x7F + remaining >>= 7 + payload.append(group | 0x80 if remaining else group) + if not remaining: + break + return reference_encoder.TagBytes(field.number, 0) + bytes(payload) + def raw_case(name, note, golden, populate): """ A vector whose bytes are assembled rather than serialized whole. @@ -448,6 +471,48 @@ def build_cases(cls): map_entry_wire("map_string_string", value="only"), lambda message: message.map_string_string.__setitem__("", "only")) + # -- 32-bit varints whose payload exceeds the low word, FL-19 ----------- + # + # Every vector here is red on all six matrix entries before the fix, which + # is what makes the suite unable to accept 5.3's signed-64-bit wraparound as + # a pass: wraparound reproduces the low word, and the low word is not the + # answer for any of these. + + raw_case("int32 from a five-byte payload", + "a uint32 producer's 0xFFFFFFFF read back through a wire-compatible int32", + varint_field_wire("optional_int32", 0xFFFFFFFF), + lambda message: setattr(message, "optional_int32", -1)) + + raw_case("int32 at the sign boundary", + "0x80000000 is the low word's first negative value", + varint_field_wire("optional_int32", 0x80000000), + lambda message: setattr(message, "optional_int32", -2147483648)) + + raw_case("int32 with a populated high word", + "bits above the low word are discarded rather than widening the value", + varint_field_wire("optional_int32", 0xFFFFFFFF00000001), + lambda message: setattr(message, "optional_int32", 1)) + + raw_case("enum from a five-byte payload", + "an enum truncates to 32 bits on the same path as int32", + varint_field_wire("optional_nested_enum", 0xFFFFFFFF), + lambda message: setattr(message, "optional_nested_enum", -1)) + + raw_case("uint32 from a ten-byte payload", + "truncation is unsigned here, so the same bytes are not int32's -1", + varint_field_wire("optional_uint32", 0xFFFFFFFFFFFFFFFF), + lambda message: setattr(message, "optional_uint32", 4294967295)) + + # A smaller high word does not discriminate: zigzag_decode32 is built from + # 32-bit primitives that truncate their own input, so it already reads + # 0x100000001 as -1. Only a payload wide enough to lose precision as a + # double reaches the defect, where the old path handed those primitives + # 1.8446744073709552e19 and they returned 0. + raw_case("sint32 from a ten-byte payload", + "the zigzag input is the truncated low word, not a rounded double", + varint_field_wire("optional_sint32", 0xFFFFFFFFFFFFFFFF), + lambda message: setattr(message, "optional_sint32", -2147483648)) + # -- oneof, wire-level only --------------------------------------------- def oneof_scalar(message): message.oneof_uint32 = 11