kpumuk commented on code in PR #3387:
URL: https://github.com/apache/thrift/pull/3387#discussion_r3076285872
##########
lib/rb/ext/compact_protocol.c:
##########
@@ -419,17 +419,17 @@ static char read_byte_direct(VALUE self) {
return (char)(FIX2INT(byte));
}
-static int64_t zig_zag_to_ll(int64_t n) {
- return (((uint64_t)n) >> 1) ^ -(n & 1);
+static int64_t zig_zag_to_ll(uint64_t n) {
+ return (int64_t)((n >> 1) ^ (0ULL - (n & 1ULL)));
}
-static int32_t zig_zag_to_int(int32_t n) {
- return (((uint32_t)n) >> 1) ^ -(n & 1);
+static int32_t zig_zag_to_int(uint32_t n) {
+ return (int32_t)((n >> 1) ^ (0U - (n & 1U)));
}
-static int64_t read_varint64(VALUE self) {
+static uint64_t read_varint64(VALUE self) {
Review Comment:
`read_varint64()` should return `uint64_t` because it reads raw varint bits,
not a signed Thrift value yet. For values like the ZigZag encoding of
`INT64_MIN`, the wire value is `0xffffffffffffffff`, which is representable as
`uint64_t` but not as a meaningful positive `int64_t`.
UBSan reports the problem in the 64-bit decode path when `read_varint64()`
returns `int64_t` and the raw unsigned value is forced through a signed type
too early:
```
runtime error: implicit conversion from type 'uint64_t' ... value
18446744073709551615 to type 'int64_t' ... changed the value to -1
```
*`read_varint64() -> uint64_t` for raw varint bits
* `zig_zag_to_ll(uint64_t) -> int64_t` for the actual signed decode
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
