THRIFT-5941: Add Ruby ext cppcheck coverage

[kpumuk]

Introduce cppcheck SCA for the Ruby native extension under lib/rb/ext and fix the existing findings so it can run in CI without noise.

Offenses addressed:

lib/rb/ext/compact_protocol.c:128:24: error: Shifting signed 32-bit value by 31 bits is undefined behaviour [shiftTooManyBitsSigned]
  return (n << 1) ^ (n >> 31);
                       ^
lib/rb/ext/compact_protocol.c:132:24: error: Shifting signed 64-bit value by 63 bits is undefined behaviour [shiftTooManyBitsSigned]
  return (n << 1) ^ (n >> 63);
                       ^
lib/rb/ext/compact_protocol.c:427:33: error: Signed integer overflow for expression '-(n&1)'. [integerOverflow]
  return (((uint32_t)n) >> 1) ^ -(n & 1);
                                ^
lib/rb/ext/struct.c:255:11: style: The scope of the variable 'key' can be reduced. [variableScope]
    VALUE key;
          ^
lib/rb/ext/struct.c:256:11: style: The scope of the variable 'val' can be reduced. [variableScope]
    VALUE val;
          ^
lib/rb/ext/struct.c:492:9: style: The scope of the variable 'i' can be reduced. [variableScope]
    int i;
        ^
lib/rb/ext/struct.c:531:9: style: The scope of the variable 'i' can be reduced. [variableScope]
    int i;
        ^
lib/rb/ext/struct.c:558:9: style: The scope of the variable 'i' can be reduced. [variableScope]
    int i;
        ^
lib/rb/ext/thrift_native.c:124:0: style: The function 'Init_thrift_native' is never used. [unusedFunction]
RUBY_FUNC_EXPORTED void Init_thrift_native(void) {
^

• Did you create an Apache Jira ticket? THRIFT-5941
• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
• Did you squash your changes to a single commit? (not required, but preferred)
• Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
• If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

[Jens-G]

Why do we change that to uint64_t? That introduces a lot of implicit things going on. Do we really want that?

[kpumuk]

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

[kpumuk]

With the last amend:

• All conversions are explicit, including (potentially overkill unnecessary seqid overflow, mirroring logic in Ruby)
• Separated int32 from int64 path via read_varint32 / read_varint64
• ZigZag decode uses unsigned ints

Still lacking:

• Size enforcement (C++ enforces 5 bytes for int32, 10 bytes for int64, binary/name length to INT32_MAX via signed out int32_t and >= 0 comparison). Semantically, we should fail on > INT32_MAX

[Jens-G]

cast to int32_t seems stale:

• read_varint64() returns uint64_t
• zig_zag_to_int() expects uint32_t

[kpumuk]

Nice catch. I was following the cppcheck complaints and missed this :-(
I also noticed that we do not validate the size of varint in Ruby (cpp fails on >10 bytes), but will leave it for the future.

[Jens-G]

+1 LGTM with one extra request

[Jens-G]

Can we have that for i64 too?

[kpumuk]

Added