From bda41b026d9d6574c5e8719d5b528b438dabee25 Mon Sep 17 00:00:00 2001 From: "svc-finitelabs[bot]" <269744575+svc-finitelabs[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:35:55 -0500 Subject: [PATCH] fix: truncate 32-bit varints to the field width instead of widening them A negative int32 or enum decoded to 1.8446744073709552e19 on Lua 5.1, 5.2 and LuaJIT, and to the right answer on 5.3 and 5.4 only because the unsigned `value[1] * 0x100000000 + value[2]` in bit64.to_number overflows signed 64-bit integers and wraps. Nothing implemented sign extension; the value was aliased. The fix is not sign extension either. Measured against the reference, the operation is truncation to the field's declared width: the five-byte payload 0xFFFFFFFF is -1 as an int32 despite no sign bit being set at 64 bits, and 0xFFFFFFFF00000001 is 1 rather than a large negative. So 5.3 and 5.4 were not merely right for the wrong reason, they were wrong on those inputs too, as were all six matrix entries. int32 and uint32 are wire compatible by design, so a peer writing uint32 = 4294967295 into a field a driver reads as int32 produces exactly that five-byte form. The same 64-bit payload has to decode to -1 as an int32 and 4294967295 as a uint32, so no definition of bit64.to_number could serve both. The operation is type directed and belongs here rather than in vendor/bitn.lua, which is left untouched and needs no re-vendor. INT32 and ENUM now read the full pair and interpret the low word signed, UINT32 reads it unsigned, and SINT32 zigzags the low word rather than a double that has already rounded: a ten-byte sint32 payload previously reached zigzag_decode32 as 1.8446744073709552e19 and came back 0 on LuaJIT. Six vectors cover the widths the reference's own serializer cannot emit, built through the existing raw_case oracle so the expectations stay derived rather than asserted by construction. Five of them fail on Lua 5.5 before this change, which is what stops the suite accepting wraparound as a pass; the sint32 one fails only on LuaJIT. The three version_dependent entries in test/known_gaps.lua are gone, leaving that tier empty. Fixes FL-19 --- CLAUDE.md | 16 ++++---- src/protobuf/init.lua | 29 +++++++++++++-- test/generated/wire_vectors.lua | 48 ++++++++++++++++++++++++ test/known_gaps.lua | 22 ++++------- tools/gen_wire_vectors | 65 +++++++++++++++++++++++++++++++++ 5 files changed, 154 insertions(+), 26 deletions(-) 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