Call Visitor::OnError() from HttpDecoder::RaiseError().
This is to make sure connection is closed on error.
This bug was caught by ClusterFuzz at https://crbug.com/989416.
gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99.
PiperOrigin-RevId: 262337299
Change-Id: Ib5241b0c53228dfe48356f5c03d9f4ffae499611
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc
index 550aa6f..e105fc6 100644
--- a/quic/core/http/http_decoder.cc
+++ b/quic/core/http/http_decoder.cc
@@ -126,7 +126,6 @@
if (current_frame_length_ > MaxFrameLength(current_frame_type_)) {
// TODO(bnc): Signal HTTP_EXCESSIVE_LOAD or similar to peer.
RaiseError(QUIC_INTERNAL_ERROR, "Frame is too large");
- visitor_->OnError(this);
return false;
}
@@ -440,6 +439,7 @@
state_ = STATE_ERROR;
error_ = error;
error_detail_ = std::move(error_detail);
+ visitor_->OnError(this);
}
bool HttpDecoder::ParsePriorityFrame(QuicDataReader* reader,
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc
index 476c2c6..7cbe949 100644
--- a/quic/core/http/http_decoder_test.cc
+++ b/quic/core/http/http_decoder_test.cc
@@ -404,6 +404,7 @@
HttpDecoder decoder(&visitor_);
EXPECT_CALL(visitor_, OnPriorityFrameStart(header_length));
+ EXPECT_CALL(visitor_, OnError(&decoder));
QuicByteCount processed_bytes =
decoder.ProcessInput(input.data(), input.size());
@@ -460,6 +461,40 @@
EXPECT_EQ("", decoder_.error_detail());
}
+TEST_F(HttpDecoderTest, CorruptSettingsFrame) {
+ const char* const kPayload =
+ "\x42\x11" // two-byte id
+ "\x80\x22\x33\x44" // four-byte value
+ "\x58\x39" // two-byte id
+ "\xf0\x22\x33\x44\x55\x66\x77\x88"; // eight-byte value
+ struct {
+ size_t payload_length;
+ const char* const error_message;
+ } kTestData[] = {
+ {1, "Unable to read settings frame identifier"},
+ {5, "Unable to read settings frame content"},
+ {7, "Unable to read settings frame identifier"},
+ {12, "Unable to read settings frame content"},
+ };
+
+ for (const auto& test_data : kTestData) {
+ std::string input;
+ input.push_back(4u); // type SETTINGS
+ input.push_back(test_data.payload_length);
+ const size_t header_length = input.size();
+ input.append(kPayload, test_data.payload_length);
+
+ HttpDecoder decoder(&visitor_);
+ EXPECT_CALL(visitor_, OnSettingsFrameStart(header_length));
+ EXPECT_CALL(visitor_, OnError(&decoder));
+
+ QuicByteCount processed_bytes =
+ decoder.ProcessInput(input.data(), input.size());
+ EXPECT_EQ(input.size(), processed_bytes);
+ EXPECT_EQ(QUIC_INTERNAL_ERROR, decoder.error());
+ EXPECT_EQ(test_data.error_message, decoder.error_detail());
+ }
+}
TEST_F(HttpDecoderTest, DataFrame) {
InSequence s;
std::string input(
@@ -792,6 +827,57 @@
EXPECT_EQ("", decoder_.error_detail());
}
+TEST_F(HttpDecoderTest, CorruptFrame) {
+ InSequence s;
+
+ struct {
+ const char* const input;
+ const char* const error_message;
+ } kTestData[] = {{"\x03" // type (CANCEL_PUSH)
+ "\x01" // length
+ "\x40", // first byte of two-byte varint push id
+ "Unable to read push_id"},
+ {"\x05" // type (PUSH_PROMISE)
+ "\x01" // length
+ "\x40", // first byte of two-byte varint push id
+ "Unable to read push_id"},
+ {"\x0D" // type (MAX_PUSH_ID)
+ "\x01" // length
+ "\x40", // first byte of two-byte varint push id
+ "Unable to read push_id"},
+ {"\x0E" // type (DUPLICATE_PUSH)
+ "\x01" // length
+ "\x40", // first byte of two-byte varint push id
+ "Unable to read push_id"},
+ {"\x07" // type (GOAWAY)
+ "\x01" // length
+ "\x40", // first byte of two-byte varint stream id
+ "Unable to read GOAWAY stream_id"}};
+
+ for (const auto& test_data : kTestData) {
+ {
+ HttpDecoder decoder(&visitor_);
+ EXPECT_CALL(visitor_, OnError(&decoder));
+
+ QuicStringPiece input(test_data.input);
+ decoder.ProcessInput(input.data(), input.size());
+ EXPECT_EQ(QUIC_INTERNAL_ERROR, decoder.error());
+ EXPECT_EQ(test_data.error_message, decoder.error_detail());
+ }
+ {
+ HttpDecoder decoder(&visitor_);
+ EXPECT_CALL(visitor_, OnError(&decoder));
+
+ QuicStringPiece input(test_data.input);
+ for (auto c : input) {
+ decoder.ProcessInput(&c, 1);
+ }
+ EXPECT_EQ(QUIC_INTERNAL_ERROR, decoder.error());
+ EXPECT_EQ(test_data.error_message, decoder.error_detail());
+ }
+ }
+} // namespace test
+
} // namespace test
} // namespace quic
