Change HTTP/3 frame type from 1 byte integer to variable length integer, and allow partial delivery. gfe-relnote: Used in version 99 only. PiperOrigin-RevId: 244287295 Change-Id: Ic9a58f2822815006295f47f23f312b1f2c040f4e
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc index ec0062a..653652f 100644 --- a/quic/core/http/http_decoder.cc +++ b/quic/core/http/http_decoder.cc
@@ -24,8 +24,6 @@ return (flags >> offset) & GetMaskFromNumBits(num_bits); } -// Length of the type field of HTTP/3 frames. -static const QuicByteCount kFrameTypeLength = 1; // Length of the weight field of a priority frame. static const size_t kPriorityWeightLength = 1; // Length of a priority frame's first byte. @@ -41,6 +39,8 @@ remaining_length_field_length_(0), current_frame_length_(0), remaining_frame_length_(0), + current_type_field_length_(0), + remaining_type_field_length_(0), error_(QUIC_NO_ERROR), error_detail_("") {} @@ -97,9 +97,39 @@ void HttpDecoder::ReadFrameType(QuicDataReader* reader) { DCHECK_NE(0u, reader->BytesRemaining()); - if (!reader->ReadUInt8(¤t_frame_type_)) { - RaiseError(QUIC_INTERNAL_ERROR, "Unable to read frame type"); - return; + if (current_type_field_length_ == 0) { + // A new frame is coming. + current_type_field_length_ = reader->PeekVarInt62Length(); + if (current_type_field_length_ == 0) { + RaiseError(QUIC_INTERNAL_ERROR, "Unable to read frame type length"); + visitor_->OnError(this); + return; + } + if (current_type_field_length_ <= reader->BytesRemaining()) { + // The reader has all type data needed, so no need to buffer. + if (!reader->ReadVarInt62(¤t_frame_type_)) { + RaiseError(QUIC_INTERNAL_ERROR, "Unable to read frame type"); + return; + } + } else { + // Buffer a new type field. + remaining_type_field_length_ = current_type_field_length_; + BufferFrameType(reader); + return; + } + } else { + // Buffer the existing type field. + BufferFrameType(reader); + // The frame is still not buffered completely. + if (remaining_type_field_length_ != 0) { + return; + } + QuicDataReader type_reader(type_buffer_.data(), current_type_field_length_); + if (!type_reader.ReadVarInt62(¤t_frame_type_)) { + RaiseError(QUIC_INTERNAL_ERROR, "Unable to read buffered frame type"); + visitor_->OnError(this); + return; + } } if (current_frame_length_ > MaxFrameLength(current_frame_type_)) { @@ -112,13 +142,16 @@ // frame payload. if (current_frame_type_ == 0x0) { visitor_->OnDataFrameStart(Http3FrameLengths( - current_length_field_size_ + kFrameTypeLength, current_frame_length_)); + current_length_field_size_ + current_type_field_length_, + current_frame_length_)); } else if (current_frame_type_ == 0x1) { visitor_->OnHeadersFrameStart(Http3FrameLengths( - current_length_field_size_ + kFrameTypeLength, current_frame_length_)); + current_length_field_size_ + current_type_field_length_, + current_frame_length_)); } else if (current_frame_type_ == 0x4) { visitor_->OnSettingsFrameStart(Http3FrameLengths( - current_length_field_size_ + kFrameTypeLength, current_frame_length_)); + current_length_field_size_ + current_type_field_length_, + current_frame_length_)); } state_ = (remaining_frame_length_ == 0) ? STATE_FINISH_PARSING @@ -331,6 +364,7 @@ } current_length_field_size_ = 0; + current_type_field_length_ = 0; state_ = STATE_READING_FRAME_LENGTH; } @@ -346,6 +380,7 @@ if (remaining_frame_length_ == 0) { state_ = STATE_READING_FRAME_LENGTH; current_length_field_size_ = 0; + current_type_field_length_ = 0; } } @@ -391,6 +426,22 @@ remaining_length_field_length_ -= bytes_to_read; } +void HttpDecoder::BufferFrameType(QuicDataReader* reader) { + if (current_type_field_length_ == remaining_type_field_length_) { + type_buffer_.fill(0); + } + QuicByteCount bytes_to_read = std::min<QuicByteCount>( + remaining_type_field_length_, reader->BytesRemaining()); + if (!reader->ReadBytes(type_buffer_.data() + current_type_field_length_ - + remaining_type_field_length_, + bytes_to_read)) { + RaiseError(QUIC_INTERNAL_ERROR, "Unable to buffer frame type bytes."); + visitor_->OnError(this); + return; + } + remaining_type_field_length_ -= bytes_to_read; +} + void HttpDecoder::RaiseError(QuicErrorCode error, std::string error_detail) { state_ = STATE_ERROR; error_ = error;
diff --git a/quic/core/http/http_decoder.h b/quic/core/http/http_decoder.h index 55de8ad..8ed7973 100644 --- a/quic/core/http/http_decoder.h +++ b/quic/core/http/http_decoder.h
@@ -117,6 +117,7 @@ QuicErrorCode error() const { return error_; } const std::string& error_detail() const { return error_detail_; } + uint64_t current_frame_type() const { return current_frame_type_; } private: // Represents the current state of the parsing state machine. @@ -152,9 +153,12 @@ void BufferFramePayload(QuicDataReader* reader); // Buffers any remaining frame length field from |reader| into - // |length_buffer_| + // |length_buffer_|. void BufferFrameLength(QuicDataReader* reader); + // Buffers any remaining frame type field from |reader| into |type_buffer_|. + void BufferFrameType(QuicDataReader* reader); + // Sets |error_| and |error_detail_| accordingly. void RaiseError(QuicErrorCode error, std::string error_detail); @@ -172,7 +176,7 @@ // Current state of the parsing. HttpDecoderState state_; // Type of the frame currently being parsed. - uint8_t current_frame_type_; + uint64_t current_frame_type_; // Size of the frame's length field. QuicByteCount current_length_field_size_; // Remaining length that's needed for the frame's length field. @@ -181,6 +185,10 @@ QuicByteCount current_frame_length_; // Remaining payload bytes to be parsed. QuicByteCount remaining_frame_length_; + // Length of the frame's type field. + QuicByteCount current_type_field_length_; + // Remaining length that's needed for the frame's type field. + QuicByteCount remaining_type_field_length_; // Last error. QuicErrorCode error_; // The issue which caused |error_| @@ -189,6 +197,8 @@ std::string buffer_; // Remaining unparsed length field data. std::string length_buffer_; + // Remaining unparsed type field data. + std::array<char, sizeof(uint64_t)> type_buffer_; }; } // namespace quic
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc index 7f0dcd4..b0a3f6b 100644 --- a/quic/core/http/http_decoder_test.cc +++ b/quic/core/http/http_decoder_test.cc
@@ -7,6 +7,7 @@ #include "net/third_party/quiche/src/quic/core/http/http_encoder.h" #include "net/third_party/quiche/src/quic/core/quic_data_writer.h" #include "net/third_party/quiche/src/quic/platform/api/quic_arraysize.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_ptr_util.h" #include "net/third_party/quiche/src/quic/platform/api/quic_test.h" using testing::InSequence; @@ -54,51 +55,99 @@ } TEST_F(HttpDecoderTest, ReservedFramesNoPayload) { + std::unique_ptr<char[]> input; for (int n = 0; n < 8; ++n) { const uint8_t type = 0xB + 0x1F * n; - char input[] = {// length - 0x00, - // type - type}; + QuicByteCount total_length = QuicDataWriter::GetVarInt62Len(0x00) + + QuicDataWriter::GetVarInt62Len(type); + input = QuicMakeUnique<char[]>(total_length); + QuicDataWriter writer(total_length, input.get()); + writer.WriteVarInt62(0x00); + writer.WriteVarInt62(type); - EXPECT_EQ(2u, decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input))) << n; + EXPECT_EQ(total_length, decoder_.ProcessInput(input.get(), total_length)) + << n; EXPECT_EQ(QUIC_NO_ERROR, decoder_.error()); ASSERT_EQ("", decoder_.error_detail()); + EXPECT_EQ(type, decoder_.current_frame_type()); } + // Test on a arbitrary reserved frame with 2-byte type field by hard coding + // variable length integer. + char in[] = {// length + 0x00, + // type 0xB + 0x1F*3 + 0x40, 0x68}; + EXPECT_EQ(3u, decoder_.ProcessInput(in, QUIC_ARRAYSIZE(in))); + EXPECT_EQ(QUIC_NO_ERROR, decoder_.error()); + ASSERT_EQ("", decoder_.error_detail()); + EXPECT_EQ(0xB + 0x1F * 3, decoder_.current_frame_type()); } TEST_F(HttpDecoderTest, ReservedFramesSmallPayload) { + std::unique_ptr<char[]> input; + const uint8_t payload_size = 50; + std::string data(payload_size, 'a'); for (int n = 0; n < 8; ++n) { const uint8_t type = 0xB + 0x1F * n; - const uint8_t payload_size = 50; - char input[payload_size + 2] = {// length - payload_size, - // type - type}; - - EXPECT_EQ(QUIC_ARRAYSIZE(input), - decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input))) + QuicByteCount total_length = QuicDataWriter::GetVarInt62Len(payload_size) + + QuicDataWriter::GetVarInt62Len(type) + + payload_size; + input = QuicMakeUnique<char[]>(total_length); + QuicDataWriter writer(total_length, input.get()); + writer.WriteVarInt62(payload_size); + writer.WriteVarInt62(type); + writer.WriteStringPiece(data); + EXPECT_EQ(total_length, decoder_.ProcessInput(input.get(), total_length)) << n; EXPECT_EQ(QUIC_NO_ERROR, decoder_.error()); ASSERT_EQ("", decoder_.error_detail()); + EXPECT_EQ(type, decoder_.current_frame_type()); } + + // Test on a arbitrary reserved frame with 2-byte type field by hard coding + // variable length integer. + char in[payload_size + 3] = {// length + payload_size, + // type 0xB + 0x1F*3 + 0x40, 0x68}; + EXPECT_EQ(QUIC_ARRAYSIZE(in), decoder_.ProcessInput(in, QUIC_ARRAYSIZE(in))); + EXPECT_EQ(QUIC_NO_ERROR, decoder_.error()); + ASSERT_EQ("", decoder_.error_detail()); + EXPECT_EQ(0xB + 0x1F * 3, decoder_.current_frame_type()); } TEST_F(HttpDecoderTest, ReservedFramesLargePayload) { + std::unique_ptr<char[]> input; + const QuicByteCount payload_size = 256; + std::string data(payload_size, 'a'); for (int n = 0; n < 8; ++n) { const uint8_t type = 0xB + 0x1F * n; - const QuicByteCount payload_size = 256; - char input[payload_size + 3] = {// length - 0x40 + 0x01, 0x00, - // type - type}; + QuicByteCount total_length = QuicDataWriter::GetVarInt62Len(payload_size) + + QuicDataWriter::GetVarInt62Len(type) + + payload_size; + input = QuicMakeUnique<char[]>(total_length); + QuicDataWriter writer(total_length, input.get()); + writer.WriteVarInt62(payload_size); + writer.WriteVarInt62(type); + writer.WriteStringPiece(data); - EXPECT_EQ(QUIC_ARRAYSIZE(input), - decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input))) + EXPECT_EQ(total_length, decoder_.ProcessInput(input.get(), total_length)) << n; EXPECT_EQ(QUIC_NO_ERROR, decoder_.error()); ASSERT_EQ("", decoder_.error_detail()); + EXPECT_EQ(type, decoder_.current_frame_type()); } + + // Test on a arbitrary reserved frame with 2-byte type field by hard coding + // variable length integer. + char in[payload_size + 4] = {// length + 0x40 + 0x01, 0x00, + // type 0xB + 0x1F*3 + 0x40, 0x68}; + EXPECT_EQ(QUIC_ARRAYSIZE(in), decoder_.ProcessInput(in, QUIC_ARRAYSIZE(in))); + EXPECT_EQ(QUIC_NO_ERROR, decoder_.error()); + ASSERT_EQ("", decoder_.error_detail()); + EXPECT_EQ(0xB + 0x1F * 3, decoder_.current_frame_type()); } TEST_F(HttpDecoderTest, CancelPush) { @@ -352,6 +401,27 @@ EXPECT_EQ("", decoder_.error_detail()); } +TEST_F(HttpDecoderTest, PartialDeliveryOfLargeFrameType) { + // Use a reserved type that's more than 1 byte in VarInt62. + const uint8_t type = 0xB + 0x1F * 3; + std::unique_ptr<char[]> input; + QuicByteCount total_length = QuicDataWriter::GetVarInt62Len(0x00) + + QuicDataWriter::GetVarInt62Len(type); + input.reset(new char[total_length]); + QuicDataWriter writer(total_length, input.get()); + writer.WriteVarInt62(0x00); + writer.WriteVarInt62(type); + + auto raw_input = input.get(); + for (uint64_t i = 0; i < total_length; ++i) { + char c = raw_input[i]; + EXPECT_EQ(1u, decoder_.ProcessInput(&c, 1)); + } + EXPECT_EQ(QUIC_NO_ERROR, decoder_.error()); + EXPECT_EQ("", decoder_.error_detail()); + EXPECT_EQ(type, decoder_.current_frame_type()); +} + TEST_F(HttpDecoderTest, GoAway) { char input[] = {// length 0x1,
diff --git a/quic/core/http/http_encoder.cc b/quic/core/http/http_encoder.cc index beeef9b..df04be2 100644 --- a/quic/core/http/http_encoder.cc +++ b/quic/core/http/http_encoder.cc
@@ -44,8 +44,6 @@ } } -// Length of the type field of a frame. -static const size_t kFrameTypeLength = 1; // Length of the weight field of a priority frame. static const size_t kPriorityWeightLength = 1; // Length of a priority frame's first byte. @@ -63,8 +61,9 @@ QuicByteCount payload_length, std::unique_ptr<char[]>* output) { DCHECK_NE(0u, payload_length); - QuicByteCount header_length = - QuicDataWriter::GetVarInt62Len(payload_length) + kFrameTypeLength; + QuicByteCount header_length = QuicDataWriter::GetVarInt62Len(payload_length) + + QuicDataWriter::GetVarInt62Len( + static_cast<uint64_t>(HttpFrameType::DATA)); output->reset(new char[header_length]); QuicDataWriter writer(header_length, output->get()); @@ -80,7 +79,9 @@ std::unique_ptr<char[]>* output) { DCHECK_NE(0u, payload_length); QuicByteCount header_length = - QuicDataWriter::GetVarInt62Len(payload_length) + kFrameTypeLength; + QuicDataWriter::GetVarInt62Len(payload_length) + + QuicDataWriter::GetVarInt62Len( + static_cast<uint64_t>(HttpFrameType::HEADERS)); output->reset(new char[header_length]); QuicDataWriter writer(header_length, output->get()); @@ -99,7 +100,8 @@ QuicDataWriter::GetVarInt62Len(priority.prioritized_element_id) + QuicDataWriter::GetVarInt62Len(priority.element_dependency_id) + kPriorityWeightLength; - QuicByteCount total_length = GetTotalLength(payload_length); + QuicByteCount total_length = + GetTotalLength(payload_length, HttpFrameType::PRIORITY); output->reset(new char[total_length]); QuicDataWriter writer(total_length, output->get()); @@ -130,7 +132,8 @@ std::unique_ptr<char[]>* output) { QuicByteCount payload_length = QuicDataWriter::GetVarInt62Len(cancel_push.push_id); - QuicByteCount total_length = GetTotalLength(payload_length); + QuicByteCount total_length = + GetTotalLength(payload_length, HttpFrameType::CANCEL_PUSH); output->reset(new char[total_length]); QuicDataWriter writer(total_length, output->get()); @@ -152,7 +155,8 @@ payload_length += QuicDataWriter::GetVarInt62Len(it->second); } - QuicByteCount total_length = GetTotalLength(payload_length); + QuicByteCount total_length = + GetTotalLength(payload_length, HttpFrameType::SETTINGS); output->reset(new char[total_length]); QuicDataWriter writer(total_length, output->get()); @@ -178,7 +182,9 @@ push_promise.headers.length(); // GetTotalLength() is not used because headers will not be serialized. QuicByteCount total_length = - QuicDataWriter::GetVarInt62Len(payload_length) + kFrameTypeLength + + QuicDataWriter::GetVarInt62Len(payload_length) + + QuicDataWriter::GetVarInt62Len( + static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE)) + QuicDataWriter::GetVarInt62Len(push_promise.push_id); output->reset(new char[total_length]); @@ -196,7 +202,8 @@ std::unique_ptr<char[]>* output) { QuicByteCount payload_length = QuicDataWriter::GetVarInt62Len(goaway.stream_id); - QuicByteCount total_length = GetTotalLength(payload_length); + QuicByteCount total_length = + GetTotalLength(payload_length, HttpFrameType::GOAWAY); output->reset(new char[total_length]); QuicDataWriter writer(total_length, output->get()); @@ -213,7 +220,8 @@ std::unique_ptr<char[]>* output) { QuicByteCount payload_length = QuicDataWriter::GetVarInt62Len(max_push_id.push_id); - QuicByteCount total_length = GetTotalLength(payload_length); + QuicByteCount total_length = + GetTotalLength(payload_length, HttpFrameType::MAX_PUSH_ID); output->reset(new char[total_length]); QuicDataWriter writer(total_length, output->get()); @@ -230,7 +238,8 @@ std::unique_ptr<char[]>* output) { QuicByteCount payload_length = QuicDataWriter::GetVarInt62Len(duplicate_push.push_id); - QuicByteCount total_length = GetTotalLength(payload_length); + QuicByteCount total_length = + GetTotalLength(payload_length, HttpFrameType::DUPLICATE_PUSH); output->reset(new char[total_length]); QuicDataWriter writer(total_length, output->get()); @@ -247,11 +256,13 @@ HttpFrameType type, QuicDataWriter* writer) { return writer->WriteVarInt62(length) && - writer->WriteUInt8(static_cast<uint8_t>(type)); + writer->WriteVarInt62(static_cast<uint64_t>(type)); } -QuicByteCount HttpEncoder::GetTotalLength(QuicByteCount payload_length) { - return QuicDataWriter::GetVarInt62Len(payload_length) + kFrameTypeLength + +QuicByteCount HttpEncoder::GetTotalLength(QuicByteCount payload_length, + HttpFrameType type) { + return QuicDataWriter::GetVarInt62Len(payload_length) + + QuicDataWriter::GetVarInt62Len(static_cast<uint64_t>(type)) + payload_length; }
diff --git a/quic/core/http/http_encoder.h b/quic/core/http/http_encoder.h index f04e6e4..6691e31 100644 --- a/quic/core/http/http_encoder.h +++ b/quic/core/http/http_encoder.h
@@ -77,7 +77,8 @@ HttpFrameType type, QuicDataWriter* writer); - QuicByteCount GetTotalLength(QuicByteCount payload_length); + QuicByteCount GetTotalLength(QuicByteCount payload_length, + HttpFrameType type); }; } // namespace quic