pawitp / protobuf-decoder

JavaScript-based web UI to decode ad-hoc Protobuf data
https://protobuf-decoder.netlify.app/
MIT License
392 stars 91 forks source link

VarInt field with INT64_MIN is decoded as INT64_MAX + 1 #77

Closed Endilll closed 8 months ago

Endilll commented 9 months ago

Schema:

message Test {
  optional int64 TestField = 1;
}

Encoded message: 08 80 80 80 80 80 80 80 80 80 01 Expected output: As Int: -9223372036854775808 Actual output: As Int: 9223372036854775808

This test case is also present in mapbox/protozero, which handles it as expected: https://github.com/mapbox/protozero/blob/542fcf7dd228672b775eb283d18ccd94c4e682d9/test/t/int64/testcase.cpp#L27-L28 https://github.com/mapbox/protozero/blob/master/test/t/int64/data-min.pbf

Good job taking care of big integers! 2 online encoders that show up in Google search results give up on anything larger than 2^53, which your decoder handles correctly.

pawitp commented 9 months ago

This behavior is because the size of the integer (int8/16/32/64) is not encoded into the Protobuf message itself, so it does not know: 1) Should it flip into negative using 2-complements or not 2) At which point it should treat the integer as 2-complements

For simplicity, in the initial implementation, the decoder treats everything as uint64. The way to fix this is to, for certain ranges, offer an alternative interpretation as 2-complement int8/16/32/64.

Would you be open to submitting a PR? It would be similar to #37.

Endilll commented 9 months ago

Would you be open to submitting a PR?

Unfortunately, I'm not a JavaScript developer. I hope someone else can step up to improve this otherwise excellent low-level decoder.

pawitp commented 8 months ago

Please give this a go: https://deploy-preview-80--protobuf-decoder.netlify.app/

Endilll commented 8 months ago

Seems to work fine to me, thank you!

I used tests I developed for my upcoming protofuf emitter in LLVM (https://github.com/Endilll/llvm-project/blob/protobuf/llvm/unittests/Support/Protobuf.cpp), which were checked against https://github.com/protobufjs/protobuf.js