Jens-G commented on PR #3610: URL: https://github.com/apache/thrift/pull/3610#issuecomment-4846458306
### Code review Found 3 issues (adversarially verified with failing tests): 1. Exploit description in test inline comment (AGENTS.md ยง6) Lines 345โ347 of the new test describe the byte-level overread path in terms that name the exact exploitation mechanism. [AGENTS.md ยง6](https://github.com/apache/thrift/blob/master/AGENTS.md#6-security-work) requires neutral functional language in inline comments; the description of what the old code allowed to happen should be rephrased in terms of the invariant being enforced rather than the failure mode. https://github.com/apache/thrift/blob/5c190a74e7ec3e3262ae73e63043d0cd955d1ef4/lib/cpp/test/ThrifttReadCheckTests.cpp#L345-L347 Verified: text absent from base commit `d68305a73`, present in PR code. 2. `strLen < 0` branch has no test The new guard catches negative `strLen` values, but no test in the PR exercises this path. A 5-byte varint with bit 31 set (e.g. `0x80 0x80 0x80 0x80 0x08`) produces a negative `int32_t` from `readVarint32` and reaches this branch; without the guard, `str.assign(ptr, (size_t)(-2147483648))` propagates a `std::length_error` rather than a `TTransportException`. https://github.com/apache/thrift/blob/5c190a74e7ec3e3262ae73e63043d0cd955d1ef4/lib/cpp/src/thrift/transport/THeaderTransport.cpp#L205-L206 Verified: Docker test confirmed the path is reachable and behaves differently on base vs. PR code. 3. `ptr` is advanced before the bounds check, contradicting the `"Advances ptr on success"` doc comment Moving `ptr += bytes` to before the `if` means `ptr` (a non-const reference) is left pointing past the varint bytes when `readString` throws. The Doxygen comment documents that the function advances `ptr` only on success. Current callers in `readHeaderFormat` do not catch `readString` exceptions, so there is no immediate runtime effect, but the documented contract no longer holds and would mislead any subclass that catches the exception and then inspects `ptr`. Either the doc comment or the advance placement should be updated. https://github.com/apache/thrift/blob/5c190a74e7ec3e3262ae73e63043d0cd955d1ef4/lib/cpp/src/thrift/transport/THeaderTransport.cpp#L204-L209 Verified: Docker two-state proof via subclass harness โ assertion that `ptr` is unchanged after exception fails on PR code, passes on base. ๐ค Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with ๐. Otherwise, react with ๐.</sub> -- 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]
