Add conformance tests for overlong varints as tags. #24711
Merged
+62
−1
Conversation
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
copybara-service
bot
force-pushed
the
test_840711546
branch
4 times, most recently
from
6a8395d to
da512cb
Compare
No wire format should ever contain an overlong varint, so the topic here is only how to react to non-standard and potentially corrupted data. The situation today is that there's 4 main ways that implementations deal when parsing tags: 1) parse up to 10 bytes, cast to uint32 2) parse up to 10 bytes, reject if it is above uint32_max 3) parse up to 5 bytes, cast to uint32 4) parse up to 5 bytes, reject if it is above uint32_max Of our primary supported implementations, these four strategies are used by Java, Go, C++ and upb correspondingly. Based on examining the situation, the decision taken is that: - Coercing down silently ignoring bits in the tag is dangerous to interpretation-confusion / silent misparsing, which means Java approach is dangerous. - Needing to support parsing up to 10 bytes (even when they may just be all 0x80 and no content) would have real performance implications on the upb and C++ parsers. Since it should really never happen taking any performance hit on all parses based on a hypothetical is considered undesirable. For that reason, the conformance test is set to match upb's behavior, which is slight mismatch to C++ and Go behavior today (in different ways), and larger mismatch to the Java behavior today. Because fixing this 'bug' may be disruptive to a customer in theory (though it would probably mean they have some bad data that was accidentally parsing), we may hold back fixing the behavior to a breaking change release; this change to the conformance suite only establishes the decision on preferred behavior. PiperOrigin-RevId: 841856475
copybara-service
bot
force-pushed
the
test_840711546
branch
from
da512cb to
448b53f
Compare
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.
Add conformance tests for overlong varints as tags.
No wire format should ever contain an overlong varint, so the topic here is only how to react to non-standard and potentially corrupted data.
The situation today is that there's 4 main ways that implementations deal when parsing tags:
Of our primary supported implementations, these four strategies are used by Java, Go, C++ and upb correspondingly.
Based on examining the situation, the decision taken is that:
Coercing down silently ignoring bits in the tag is dangerous to interpretation-confusion / silent misparsing, which means Java approach is dangerous.
Needing to support parsing up to 10 bytes (even when they may just be all 0x80 and no content) would have real performance implications on the upb and C++ parsers. Since it should really never happen taking any performance hit on all parses based on a hypothetical is considered undesirable.
For that reason, the conformance test is set to match upb's behavior, which is slight mismatch to C++ and Go behavior today (in different ways), and larger mismatch to the Java behavior today.
Because fixing this 'bug' may be disruptive to a customer in theory (though it would probably mean they have some bad data that was accidentally parsing), we may hold back fixing the behavior to a breaking change release; this change to the conformance suite only establishes the decision on preferred behavior.