Consume unknown frames in QuicReceiveControlStream.
Note that consuming unknown frames in QuicSpdyStream will be much more
complicated because of the requirement to keep DATA frame payloads buffered by
the sequencer. It will come in a separate CL.
gfe-relnote: n/a, QUIC v99-only change.
PiperOrigin-RevId: 258548485
Change-Id: Id3ce05642b0ee3deb9bb98d0d574833c2c42d22d
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc
index 3388b2c..90df506 100644
--- a/quic/core/http/http_decoder.cc
+++ b/quic/core/http/http_decoder.cc
@@ -152,14 +152,34 @@
auto frame_meta = Http3FrameLengths(
current_length_field_length_ + current_type_field_length_,
current_frame_length_);
- if (current_frame_type_ == 0x0) {
- continue_processing = visitor_->OnDataFrameStart(frame_meta);
- } else if (current_frame_type_ == 0x1) {
- continue_processing = visitor_->OnHeadersFrameStart(frame_meta);
- } else if (current_frame_type_ == 0x4) {
- continue_processing = visitor_->OnSettingsFrameStart(frame_meta);
- } else if (current_frame_type_ == 0x2) {
- continue_processing = visitor_->OnPriorityFrameStart(frame_meta);
+
+ switch (current_frame_type_) {
+ case 0x00: // DATA
+ continue_processing = visitor_->OnDataFrameStart(frame_meta);
+ break;
+ case 0x01: // HEADERS
+ continue_processing = visitor_->OnHeadersFrameStart(frame_meta);
+ break;
+ case 0x02: // PRIORITY
+ continue_processing = visitor_->OnPriorityFrameStart(frame_meta);
+ break;
+ case 0x03: // CANCEL_PUSH
+ break;
+ case 0x04: // SETTINGS
+ continue_processing = visitor_->OnSettingsFrameStart(frame_meta);
+ break;
+ case 0x05: // PUSH_PROMISE
+ break;
+ case 0x07: // GOAWAY
+ break;
+ case 0x0d: // MAX_PUSH_ID
+ break;
+ case 0x0e: // DUPLICATE_PUSH
+ break;
+ default:
+ continue_processing =
+ visitor_->OnUnknownFrameStart(current_frame_type_, frame_meta);
+ break;
}
remaining_frame_length_ = current_frame_length_;
@@ -255,28 +275,17 @@
BufferFramePayload(reader);
break;
}
- // Reserved frame types.
- // TODO(rch): Since these are actually the same behavior as the
- // default, we probably don't need to special case them here?
- case 0xB:
- QUIC_FALLTHROUGH_INTENDED;
- case 0xB + 0x1F:
- QUIC_FALLTHROUGH_INTENDED;
- case 0xB + 0x1F * 2:
- QUIC_FALLTHROUGH_INTENDED;
- case 0xB + 0x1F * 3:
- QUIC_FALLTHROUGH_INTENDED;
- case 0xB + 0x1F * 4:
- QUIC_FALLTHROUGH_INTENDED;
- case 0xB + 0x1F * 5:
- QUIC_FALLTHROUGH_INTENDED;
- case 0xB + 0x1F * 6:
- QUIC_FALLTHROUGH_INTENDED;
- case 0xB + 0x1F * 7:
- QUIC_FALLTHROUGH_INTENDED;
- default:
- DiscardFramePayload(reader);
- return true;
+ default: {
+ QuicByteCount bytes_to_read = std::min<QuicByteCount>(
+ remaining_frame_length_, reader->BytesRemaining());
+ QuicStringPiece payload;
+ bool success = reader->ReadStringPiece(&payload, bytes_to_read);
+ DCHECK(success);
+ DCHECK(!payload.empty());
+ continue_processing = visitor_->OnUnknownFramePayload(payload);
+ remaining_frame_length_ -= payload.length();
+ break;
+ }
}
if (remaining_frame_length_ == 0) {
@@ -376,6 +385,10 @@
continue_processing = visitor_->OnDuplicatePushFrame(frame);
break;
}
+ default: {
+ continue_processing = visitor_->OnUnknownFrameEnd();
+ break;
+ }
}
current_length_field_length_ = 0;
diff --git a/quic/core/http/http_decoder.h b/quic/core/http/http_decoder.h
index b13af0e..e3d42ac 100644
--- a/quic/core/http/http_decoder.h
+++ b/quic/core/http/http_decoder.h
@@ -105,9 +105,17 @@
// Called when a PUSH_PROMISE frame has been completely processed.
virtual bool OnPushPromiseFrameEnd() = 0;
- // TODO(rch): Consider adding methods like:
- // OnUnknownFrame{Start,Payload,End}()
- // to allow callers to handle unknown frames.
+ // Called when a frame of unknown type |frame_type| has been received.
+ // Frame type might be reserved, Visitor must make sure to ignore.
+ // |frame_length| contains frame length and payload length.
+ virtual bool OnUnknownFrameStart(uint64_t frame_type,
+ Http3FrameLengths frame_length) = 0;
+ // Called when part of the payload of the unknown frame has been read. May
+ // be called multiple times for a single frame. |payload| is guaranteed to
+ // be non-empty.
+ virtual bool OnUnknownFramePayload(QuicStringPiece payload) = 0;
+ // Called when the unknown frame has been completely processed.
+ virtual bool OnUnknownFrameEnd() = 0;
};
// |visitor| must be non-null, and must outlive HttpDecoder.
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc
index 735f322..fbc167c 100644
--- a/quic/core/http/http_decoder_test.cc
+++ b/quic/core/http/http_decoder_test.cc
@@ -53,6 +53,10 @@
MOCK_METHOD1(OnPushPromiseFrameStart, bool(PushId push_id));
MOCK_METHOD1(OnPushPromiseFramePayload, bool(QuicStringPiece payload));
MOCK_METHOD0(OnPushPromiseFrameEnd, bool());
+
+ MOCK_METHOD2(OnUnknownFrameStart, bool(uint64_t, Http3FrameLengths));
+ MOCK_METHOD1(OnUnknownFramePayload, bool(QuicStringPiece));
+ MOCK_METHOD0(OnUnknownFrameEnd, bool());
};
class HttpDecoderTest : public QuicTest {
@@ -75,6 +79,9 @@
ON_CALL(visitor_, OnPushPromiseFrameStart(_)).WillByDefault(Return(true));
ON_CALL(visitor_, OnPushPromiseFramePayload(_)).WillByDefault(Return(true));
ON_CALL(visitor_, OnPushPromiseFrameEnd()).WillByDefault(Return(true));
+ ON_CALL(visitor_, OnUnknownFrameStart(_, _)).WillByDefault(Return(true));
+ ON_CALL(visitor_, OnUnknownFramePayload(_)).WillByDefault(Return(true));
+ ON_CALL(visitor_, OnUnknownFrameEnd()).WillByDefault(Return(true));
}
~HttpDecoderTest() override = default;
@@ -120,100 +127,47 @@
EXPECT_EQ("", decoder_.error_detail());
}
-TEST_F(HttpDecoderTest, ReservedFramesNoPayload) {
+TEST_F(HttpDecoderTest, UnknownFrame) {
std::unique_ptr<char[]> input;
- for (int n = 0; n < 8; ++n) {
- const uint8_t type = 0xB + 0x1F * n;
- QuicByteCount total_length = QuicDataWriter::GetVarInt62Len(0x00) +
- QuicDataWriter::GetVarInt62Len(type);
- input = QuicMakeUnique<char[]>(total_length);
- QuicDataWriter writer(total_length, input.get());
- writer.WriteVarInt62(type);
- writer.WriteVarInt62(0x00);
- 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, current_frame_type());
+ const QuicByteCount payload_lengths[] = {0, 14, 100};
+ const uint64_t frame_types[] = {
+ 0x21, 0x40, 0x5f, 0x7e, 0x9d, // some reserved frame types
+ 0x06, 0x0f, 0x14 // some unknown, not reserved frame types
+ };
+
+ for (auto payload_length : payload_lengths) {
+ std::string data(payload_length, 'a');
+
+ for (auto frame_type : frame_types) {
+ const QuicByteCount total_length =
+ QuicDataWriter::GetVarInt62Len(frame_type) +
+ QuicDataWriter::GetVarInt62Len(payload_length) + payload_length;
+ input = QuicMakeUnique<char[]>(total_length);
+
+ QuicDataWriter writer(total_length, input.get());
+ writer.WriteVarInt62(frame_type);
+ writer.WriteVarInt62(payload_length);
+ const QuicByteCount header_length = writer.length();
+ if (payload_length > 0) {
+ writer.WriteStringPiece(data);
+ }
+
+ EXPECT_CALL(visitor_, OnUnknownFrameStart(
+ frame_type, Http3FrameLengths(header_length,
+ payload_length)));
+ if (payload_length > 0) {
+ EXPECT_CALL(visitor_, OnUnknownFramePayload(data));
+ }
+ EXPECT_CALL(visitor_, OnUnknownFrameEnd());
+
+ EXPECT_EQ(total_length, decoder_.ProcessInput(input.get(), total_length));
+
+ EXPECT_EQ(QUIC_NO_ERROR, decoder_.error());
+ ASSERT_EQ("", decoder_.error_detail());
+ EXPECT_EQ(frame_type, current_frame_type());
+ }
}
- // Test on a arbitrary reserved frame with 2-byte type field by hard coding
- // variable length integer.
- char in[] = {// type 0xB + 0x1F*3
- 0x40, 0x68,
- // length
- 0x00};
- 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 * 3u, 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;
- 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(type);
- writer.WriteVarInt62(payload_size);
- 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, 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] = {// type 0xB + 0x1F*3
- 0x40, 0x68,
- // length
- payload_size};
- 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 * 3u, 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;
- 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(type);
- writer.WriteVarInt62(payload_size);
- 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, 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] = {// type 0xB + 0x1F*3
- 0x40, 0x68,
- // length
- 0x40 + 0x01, 0x00};
- 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 * 3u, current_frame_type());
}
TEST_F(HttpDecoderTest, CancelPush) {
@@ -583,24 +537,32 @@
}
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]);
+ // Use a reserved type that takes four bytes as a varint.
+ const uint64_t frame_type = 0x1f * 0x222 + 0x21;
+ const QuicByteCount payload_length = 0;
+ const QuicByteCount total_length =
+ QuicDataWriter::GetVarInt62Len(frame_type) +
+ QuicDataWriter::GetVarInt62Len(payload_length);
+
+ auto input = QuicMakeUnique<char[]>(total_length);
QuicDataWriter writer(total_length, input.get());
- writer.WriteVarInt62(type);
- writer.WriteVarInt62(0x00);
+ writer.WriteVarInt62(frame_type);
+ writer.WriteVarInt62(payload_length);
+
+ EXPECT_CALL(visitor_,
+ OnUnknownFrameStart(
+ frame_type, Http3FrameLengths(total_length, payload_length)));
+ EXPECT_CALL(visitor_, OnUnknownFrameEnd());
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, current_frame_type());
+ EXPECT_EQ(frame_type, current_frame_type());
}
TEST_F(HttpDecoderTest, GoAway) {
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc
index c01040b..66b7be6 100644
--- a/quic/core/http/quic_receive_control_stream.cc
+++ b/quic/core/http/quic_receive_control_stream.cc
@@ -119,6 +119,22 @@
return false;
}
+ bool OnUnknownFrameStart(uint64_t /* frame_type */,
+ Http3FrameLengths /* frame_length */) override {
+ // Ignore unknown frame types.
+ return true;
+ }
+
+ bool OnUnknownFramePayload(QuicStringPiece /* payload */) override {
+ // Ignore unknown frame types.
+ return true;
+ }
+
+ bool OnUnknownFrameEnd() override {
+ // Ignore unknown frame types.
+ return true;
+ }
+
private:
void CloseConnectionOnWrongFrame(std::string frame_type) {
// TODO(renjietang): Change to HTTP/3 error type.
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc
index 412ee0c..3601cff 100644
--- a/quic/core/http/quic_receive_control_stream_test.cc
+++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -6,6 +6,7 @@
#include "net/third_party/quiche/src/quic/core/quic_utils.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_ptr_util.h"
+#include "net/third_party/quiche/src/quic/platform/api/quic_text_utils.h"
#include "net/third_party/quiche/src/quic/test_tools/quic_spdy_session_peer.h"
#include "net/third_party/quiche/src/quic/test_tools/quic_stream_peer.h"
#include "net/third_party/quiche/src/quic/test_tools/quic_test_utils.h"
@@ -242,6 +243,22 @@
receive_control_stream_->OnStreamFrame(frame);
}
+// Regression test for b/137554973: unknown frames should be consumed.
+TEST_P(QuicReceiveControlStreamTest, ConsumeUnknownFrame) {
+ std::string unknown_frame = QuicTextUtils::HexDecode(
+ "21" // reserved frame type
+ "03" // payload length
+ "666f6f"); // payload "foo"
+
+ EXPECT_EQ(0u, NumBytesConsumed());
+
+ receive_control_stream_->OnStreamFrame(
+ QuicStreamFrame(receive_control_stream_->id(), /* fin = */ false,
+ /* offset = */ 0, unknown_frame));
+
+ EXPECT_EQ(unknown_frame.size(), NumBytesConsumed());
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index dd71302..55ae33f 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -82,6 +82,7 @@
}
bool OnDuplicatePushFrame(const DuplicatePushFrame& /*frame*/) override {
+ // TODO(b/137554973): Consume frame.
CloseConnectionOnWrongFrame("Duplicate Push");
return false;
}
@@ -123,11 +124,13 @@
}
bool OnPushPromiseFrameStart(PushId /*push_id*/) override {
+ // TODO(b/137554973): Consume frame header.
CloseConnectionOnWrongFrame("Push Promise");
return false;
}
bool OnPushPromiseFramePayload(QuicStringPiece payload) override {
+ // TODO(b/137554973): Consume frame payload.
DCHECK(!payload.empty());
CloseConnectionOnWrongFrame("Push Promise");
return false;
@@ -138,6 +141,19 @@
return false;
}
+ bool OnUnknownFrameStart(uint64_t /* frame_type */,
+ Http3FrameLengths /* frame_length */) override {
+ // TODO(b/137554973): Consume frame header.
+ return true;
+ }
+
+ bool OnUnknownFramePayload(QuicStringPiece /* payload */) override {
+ // TODO(b/137554973): Consume frame payload.
+ return true;
+ }
+
+ bool OnUnknownFrameEnd() override { return true; }
+
private:
void CloseConnectionOnWrongFrame(std::string frame_type) {
stream_->session()->connection()->CloseConnection(
