KRPC-564: Reject >10-byte varint tags in protobuf parser - #672
Merged
Conversation
Contributor
Author
Internal code reviewAll issues identified by agent reviewers were fixed. |
Contributor
Author
CI ReportPassed
Failed
|
Mr3zee
approved these changes
Apr 14, 2026
The previous commit moved ConsumedEntireMessage() before ReadVarint64 to avoid unreliable state after fast-path varint failures. However, ConsumedEntireMessage() only tracks the top-level stream end via legitimate_message_end_, not PushLimit boundaries. This caused all native sub-message decoding to fail (71 linuxX64 test failures) because readTag() returned error instead of EOF at limit boundaries. Fix: add BytesUntilLimit() == 0 check before ConsumedEntireMessage(). BytesUntilLimit reliably detects sub-message EOF regardless of the legitimate_message_end_ flag. ConsumedEntireMessage() remains for top-level EOF where no limit is pushed. Bump protobuf shim version to 31.1-4. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…g reader Replace the ReadVarint64 + ConsumedEntireMessage approach with manual byte-by-byte varint parsing via ReadRaw. This fixes two issues: 1. ConsumedEntireMessage() never returns true at the top level because CodedInputStream has no explicit limit set (defaults to INT_MAX), so legitimate_message_end_ is never set when the stream ends. 2. ReadTag() returns 0 for both legitimate EOF and varint errors (>10-byte), with no way to distinguish them after the fact. The new approach reads the first byte with ReadRaw(1) — failure means genuine EOF (the BytesUntilLimit check already handled sub-message boundaries). Success means data is available, so any subsequent varint parsing failure is a real error. This correctly handles all cases: top-level EOF, sub-message boundaries, >10-byte varints, overlong encodings, and truncated varints. Also pass the actual tag value to invalidTag() for better diagnostics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Shim revision bump for the manual varint parsing fix in pw_decoder_read_validated_tag. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix uninitialized tag.value read on native error path: use invalidTag() default (0u) instead of reading unwritten memory - Add unit tests for >10-byte varint and overlong encoding rejection (previously only covered by conformance tests) - Distinguish I/O error from EOF in first ReadRaw: if BytesUntilLimit > 0 but ReadRaw fails, the stream is truncated (return -1), not at a clean EOF boundary Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Write the (partial) decoded tag value to tag_out on error paths in pw_decoder_read_validated_tag, so the Kotlin caller can include the real value in ProtobufDecodingException instead of always showing 0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mr3zee
approved these changes
Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Subsystem
protobuf-api, protobuf native shim
Problem
YouTrack: KRPC-564
The native protobuf parser silently accepted >10-byte varint tags (and overlong varint encodings) instead of rejecting them. This caused 4 conformance test failures (
BadTag_VarintMoreThanTenBytes).Solution
Native C++ (
pw_decoder_read_validated_tag): Replaced theReadVarint64+ConsumedEntireMessageapproach with manual byte-by-byte varint parsing viaReadRaw. The previous approach had a fundamental flaw:ConsumedEntireMessage()never returns true at the top level becauseCodedInputStreamhas no explicit limit set (defaults toINT_MAX), solegitimate_message_end_is never set — causing ALLreadTagcalls to fail at top-level EOF.The new approach:
BytesUntilLimit() == 0— detects sub-message EOF atPushLimitboundaries (unchanged).ReadRaw(&b, 1)— reads a single byte to distinguish top-level EOF (fails → no data) from varint start (succeeds → data available). This avoidsReadTag()'s ambiguous return-0-for-both-EOF-and-errors.Why not
ReadTag()+ position tracking?ReadTag()on a >10-byte varint doesn't advanceCurrentPosition()(fast-path varint reader uses a local pointer), so EOF and error are indistinguishable.Why not top-level
PushLimit(source_size)?PushLimitchanges nested limit behavior — sub-messagePushLimitcalls get capped to the top-level limit, causing truncated messages to be silently accepted instead of rejected (36 conformance regressions).JVM: Added a try-catch for
InvalidProtocolBufferExceptioninreadTag(), making it self-contained.Diagnostics:
invalidTag()now includes the actual tag value in the error message.Known failures: Removed 4
BadTag_VarintMoreThanTenBytesentries fromnative_known_failures.txt— all 5670 conformance tests now pass.Shim version: Bumped protobuf shim from 31.1-4 to 31.1-5.
Note
Fully autonomous AI-generated PR — no human reviewed the code before submission.
Problem analysis and root cause details: KRPC-564