Jens-G commented on PR #3410: URL: https://github.com/apache/thrift/pull/3410#issuecomment-4300407201
> nit: instead of 10 everywhere, should we use a named constant or comment in the code? as it stands this is kind of like a magic number and hard to reason about Should be covered now. > This only covers pure Ruby implementation, we also have C extension here https://github.com/apache/thrift/blob/master/lib/rb/ext/compact_protocol.c#L442 > > The description also mentions 5 bytes for 32-bit, which is not covered in the change. Issue 1 — Ruby C extension missing: lib/rb/ext/compact_protocol.c:read_varint64 had an unbounded while (true) loop, and read_varint32 just delegated to it. Both are now fixed with bounded for loops using `rb_exc_raise(get_protocol_exception(PROTOERR_INVALID_DATA, ...))`. Issue 2 — 5-byte limit for 32-bit not enforced: The description claimed ceil(32/7) = 5 but neither Ruby path enforced it. Fixed in both the C extension and pure Ruby — read_varint32 now has its own 5-byte loop instead of delegating. What changed: - lib/rb/ext/compact_protocol.c — added `MAX_VARINT32_BYTES 5 / MAX_VARINT64_BYTES 10` defines; replaced both reader functions - lib/rb/lib/thrift/protocol/compact_protocol.rb — added `MAX_VARINT32_BYTES = 5`; `read_varint32` now has its own 5-byte loop - lib/rb/spec/compact_protocol_spec.rb — 2 new tests for 32-bit overlong rejection and valid 5-byte acceptance (23/23 pass) - Commit amended and force-pushed; PR description updated to explain that Java, Ruby, and Rust enforce the 5/10 split, while other runtimes use 10 uniformly -- 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]
