Skip to content

Commit da512cb

Browse files
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: 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: 840711546
1 parent 54a48aa commit da512cb

11 files changed

+62
-1
lines changed

conformance/binary_json_conformance_suite.cc

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1411,6 +1411,41 @@ void BinaryAndJsonConformanceSuiteImpl<MessageType>::TestIllegalTags() {
14111411
name.back() += i;
14121412
ExpectParseFailureForProto(nullfield[i], name, REQUIRED);
14131413
}
1414+
1415+
// Reused for a few cases: this is a single byte tag for a field number 1
1416+
// varint but where the byte has the continuation bit set, so that we can
1417+
// easily construct longer tags.
1418+
std::string tag_with_continuation_bit =
1419+
tag(1, WireFormatLite::WIRETYPE_VARINT);
1420+
assert(tag_with_continuation_bit.size() == 1);
1421+
tag_with_continuation_bit[0] |= 0b10000000;
1422+
1423+
// A varint where field number is far out of range of
1424+
// maximum legal field number. The lower 5 bytes of the varint do look like a
1425+
// well-formed tag for field number 1.
1426+
ExpectParseFailureForProto(
1427+
absl::StrCat(tag_with_continuation_bit, "\x80\x80\x80\x80\x80\x0F",
1428+
varint(1234)),
1429+
"BadTag_FieldNumberTooHigh", REQUIRED);
1430+
1431+
// A 5-byte tag varint where the value is above UINT32_MAX (bit 35 is set)
1432+
ExpectParseFailureForProto(
1433+
absl::StrCat(tag_with_continuation_bit, "\x80\x80\x80\x40", varint(1234)),
1434+
"BadTag_FieldNumberSlightlyTooHigh", REQUIRED);
1435+
1436+
// A tag where the varint is more than 5 bytes but only because it is an
1437+
// overlong varint, so the decoded value is still below UINT32_MAX.
1438+
ExpectParseFailureForProto(
1439+
absl::StrCat(tag_with_continuation_bit, "\x80\x80\x80\x80\x80\x80\x80",
1440+
std::string("\0", 1), varint(1234)),
1441+
"BadTag_OverlongVarint", REQUIRED);
1442+
1443+
// An overlong varint that is even more than 10 bytes.
1444+
ExpectParseFailureForProto(
1445+
absl::StrCat(tag_with_continuation_bit,
1446+
"\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80",
1447+
std::string("\0", 1), varint(1234)),
1448+
"BadTag_VarintMoreThanTenBytes", REQUIRED);
14141449
}
14151450

14161451
template <typename MessageType>

conformance/failure_list_cpp.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,4 +34,4 @@ Recommended.*.FieldMaskTooManyUnderscore.JsonOutput
3434
Recommended.*.JsonInput.FieldMaskInvalidCharacter # Should have failed to parse, but didn't.
3535
Required.*.JsonInput.SingleValueForRepeatedFieldInt32 # Should have failed to parse, but didn't.
3636
Required.*.JsonInput.SingleValueForRepeatedFieldMessage # Should have failed to parse, but didn't.
37-
37+
Required.*.ProtobufInput.BadTag_FieldNumberSlightlyTooHigh # Should have failed to parse, but didn't.

conformance/failure_list_csharp.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,3 +37,7 @@ Recommended.Editions_Proto3.ValueRejectNanNumberValue.JsonOutput
3737
Required.Editions_Proto2.ProtobufInput.UnknownOrdering.ProtobufOutput
3838
Required.Editions_Proto3.ProtobufInput.UnknownOrdering.ProtobufOutput
3939

40+
Required.*.ProtobufInput.BadTag_FieldNumberTooHigh # Should have failed to parse, but didn't.
41+
Required.*.ProtobufInput.BadTag_FieldNumberSlightlyTooHigh # Should have failed to parse, but didn't.
42+
Required.*.ProtobufInput.BadTag_OverlongVarint # Should have failed to parse, but didn't.
43+

conformance/failure_list_java.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,3 +43,6 @@ Required.*.JsonInput.RepeatedFieldWrongElementTypeExpectingStringsGotBool
4343
Required.*.JsonInput.RepeatedFieldWrongElementTypeExpectingStringsGotInt # Should have failed to parse, but didn't.
4444
Required.*.JsonInput.StringFieldNotAString # Should have failed to parse, but didn't.
4545
Required.*.ProtobufInput.UnknownOrdering.ProtobufOutput # Unknown field mismatch
46+
Required.*.ProtobufInput.BadTag_FieldNumberTooHigh # Should have failed to parse, but didn't.
47+
Required.*.ProtobufInput.BadTag_FieldNumberSlightlyTooHigh # Should have failed to parse, but didn't.
48+
Required.*.ProtobufInput.BadTag_OverlongVarint # Should have failed to parse, but didn't.

conformance/failure_list_java_lite.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,6 @@
66

77
Required.*.ProtobufInput.PrematureEofInDelimitedDataForKnownNonRepeatedValue.MESSAGE # Should have failed to parse, but didn't.
88
Required.*.ProtobufInput.PrematureEofInDelimitedDataForKnownRepeatedValue.MESSAGE # Should have failed to parse, but didn't.
9+
Required.*.ProtobufInput.BadTag_FieldNumberTooHigh # Should have failed to parse, but didn't.
10+
Required.*.ProtobufInput.BadTag_FieldNumberSlightlyTooHigh # Should have failed to parse, but didn't.
11+
Required.*.ProtobufInput.BadTag_OverlongVarint # Should have failed to parse, but didn't.

conformance/failure_list_jruby.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,3 +144,6 @@ Required.Editions_Proto2.ProtobufInput.UnknownOrdering.ProtobufOutput
144144
Required.Editions_Proto3.ProtobufInput.UnknownOrdering.ProtobufOutput
145145
Required.Proto2.ProtobufInput.UnknownOrdering.ProtobufOutput
146146
Required.Proto3.ProtobufInput.UnknownOrdering.ProtobufOutput
147+
Required.*.ProtobufInput.BadTag_FieldNumberTooHigh # Should have failed to parse, but didn't.
148+
Required.*.ProtobufInput.BadTag_FieldNumberSlightlyTooHigh # Should have failed to parse, but didn't.
149+
Required.*.ProtobufInput.BadTag_OverlongVarint # Should have failed to parse, but didn't.

conformance/failure_list_objc.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,6 @@
11
# JSON input or output tests are skipped (in conformance_objc.m) as mobile
22
# platforms don't support JSON wire format to avoid code bloat.
3+
4+
Required.*.ProtobufInput.BadTag_FieldNumberTooHigh # Should have failed to parse, but didn't.
5+
Required.*.ProtobufInput.BadTag_FieldNumberSlightlyTooHigh # Should have failed to parse, but didn't.
6+
Required.*.ProtobufInput.BadTag_OverlongVarint # Should have failed to parse, but didn't.

conformance/failure_list_php.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,3 +45,7 @@ Required.*.DurationProtoNanosWrongSignNegativeSecs.JsonOutput # Should have fail
4545
Required.*.TimestampProtoNanoTooLarge.JsonOutput # Should have failed to serialize, but didn't.
4646
Required.*.TimestampProtoNegativeNanos.JsonOutput # Should have failed to serialize, but didn't.
4747
Required.*.JsonInput.SingleValueForRepeatedFieldInt32 # Should have failed to parse, but didn't.
48+
Required.*.ProtobufInput.BadTag_FieldNumberTooHigh # Should have failed to parse, but didn't.
49+
Required.*.ProtobufInput.BadTag_FieldNumberSlightlyTooHigh # Should have failed to parse, but didn't.
50+
Required.*.ProtobufInput.BadTag_OverlongVarint # Should have failed to parse, but didn't.
51+
Required.*.ProtobufInput.BadTag_VarintMoreThanTenBytes # Should have failed to parse, but didn't.
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,5 @@
11
Recommended.*.JsonInput.FieldNameDuplicateDifferentCasing1 # Should have failed to parse, but didn't.
22
Recommended.*.JsonInput.FieldNameDuplicateDifferentCasing2 # Should have failed to parse, but didn't.
3+
Required.*.ProtobufInput.BadTag_FieldNumberTooHigh # Should have failed to parse, but didn't.
4+
Required.*.ProtobufInput.BadTag_FieldNumberSlightlyTooHigh # Should have failed to parse, but didn't.
5+
Required.*.ProtobufInput.BadTag_OverlongVarint # Should have failed to parse, but didn't.

conformance/failure_list_python_cpp.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,4 @@
99

1010
Recommended.*.JsonInput.FieldNameDuplicateDifferentCasing1 # Should have failed to parse, but didn't.
1111
Recommended.*.JsonInput.FieldNameDuplicateDifferentCasing2 # Should have failed to parse, but didn't.
12+
Required.*.ProtobufInput.BadTag_FieldNumberSlightlyTooHigh # Should have failed to parse, but didn't.

0 commit comments

Comments
 (0)