Add HttpDecoder::OnPriorityFrameStart() so that QuicSpdyStream knows the size of the priority frame and can consume correct amount of bytes. gfe-relnote: v99 only, not in prod. PiperOrigin-RevId: 254279623 Change-Id: Ic24fd04b055033c0735a224e76a949714b3ef268
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc index 4513f04..179ab83 100644 --- a/quic/core/http/http_decoder.cc +++ b/quic/core/http/http_decoder.cc
@@ -160,27 +160,32 @@ return; } - // Calling the following two visitor methods does not require parsing of any + // Calling the following visitor methods does not require parsing of any // frame payload. + auto frame_meta = Http3FrameLengths( + current_length_field_length_ + current_type_field_length_, + current_frame_length_); if (current_frame_type_ == 0x0) { - if (!visitor_->OnDataFrameStart(Http3FrameLengths( - current_length_field_length_ + current_type_field_length_, - current_frame_length_))) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + if (!visitor_->OnDataFrameStart(frame_meta)) { + RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down on DATA frame start."); return; } } else if (current_frame_type_ == 0x1) { - if (!visitor_->OnHeadersFrameStart(Http3FrameLengths( - current_length_field_length_ + current_type_field_length_, - current_frame_length_))) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + if (!visitor_->OnHeadersFrameStart(frame_meta)) { + RaiseError(QUIC_INTERNAL_ERROR, + "Visitor shut down on HEADERS frame start."); return; } } else if (current_frame_type_ == 0x4) { - if (!visitor_->OnSettingsFrameStart(Http3FrameLengths( - current_length_field_length_ + current_type_field_length_, - current_frame_length_))) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + if (!visitor_->OnSettingsFrameStart(frame_meta)) { + RaiseError(QUIC_INTERNAL_ERROR, + "Visitor shut down on SETTINGS frame start."); + return; + } + } else if (current_frame_type_ == 0x2) { + if (!visitor_->OnPriorityFrameStart(frame_meta)) { + RaiseError(QUIC_INTERNAL_ERROR, + "Visitor shut down on PRIORITY frame start."); return; } } @@ -204,7 +209,7 @@ } DCHECK(!payload.empty()); if (!visitor_->OnDataFramePayload(payload)) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down on DATA frame."); return; } remaining_frame_length_ -= payload.length(); @@ -220,7 +225,7 @@ } DCHECK(!payload.empty()); if (!visitor_->OnHeadersFramePayload(payload)) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down on HEADERS frame."); return; } remaining_frame_length_ -= payload.length(); @@ -251,7 +256,8 @@ } remaining_frame_length_ -= bytes_remaining - reader->BytesRemaining(); if (!visitor_->OnPushPromiseFrameStart(push_id)) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, + "Visitor shut down on PUSH_PROMISE frame start."); return; } } @@ -268,7 +274,8 @@ } DCHECK(!payload.empty()); if (!visitor_->OnPushPromiseFramePayload(payload)) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, + "Visitor shut down on PUSH_PROMISE frame."); return; } remaining_frame_length_ -= payload.length(); @@ -323,14 +330,15 @@ switch (current_frame_type_) { case 0x0: { // DATA if (!visitor_->OnDataFrameEnd()) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down on DATA frame end."); return; } break; } case 0x1: { // HEADERS if (!visitor_->OnHeadersFrameEnd()) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, + "Visitor shut down on HEADERS frame end."); return; } break; @@ -344,7 +352,7 @@ return; } if (!visitor_->OnPriorityFrame(frame)) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down on PRIORITY frame."); return; } break; @@ -358,7 +366,8 @@ return; } if (!visitor_->OnCancelPushFrame(frame)) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, + "Visitor shut down on CANCEL_PUSH frame."); return; } break; @@ -370,14 +379,15 @@ return; } if (!visitor_->OnSettingsFrame(frame)) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down on SETTINGS frame."); return; } break; } case 0x5: { // PUSH_PROMISE if (!visitor_->OnPushPromiseFrameEnd()) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, + "Visitor shut down on PUSH_PROMISE frame end."); return; } break; @@ -396,7 +406,7 @@ } frame.stream_id = static_cast<QuicStreamId>(stream_id); if (!visitor_->OnGoAwayFrame(frame)) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down on GOAWAY frame."); return; } break; @@ -410,7 +420,8 @@ return; } if (!visitor_->OnMaxPushIdFrame(frame)) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, + "Visitor shut down on MAX_PUSH_ID frame."); return; } break; @@ -424,7 +435,8 @@ return; } if (!visitor_->OnDuplicatePushFrame(frame)) { - RaiseError(QUIC_INTERNAL_ERROR, "Visitor shut down."); + RaiseError(QUIC_INTERNAL_ERROR, + "Visitor shut down on DUPLICATE_PUSH frame."); return; } break;
diff --git a/quic/core/http/http_decoder.h b/quic/core/http/http_decoder.h index 9f0db73..0304b56 100644 --- a/quic/core/http/http_decoder.h +++ b/quic/core/http/http_decoder.h
@@ -40,6 +40,11 @@ // Called if an error is detected. virtual void OnError(HttpDecoder* decoder) = 0; + // Called when a PRIORITY frame has been received. + // |frame_length| contains PRIORITY frame length and payload length. + // Returns true to permit furthuring decoding, and false to prevent it. + virtual bool OnPriorityFrameStart(Http3FrameLengths frame_length) = 0; + // Called when a PRIORITY frame has been successfully parsed. // Returns true to permit furthuring decoding, and false to prevent it. virtual bool OnPriorityFrame(const PriorityFrame& frame) = 0;
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc index c25d086..b74918a 100644 --- a/quic/core/http/http_decoder_test.cc +++ b/quic/core/http/http_decoder_test.cc
@@ -23,6 +23,7 @@ // Called if an error is detected. MOCK_METHOD1(OnError, void(HttpDecoder* decoder)); + MOCK_METHOD1(OnPriorityFrameStart, bool(Http3FrameLengths frame_lengths)); MOCK_METHOD1(OnPriorityFrame, bool(const PriorityFrame& frame)); MOCK_METHOD1(OnCancelPushFrame, bool(const CancelPushFrame& frame)); MOCK_METHOD1(OnMaxPushIdFrame, bool(const MaxPushIdFrame& frame)); @@ -47,6 +48,7 @@ class HttpDecoderTest : public QuicTest { public: HttpDecoderTest() { + ON_CALL(visitor_, OnPriorityFrameStart(_)).WillByDefault(Return(true)); ON_CALL(visitor_, OnPriorityFrame(_)).WillByDefault(Return(true)); ON_CALL(visitor_, OnCancelPushFrame(_)).WillByDefault(Return(true)); ON_CALL(visitor_, OnMaxPushIdFrame(_)).WillByDefault(Return(true)); @@ -199,7 +201,7 @@ .WillOnce(Return(false)); EXPECT_EQ(0u, decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input))); EXPECT_EQ(QUIC_INTERNAL_ERROR, decoder_.error()); - EXPECT_EQ("Visitor shut down.", decoder_.error_detail()); + EXPECT_EQ("Visitor shut down on CANCEL_PUSH frame.", decoder_.error_detail()); } TEST_F(HttpDecoderTest, PushPromiseFrame) { @@ -242,7 +244,8 @@ EXPECT_CALL(visitor_, OnPushPromiseFrameStart(1)).WillOnce(Return(false)); EXPECT_EQ(0u, decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input))); EXPECT_EQ(QUIC_INTERNAL_ERROR, decoder_.error()); - EXPECT_EQ("Visitor shut down.", decoder_.error_detail()); + EXPECT_EQ("Visitor shut down on PUSH_PROMISE frame start.", + decoder_.error_detail()); } TEST_F(HttpDecoderTest, MaxPushId) { @@ -273,7 +276,7 @@ .WillOnce(Return(false)); EXPECT_EQ(0u, decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input))); EXPECT_EQ(QUIC_INTERNAL_ERROR, decoder_.error()); - EXPECT_EQ("Visitor shut down.", decoder_.error_detail()); + EXPECT_EQ("Visitor shut down on MAX_PUSH_ID frame.", decoder_.error_detail()); } TEST_F(HttpDecoderTest, DuplicatePush) { @@ -303,7 +306,8 @@ .WillOnce(Return(false)); EXPECT_EQ(0u, decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input))); EXPECT_EQ(QUIC_INTERNAL_ERROR, decoder_.error()); - EXPECT_EQ("Visitor shut down.", decoder_.error_detail()); + EXPECT_EQ("Visitor shut down on DUPLICATE_PUSH frame.", + decoder_.error_detail()); } TEST_F(HttpDecoderTest, PriorityFrame) { @@ -329,6 +333,7 @@ frame.weight = 0xFF; // Process the full frame. + EXPECT_CALL(visitor_, OnPriorityFrameStart(Http3FrameLengths(2, 4))); EXPECT_CALL(visitor_, OnPriorityFrame(frame)); EXPECT_EQ(QUIC_ARRAYSIZE(input), decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input))); @@ -336,6 +341,7 @@ EXPECT_EQ("", decoder_.error_detail()); // Process the frame incremently. + EXPECT_CALL(visitor_, OnPriorityFrameStart(Http3FrameLengths(2, 4))); EXPECT_CALL(visitor_, OnPriorityFrame(frame)); for (char c : input) { EXPECT_EQ(1u, decoder_.ProcessInput(&c, 1)); @@ -357,6 +363,7 @@ frame2.exclusive = true; frame2.weight = 0xFF; + EXPECT_CALL(visitor_, OnPriorityFrameStart(Http3FrameLengths(2, 2))); EXPECT_CALL(visitor_, OnPriorityFrame(frame2)); EXPECT_EQ(QUIC_ARRAYSIZE(input2), decoder_.ProcessInput(input2, QUIC_ARRAYSIZE(input2))); @@ -364,10 +371,11 @@ EXPECT_EQ("", decoder_.error_detail()); // Test on the situation when the visitor wants to stop processing. + EXPECT_CALL(visitor_, OnPriorityFrameStart(Http3FrameLengths(2, 4))); EXPECT_CALL(visitor_, OnPriorityFrame(frame)).WillOnce(Return(false)); EXPECT_EQ(0u, decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input))); EXPECT_EQ(QUIC_INTERNAL_ERROR, decoder_.error()); - EXPECT_EQ("Visitor shut down.", decoder_.error_detail()); + EXPECT_EQ("Visitor shut down on PRIORITY frame.", decoder_.error_detail()); } TEST_F(HttpDecoderTest, SettingsFrame) { @@ -419,7 +427,8 @@ .WillOnce(Return(false)); EXPECT_EQ(0u, decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input))); EXPECT_EQ(QUIC_INTERNAL_ERROR, decoder_.error()); - EXPECT_EQ("Visitor shut down.", decoder_.error_detail()); + EXPECT_EQ("Visitor shut down on SETTINGS frame start.", + decoder_.error_detail()); } TEST_F(HttpDecoderTest, DataFrame) { @@ -459,7 +468,7 @@ .WillOnce(Return(false)); EXPECT_EQ(0u, decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input))); EXPECT_EQ(QUIC_INTERNAL_ERROR, decoder_.error()); - EXPECT_EQ("Visitor shut down.", decoder_.error_detail()); + EXPECT_EQ("Visitor shut down on DATA frame start.", decoder_.error_detail()); } TEST_F(HttpDecoderTest, FrameHeaderPartialDelivery) {
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc index 986c4e4..e7efad1 100644 --- a/quic/core/http/quic_receive_control_stream.cc +++ b/quic/core/http/quic_receive_control_stream.cc
@@ -4,6 +4,7 @@ #include "net/third_party/quiche/src/quic/core/http/quic_receive_control_stream.h" +#include "net/third_party/quiche/src/quic/core/http/http_decoder.h" #include "net/third_party/quiche/src/quic/core/http/quic_spdy_session.h" namespace quic { @@ -24,6 +25,11 @@ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); } + bool OnPriorityFrameStart(Http3FrameLengths /*frame_lengths*/) override { + CloseConnectionOnWrongFrame("Priority"); + return false; + } + bool OnPriorityFrame(const PriorityFrame& /*frame*/) override { CloseConnectionOnWrongFrame("Priority"); return false;
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc index aa20771..02601ba 100644 --- a/quic/core/http/quic_spdy_stream.cc +++ b/quic/core/http/quic_spdy_stream.cc
@@ -8,6 +8,7 @@ #include <string> #include <utility> +#include "net/third_party/quiche/src/quic/core/http/http_decoder.h" #include "net/third_party/quiche/src/quic/core/http/quic_spdy_session.h" #include "net/third_party/quiche/src/quic/core/http/spdy_utils.h" #include "net/third_party/quiche/src/quic/core/qpack/qpack_decoded_headers_accumulator.h" @@ -45,6 +46,11 @@ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); } + bool OnPriorityFrameStart(Http3FrameLengths /*frame_lengths*/) override { + CloseConnectionOnWrongFrame("Priority"); + return false; + } + bool OnPriorityFrame(const PriorityFrame& /*frame*/) override { CloseConnectionOnWrongFrame("Priority"); return false;