Handle false return values from visitor_.OnMetadataEndForStream() in OgHttp2Session. This CL applies the same error-handling from visitor_.OnMetadataForStream() to visitor_.OnMetadataEndForStream(). Specifically, if the visitor callback fails, indicating a connection-level error, then OgHttp2Session saves the error and stops further processing on the connection. This change further aligns oghttp2 with nghttp2. With this CL, codec_impl_test BadMetadataVecReceivedTest passes with oghttp2: http://sponge2/d36f0333-8080-4e97-97fb-21dd3514caa1 This CL also performs some minor test changes. PiperOrigin-RevId: 417478489
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index 15d1e3a..4970f52 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -622,29 +622,26 @@ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS})); } -TEST(OgHttp2AdapterClientTest, ClientHandlesMetadataWithError) { +TEST(OgHttp2AdapterClientTest, ClientHandlesMetadataWithPayloadError) { DataSavingVisitor visitor; OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; auto adapter = OgHttp2Adapter::Create(visitor, options); testing::InSequence s; - const std::vector<const Header> headers1 = + const std::vector<const Header> headers = ToHeaders({{":method", "GET"}, {":scheme", "http"}, {":authority", "example.com"}, {":path", "/this/is/request/one"}}); - const char* kSentinel1 = "arbitrary pointer 1"; - const int32_t stream_id1 = - adapter->SubmitRequest(headers1, nullptr, const_cast<char*>(kSentinel1)); - ASSERT_GT(stream_id1, 0); - QUICHE_LOG(INFO) << "Created stream: " << stream_id1; + const int32_t stream_id = adapter->SubmitRequest(headers, nullptr, nullptr); + ASSERT_GT(stream_id, 0); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); - EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5)); - EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id, _, 0x5, 0)); int result = adapter->Send(); EXPECT_EQ(0, result); @@ -654,13 +651,13 @@ TestFrameSequence() .ServerPreface() .Metadata(0, "Example connection metadata") - .Headers(1, + .Headers(stream_id, {{":status", "200"}, {"server", "my-fake-server"}, {"date", "Tue, 6 Apr 2021 12:54:01 GMT"}}, /*fin=*/false) - .Metadata(1, "Example stream metadata") - .Data(1, "This is the response body.", true) + .Metadata(stream_id, "Example stream metadata") + .Data(stream_id, "This is the response body.", true) .Serialize(); // Server preface (empty SETTINGS) @@ -672,16 +669,86 @@ EXPECT_CALL(visitor, OnBeginMetadataForStream(0, _)); EXPECT_CALL(visitor, OnMetadataForStream(0, _)); EXPECT_CALL(visitor, OnMetadataEndForStream(0)); - EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); - EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); - EXPECT_CALL(visitor, OnHeaderForStream(1, ":status", "200")); - EXPECT_CALL(visitor, OnHeaderForStream(1, "server", "my-fake-server")); - EXPECT_CALL(visitor, - OnHeaderForStream(1, "date", "Tue, 6 Apr 2021 12:54:01 GMT")); - EXPECT_CALL(visitor, OnEndHeadersForStream(1)); - EXPECT_CALL(visitor, OnFrameHeader(1, _, kMetadataFrameType, 4)); - EXPECT_CALL(visitor, OnBeginMetadataForStream(1, _)); - EXPECT_CALL(visitor, OnMetadataForStream(1, _)) + EXPECT_CALL(visitor, OnFrameHeader(stream_id, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(stream_id)); + EXPECT_CALL(visitor, OnHeaderForStream(stream_id, _, _)).Times(3); + EXPECT_CALL(visitor, OnEndHeadersForStream(stream_id)); + EXPECT_CALL(visitor, OnFrameHeader(stream_id, _, kMetadataFrameType, 4)); + EXPECT_CALL(visitor, OnBeginMetadataForStream(stream_id, _)); + EXPECT_CALL(visitor, OnMetadataForStream(stream_id, _)) + .WillOnce(testing::Return(false)); + // Remaining frames are not processed due to the error. + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); + + const int64_t stream_result = adapter->ProcessBytes(stream_frames); + // Negative integer returned to indicate an error. + EXPECT_LT(stream_result, 0); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0)); + + EXPECT_FALSE(adapter->want_read()); + EXPECT_TRUE(adapter->want_write()); + result = adapter->Send(); + EXPECT_EQ(0, result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS})); +} + +TEST(OgHttp2AdapterClientTest, ClientHandlesMetadataWithCompletionError) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + testing::InSequence s; + + const std::vector<const Header> headers = + ToHeaders({{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}); + + const int32_t stream_id = adapter->SubmitRequest(headers, nullptr, nullptr); + ASSERT_GT(stream_id, 0); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id, _, 0x5, 0)); + + int result = adapter->Send(); + EXPECT_EQ(0, result); + visitor.Clear(); + + const std::string stream_frames = + TestFrameSequence() + .ServerPreface() + .Metadata(0, "Example connection metadata") + .Headers(stream_id, + {{":status", "200"}, + {"server", "my-fake-server"}, + {"date", "Tue, 6 Apr 2021 12:54:01 GMT"}}, + /*fin=*/false) + .Metadata(stream_id, "Example stream metadata") + .Data(stream_id, "This is the response body.", true) + .Serialize(); + + // Server preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + + EXPECT_CALL(visitor, OnFrameHeader(0, _, kMetadataFrameType, 4)); + EXPECT_CALL(visitor, OnBeginMetadataForStream(0, _)); + EXPECT_CALL(visitor, OnMetadataForStream(0, _)); + EXPECT_CALL(visitor, OnMetadataEndForStream(0)); + EXPECT_CALL(visitor, OnFrameHeader(stream_id, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(stream_id)); + EXPECT_CALL(visitor, OnHeaderForStream(stream_id, _, _)).Times(3); + EXPECT_CALL(visitor, OnEndHeadersForStream(stream_id)); + EXPECT_CALL(visitor, OnFrameHeader(stream_id, _, kMetadataFrameType, 4)); + EXPECT_CALL(visitor, OnBeginMetadataForStream(stream_id, _)); + EXPECT_CALL(visitor, OnMetadataForStream(stream_id, _)); + EXPECT_CALL(visitor, OnMetadataEndForStream(stream_id)) .WillOnce(testing::Return(false)); // Remaining frames are not processed due to the error. EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError));
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index af843e8..0357f7b 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -1300,12 +1300,17 @@ void OgHttp2Session::OnFramePayload(const char* data, size_t len) { if (metadata_length_ > 0) { QUICHE_DCHECK_LE(len, metadata_length_); - const bool success = visitor_.OnMetadataForStream( + const bool payload_success = visitor_.OnMetadataForStream( metadata_stream_id_, absl::string_view(data, len)); - if (success) { + if (payload_success) { metadata_length_ -= len; if (metadata_length_ == 0 && end_metadata_) { - visitor_.OnMetadataEndForStream(metadata_stream_id_); + const bool completion_success = + visitor_.OnMetadataEndForStream(metadata_stream_id_); + if (!completion_success) { + fatal_visitor_callback_failure_ = true; + decoder_.StopProcessing(); + } metadata_stream_id_ = 0; end_metadata_ = false; }