Do not clip error codes larger than QUIC_LAST_ERROR. When an incoming Google QUIC CONNECTION_CLOSE or GOAWAY frame has an error code larger than QUIC_LAST_ERROR, preserve the value instead of replacing it with QUIC_LAST_ERROR. This will make debugging of newly added error codes emitted by the server received by older versions of Chrome significantly little less confusing. IETF QUIC behavior is not affected: IETF CONNECTION_CLOSE can also carry a QuicErrorCode value in its description which is parsed out on the receiving side, but that did not suffer from being clipped to QUIC_LAST_ERROR. This CL adds a test for that behavior. IETF GOAWAY frame does not carry an error code. Protected by FLAGS_quic_reloadable_flag_quic_do_not_clip_received_error_code. PiperOrigin-RevId: 334160593 Change-Id: Ic830254c0dcb1cb6ca6687bae7a699df21b7e0e3
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc index 8b9d828..e57277c 100644 --- a/quic/core/quic_framer.cc +++ b/quic/core/quic_framer.cc
@@ -4058,7 +4058,8 @@ return false; } - if (error_code >= QUIC_LAST_ERROR) { + if (!GetQuicReloadableFlag(quic_do_not_clip_received_error_code) && + error_code >= QUIC_LAST_ERROR) { // Ignore invalid QUIC error code if any. error_code = QUIC_LAST_ERROR; } @@ -4086,7 +4087,8 @@ return false; } - if (error_code >= QUIC_LAST_ERROR) { + if (!GetQuicReloadableFlag(quic_do_not_clip_received_error_code) && + error_code >= QUIC_LAST_ERROR) { // Ignore invalid QUIC error code if any. error_code = QUIC_LAST_ERROR; }
diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc index 95e505b..4da3e03 100644 --- a/quic/core/quic_framer_test.cc +++ b/quic/core/quic_framer_test.cc
@@ -4398,8 +4398,9 @@ {"Unable to read connection close error details.", { // error details length - kVarInt62OneByte + 0x0d, - // error details + kVarInt62OneByte + 0x11, + // error details with QuicErrorCode serialized + '1', '1', '5', ':', 'b', 'e', 'c', 'a', 'u', 's', 'e', ' ', 'I', ' ', 'c', 'a', @@ -4429,8 +4430,7 @@ if (VersionHasIetfQuicFrames(framer_.transport_version())) { EXPECT_EQ(0x1234u, visitor_.connection_close_frame_.transport_close_frame_type); - EXPECT_THAT(visitor_.connection_close_frame_.quic_error_code, - IsError(QUIC_IETF_GQUIC_ERROR_MISSING)); + EXPECT_EQ(115u, visitor_.connection_close_frame_.quic_error_code); } else { // For Google QUIC frame, |quic_error_code| and |wire_error_code| has the // same value. @@ -4443,6 +4443,137 @@ CheckFramingBoundaries(fragments, QUIC_INVALID_CONNECTION_CLOSE_DATA); } +TEST_P(QuicFramerTest, ConnectionCloseFrameWithUnknownErrorCode) { + SetDecrypterLevel(ENCRYPTION_FORWARD_SECURE); + // clang-format off + PacketFragments packet = { + // public flags (8 byte connection_id) + {"", + {0x28}}, + // connection_id + {"", + {0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10}}, + // packet number + {"", + {0x12, 0x34, 0x56, 0x78}}, + // frame type (connection close frame) + {"", + {0x02}}, + // error code larger than QUIC_LAST_ERROR + {"Unable to read connection close error code.", + {0x00, 0x00, 0xC0, 0xDE}}, + {"Unable to read connection close error details.", + { + // error details length + 0x0, 0x0d, + // error details + 'b', 'e', 'c', 'a', + 'u', 's', 'e', ' ', + 'I', ' ', 'c', 'a', + 'n'} + } + }; + + PacketFragments packet46 = { + // type (short header, 4 byte packet number) + {"", + {0x43}}, + // connection_id + {"", + {0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10}}, + // packet number + {"", + {0x12, 0x34, 0x56, 0x78}}, + // frame type (connection close frame) + {"", + {0x02}}, + // error code larger than QUIC_LAST_ERROR + {"Unable to read connection close error code.", + {0x00, 0x00, 0xC0, 0xDE}}, + {"Unable to read connection close error details.", + { + // error details length + 0x0, 0x0d, + // error details + 'b', 'e', 'c', 'a', + 'u', 's', 'e', ' ', + 'I', ' ', 'c', 'a', + 'n'} + } + }; + + PacketFragments packet99 = { + // type (short header, 4 byte packet number) + {"", + {0x43}}, + // connection_id + {"", + {0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10}}, + // packet number + {"", + {0x12, 0x34, 0x56, 0x78}}, + // frame type (IETF Transport CONNECTION_CLOSE frame) + {"", + {0x1c}}, + // error code + {"Unable to read connection close error code.", + {kVarInt62FourBytes + 0x00, 0x00, 0xC0, 0xDE}}, + {"Unable to read connection close frame type.", + {kVarInt62TwoBytes + 0x12, 0x34 }}, + {"Unable to read connection close error details.", + { + // error details length + kVarInt62OneByte + 0x11, + // error details with QuicErrorCode larger than QUIC_LAST_ERROR + '8', '4', '9', ':', + 'b', 'e', 'c', 'a', + 'u', 's', 'e', ' ', + 'I', ' ', 'c', 'a', + 'n'} + } + }; + // clang-format on + + PacketFragments& fragments = + VersionHasIetfQuicFrames(framer_.transport_version()) + ? packet99 + : (framer_.version().HasIetfInvariantHeader() ? packet46 : packet); + std::unique_ptr<QuicEncryptedPacket> encrypted( + AssemblePacketFromFragments(fragments)); + EXPECT_TRUE(framer_.ProcessPacket(*encrypted)); + + EXPECT_THAT(framer_.error(), IsQuicNoError()); + ASSERT_TRUE(visitor_.header_.get()); + EXPECT_TRUE(CheckDecryption( + *encrypted, !kIncludeVersion, !kIncludeDiversificationNonce, + PACKET_8BYTE_CONNECTION_ID, PACKET_0BYTE_CONNECTION_ID)); + + EXPECT_EQ(0u, visitor_.stream_frames_.size()); + EXPECT_EQ("because I can", visitor_.connection_close_frame_.error_details); + if (VersionHasIetfQuicFrames(framer_.transport_version())) { + EXPECT_EQ(0x1234u, + visitor_.connection_close_frame_.transport_close_frame_type); + EXPECT_EQ(0xC0DEu, visitor_.connection_close_frame_.wire_error_code); + EXPECT_EQ(849u, visitor_.connection_close_frame_.quic_error_code); + } else { + // For Google QUIC frame, |quic_error_code| and |wire_error_code| has the + // same value. + if (GetQuicReloadableFlag(quic_do_not_clip_received_error_code)) { + EXPECT_EQ(0xC0DEu, visitor_.connection_close_frame_.wire_error_code); + EXPECT_EQ(0xC0DEu, visitor_.connection_close_frame_.quic_error_code); + } else { + EXPECT_EQ(QUIC_LAST_ERROR, + visitor_.connection_close_frame_.wire_error_code); + EXPECT_EQ(QUIC_LAST_ERROR, + visitor_.connection_close_frame_.quic_error_code); + } + } + + ASSERT_EQ(0u, visitor_.ack_frames_.size()); + + CheckFramingBoundaries(fragments, QUIC_INVALID_CONNECTION_CLOSE_DATA); +} + // As above, but checks that for Google-QUIC, if there happens // to be an ErrorCode string at the start of the details, it is // NOT extracted/parsed/folded/spindled/and/mutilated. @@ -4790,6 +4921,101 @@ CheckFramingBoundaries(fragments, QUIC_INVALID_GOAWAY_DATA); } +TEST_P(QuicFramerTest, GoAwayFrameWithUnknownErrorCode) { + if (VersionHasIetfQuicFrames(framer_.transport_version())) { + // This frame is not in IETF QUIC. + return; + } + SetDecrypterLevel(ENCRYPTION_FORWARD_SECURE); + // clang-format off + PacketFragments packet = { + // public flags (8 byte connection_id) + {"", + {0x28}}, + // connection_id + {"", + {0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10}}, + // packet number + {"", + {0x12, 0x34, 0x56, 0x78}}, + // frame type (go away frame) + {"", + {0x03}}, + // error code larger than QUIC_LAST_ERROR + {"Unable to read go away error code.", + {0x00, 0x00, 0xC0, 0xDE}}, + // stream id + {"Unable to read last good stream id.", + {0x01, 0x02, 0x03, 0x04}}, + // stream id + {"Unable to read goaway reason.", + { + // error details length + 0x0, 0x0d, + // error details + 'b', 'e', 'c', 'a', + 'u', 's', 'e', ' ', + 'I', ' ', 'c', 'a', + 'n'} + } + }; + + PacketFragments packet46 = { + // type (short header, 4 byte packet number) + {"", + {0x43}}, + // connection_id + {"", + {0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10}}, + // packet number + {"", + {0x12, 0x34, 0x56, 0x78}}, + // frame type (go away frame) + {"", + {0x03}}, + // error code larger than QUIC_LAST_ERROR + {"Unable to read go away error code.", + {0x00, 0x00, 0xC0, 0xDE}}, + // stream id + {"Unable to read last good stream id.", + {0x01, 0x02, 0x03, 0x04}}, + // stream id + {"Unable to read goaway reason.", + { + // error details length + 0x0, 0x0d, + // error details + 'b', 'e', 'c', 'a', + 'u', 's', 'e', ' ', + 'I', ' ', 'c', 'a', + 'n'} + } + }; + // clang-format on + + PacketFragments& fragments = + framer_.version().HasIetfInvariantHeader() ? packet46 : packet; + std::unique_ptr<QuicEncryptedPacket> encrypted( + AssemblePacketFromFragments(fragments)); + EXPECT_TRUE(framer_.ProcessPacket(*encrypted)); + + EXPECT_THAT(framer_.error(), IsQuicNoError()); + ASSERT_TRUE(visitor_.header_.get()); + EXPECT_TRUE(CheckDecryption( + *encrypted, !kIncludeVersion, !kIncludeDiversificationNonce, + PACKET_8BYTE_CONNECTION_ID, PACKET_0BYTE_CONNECTION_ID)); + + EXPECT_EQ(kStreamId, visitor_.goaway_frame_.last_good_stream_id); + if (GetQuicReloadableFlag(quic_do_not_clip_received_error_code)) { + EXPECT_EQ(0xC0DE, visitor_.goaway_frame_.error_code); + } else { + EXPECT_EQ(QUIC_LAST_ERROR, visitor_.goaway_frame_.error_code); + } + EXPECT_EQ("because I can", visitor_.goaway_frame_.reason_phrase); + + CheckFramingBoundaries(fragments, QUIC_INVALID_GOAWAY_DATA); +} + TEST_P(QuicFramerTest, WindowUpdateFrame) { if (VersionHasIetfQuicFrames(framer_.transport_version())) { // This frame is not in IETF QUIC, see MaxDataFrame and MaxStreamDataFrame