I32 varints should be (de)serialized as 10 bytes of all-1s, not 5.

[hans-tvs]

I submitted a PR recently to convert the problem from a panic to a runtime error, but I was under the mistaken impression that the protobuf data we were being given was occasionally broken. Instead, the Python and C++ canonical implementations, along with the docs, indicate that the appropriate order of operations is something like the following:

// potential_varint: []const u8 -- valid if at most 10 bytes have 1 as the MSB
const decoded_x: u64 = try decode_varint(potential_varint);
const interpreted_x: i32 = @intCast(@as(i64, @bitCast(decoded_x)));

As opposed to the current logic, which either truncates the extra 4 bytes of decoded data, or fails when any of those extra 4 bytes are 0.

This is likely a (small) problem throughout the codebase, not isolated to enums or any one feature.

[hans-tvs]

Sorry about the issue noise; I misread several things when posting this.

[menduz]

false alarm?

[Arwalk]

Yeah, is there an actual problem or not? 🤔

And no problem for the issue at all @hans-tvs , even if your assumptions were incorrect, discussion is among the most valuable things we can have here. Which is why i moved it to the discussion page.

[hans-tvs]

I looked at this more closely, and I'm pretty sure now it wasn't a false alarm:

Wire Format Spec

The docs are slightly ambiguous, but the sentence

However, varints are unsigned, so the different signed types, sint32 and sint64 vs int32 or int64, encode negative integers differently.

seems to place an emphasis on the difference being between sint and int types, grouping sint32 and sint64 into the same representation, then also int32 and int64 into the same representation.

Other Libraries

The reason I wrote that PR in the first place is that we were getting oddly encoded negative integers from one of our vendors (64-bits of 1s in the decoded varint vs 32 for an int32 value of -1). They've talked quite a bit about the various optimizations they'd done in their C++ codebase, so I'd thus-far assumed they'd written their own protobuf serializer, but on closer inspection both the official Python and C++ serializers emit something closer to varint(bytes(int_cast(some_i32_value))) than varint(bytes(some_i32_value)).

[menduz]

amazing, thanks for the analysis and sharing it. I think it would be really helpful to get some test vectors to validate against other implementations and write the fix and tests. Did you encounter some examples we can use for that purpose?

[Arwalk]

Thanks, I'll look into the details if I get some time soon. I agree with @menduz , the sooner we have some test examples that illustrates the problem, the better.

[Arwalk]

If i understand correctly your analysis, the specs means that the varint wire format is always based on i64 data, meaning that for example, if you encode an i32 with value -1, you must encode the equivalent i64 = -1 (resulting in encoding 10 bytes instead of 5)? Only in the case of proto type int32.

sint32/sint64 shouldn't be affected by the mistake as they are zig-zag encoded which is something different.

int64 should be fine too currently as there is no truncated data in it.

I have to check in reference implementations for this, if that's the case that means I fucked up pretty hard from the beginning :(

[Arwalk]

@hans-tvs @menduz I made a branch with the potential fix (but i'm still waiting for you to confirm that i correctly understood the problem). Here is the commit on the branch that should fix everything (it breaks tests, and that's expected) aa57f3c

Luckily i ensured that encoding/decoding is always at the same place so there should not be any other place to fix.

[Arwalk]

I'll add some tests for all varints with max values, -1. I'll compare with the python implementation.

[Arwalk]

@hans-tvs @menduz I checked and here are the depressing results.

I created this simple proto file

syntax = "proto3";
package demo;

message Base {
        int32 a_number = 1;
}

using protoc i made the python generated code, installed the protobuf package for python, and run this simple test

Python 3.13.0 (main, Nov 12 2024, 06:05:34) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from demo_pb2 import Base
>>> item = Base()
>>> item.a_number = -1
>>> serialized = item.SerializeToString()
>>> print(serialized.hex())
08ffffffffffffffffff01

we can clearly see the 10 bytes, which means i have let a terribly bad bug in the code since the very beginning.

Apart from fixing it, we need to communicate on solutions for people affected with it.

[menduz]

Oh boy, we did have a good strike of compatibility for a first major

[Arwalk]

I have an active branch where I generated all limits for varints using the python implementation as a reference. Overall it would seem my varint implementation sucked, as it failed for more of them that I would have thought. I'll try to finish this one of these days in between sobbing in a corner.

[Arwalk]

zig https://github.com/Arwalk/zig-protobuf/releases/tag/v2.0.0 has been released with the fix, and some informations about how to handle any problem.