Adds a way to notify the `Http2Adapter` when the underlying codec decides not to send a frame. This will help prevent a potential leak of `SelfDeletingMetadataSource` objects. Protected by not protected; does not affect the GFE binary. PiperOrigin-RevId: 615204887
diff --git a/quiche/http2/adapter/http2_adapter.h b/quiche/http2/adapter/http2_adapter.h index 0ab0a72..817d4ea 100644 --- a/quiche/http2/adapter/http2_adapter.h +++ b/quiche/http2/adapter/http2_adapter.h
@@ -143,6 +143,11 @@ // the stream was successfully resumed. virtual bool ResumeStream(Http2StreamId stream_id) = 0; + // Called to communicate that a frame on a stream will not be sent. + // TODO(birenroy): remove when removing support for nghttp2. + virtual void FrameNotSent(Http2StreamId /*stream_id*/, + uint8_t /*frame_type*/) {} + protected: // Subclasses should expose a public factory method for constructing and // initializing (via Initialize()) adapter instances.
diff --git a/quiche/http2/adapter/nghttp2_adapter.cc b/quiche/http2/adapter/nghttp2_adapter.cc index f724ea3..ded8307 100644 --- a/quiche/http2/adapter/nghttp2_adapter.cc +++ b/quiche/http2/adapter/nghttp2_adapter.cc
@@ -19,11 +19,16 @@ using ConnectionError = Http2VisitorInterface::ConnectionError; -// A metadata source that deletes itself upon completion. -class SelfDeletingMetadataSource : public MetadataSource { +} // anonymous namespace + +// A metadata source that notifies the owning NgHttp2Adapter upon completion or +// failure. +class NgHttp2Adapter::NotifyingMetadataSource : public MetadataSource { public: - explicit SelfDeletingMetadataSource(std::unique_ptr<MetadataSource> source) - : source_(std::move(source)) {} + explicit NotifyingMetadataSource(NgHttp2Adapter* adapter, + Http2StreamId stream_id, + std::unique_ptr<MetadataSource> source) + : adapter_(adapter), stream_id_(stream_id), source_(std::move(source)) {} size_t NumFrames(size_t max_frame_size) const override { return source_->NumFrames(max_frame_size); @@ -32,22 +37,22 @@ std::pair<int64_t, bool> Pack(uint8_t* dest, size_t dest_len) override { const auto result = source_->Pack(dest, dest_len); if (result.first < 0 || result.second) { - delete this; + adapter_->RemovePendingMetadata(stream_id_); } return result; } void OnFailure() override { source_->OnFailure(); - delete this; + adapter_->RemovePendingMetadata(stream_id_); } private: + NgHttp2Adapter* const adapter_; + const Http2StreamId stream_id_; std::unique_ptr<MetadataSource> source_; }; -} // anonymous namespace - /* static */ std::unique_ptr<NgHttp2Adapter> NgHttp2Adapter::CreateClientAdapter( Http2VisitorInterface& visitor, const nghttp2_option* options) { @@ -128,13 +133,15 @@ void NgHttp2Adapter::SubmitMetadata(Http2StreamId stream_id, size_t max_frame_size, std::unique_ptr<MetadataSource> source) { - auto* wrapped_source = new SelfDeletingMetadataSource(std::move(source)); + auto wrapped_source = std::make_unique<NotifyingMetadataSource>( + this, stream_id, std::move(source)); const size_t num_frames = wrapped_source->NumFrames(max_frame_size); size_t num_successes = 0; for (size_t i = 1; i <= num_frames; ++i) { - const int result = nghttp2_submit_extension( - session_->raw_ptr(), kMetadataFrameType, - i == num_frames ? kMetadataEndFlag : 0, stream_id, wrapped_source); + const int result = + nghttp2_submit_extension(session_->raw_ptr(), kMetadataFrameType, + i == num_frames ? kMetadataEndFlag : 0, + stream_id, wrapped_source.get()); if (result != 0) { QUICHE_LOG(DFATAL) << "Failed to submit extension frame " << i << " of " << num_frames; @@ -142,8 +149,11 @@ } ++num_successes; } - if (num_successes == 0) { - delete wrapped_source; + if (num_successes > 0) { + // Finds the MetadataSourceVec for `stream_id` or inserts a new one if not + // present. + auto [it, _] = stream_metadata_.insert({stream_id, MetadataSourceVec{}}); + it->second.push_back(std::move(wrapped_source)); } } @@ -268,6 +278,12 @@ return 0 == nghttp2_session_resume_data(session_->raw_ptr(), stream_id); } +void NgHttp2Adapter::FrameNotSent(Http2StreamId stream_id, uint8_t frame_type) { + if (frame_type == kMetadataFrameType) { + RemovePendingMetadata(stream_id); + } +} + void NgHttp2Adapter::RemoveStream(Http2StreamId stream_id) { sources_.erase(stream_id); } @@ -305,5 +321,15 @@ options_ = nullptr; } +void NgHttp2Adapter::RemovePendingMetadata(Http2StreamId stream_id) { + auto it = stream_metadata_.find(stream_id); + if (it != stream_metadata_.end()) { + it->second.erase(it->second.begin()); + if (it->second.empty()) { + stream_metadata_.erase(it); + } + } +} + } // namespace adapter } // namespace http2
diff --git a/quiche/http2/adapter/nghttp2_adapter.h b/quiche/http2/adapter/nghttp2_adapter.h index 7ae9645..9337bb9 100644 --- a/quiche/http2/adapter/nghttp2_adapter.h +++ b/quiche/http2/adapter/nghttp2_adapter.h
@@ -4,6 +4,7 @@ #include <cstdint> #include "absl/container/flat_hash_map.h" +#include "absl/container/inlined_vector.h" #include "quiche/http2/adapter/http2_adapter.h" #include "quiche/http2/adapter/http2_protocol.h" #include "quiche/http2/adapter/nghttp2_session.h" @@ -87,13 +88,25 @@ bool ResumeStream(Http2StreamId stream_id) override; + void FrameNotSent(Http2StreamId stream_id, uint8_t frame_type) override; + // Removes references to the `stream_id` from this adapter. void RemoveStream(Http2StreamId stream_id); // Accessor for testing. size_t sources_size() const { return sources_.size(); } + size_t stream_metadata_size() const { return stream_metadata_.size(); } + size_t pending_metadata_count(Http2StreamId stream_id) const { + if (auto it = stream_metadata_.find(stream_id); + it != stream_metadata_.end()) { + return it->second.size(); + } + return 0; + } private: + class NotifyingMetadataSource; + NgHttp2Adapter(Http2VisitorInterface& visitor, Perspective perspective, const nghttp2_option* options); @@ -101,11 +114,18 @@ // such as preparing initial SETTINGS. void Initialize(); + void RemovePendingMetadata(Http2StreamId stream_id); + std::unique_ptr<NgHttp2Session> session_; Http2VisitorInterface& visitor_; const nghttp2_option* options_; Perspective perspective_; + using MetadataSourceVec = + absl::InlinedVector<std::unique_ptr<MetadataSource>, 2>; + using MetadataMap = absl::flat_hash_map<Http2StreamId, MetadataSourceVec>; + MetadataMap stream_metadata_; + absl::flat_hash_map<int32_t, std::unique_ptr<DataFrameSource>> sources_; };
diff --git a/quiche/http2/adapter/nghttp2_adapter_test.cc b/quiche/http2/adapter/nghttp2_adapter_test.cc index 27770f5..2990190 100644 --- a/quiche/http2/adapter/nghttp2_adapter_test.cc +++ b/quiche/http2/adapter/nghttp2_adapter_test.cc
@@ -2645,6 +2645,66 @@ EXPECT_FALSE(adapter->want_write()); } +TEST(NgHttp2AdapterTest, ClientSubmitMetadataWithGoaway) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); + + adapter->SubmitSettings({}); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, _, _, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, _, _, 0x0, 0)); + adapter->Send(); + + const std::vector<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); + + auto source = std::make_unique<TestMetadataSource>(ToHeaderBlock(ToHeaders( + {{"query-cost", "is too darn high"}, {"secret-sauce", "hollandaise"}}))); + adapter->SubmitMetadata(stream_id, 16384u, std::move(source)); + EXPECT_TRUE(adapter->want_write()); + + const std::string initial_frames = + TestFrameSequence() + .ServerPreface() + .GoAway(3, Http2ErrorCode::HTTP2_NO_ERROR, "server shutting down") + .Serialize(); + testing::InSequence s; + + // Server preface + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + EXPECT_CALL(visitor, OnFrameHeader(0, _, GOAWAY, 0)); + EXPECT_CALL(visitor, OnGoAway(3, Http2ErrorCode::HTTP2_NO_ERROR, _)); + + const int64_t initial_result = adapter->ProcessBytes(initial_frames); + EXPECT_EQ(initial_frames.size(), initial_result); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0)); + // HEADERS frame is not sent. + EXPECT_CALL(visitor, + OnBeforeFrameSent(kMetadataFrameType, stream_id, _, 0x4)); + EXPECT_CALL(visitor, OnFrameSent(kMetadataFrameType, stream_id, _, 0x4, 0)); + EXPECT_CALL(visitor, + OnCloseStream(stream_id, Http2ErrorCode::REFUSED_STREAM)); + + int result = adapter->Send(); + EXPECT_EQ(0, result); + absl::string_view serialized = visitor.data(); + EXPECT_THAT(serialized, + testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix)); + serialized.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix)); + EXPECT_THAT(serialized, + EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::SETTINGS, + static_cast<SpdyFrameType>(kMetadataFrameType)})); + EXPECT_FALSE(adapter->want_write()); +} + TEST(NgHttp2AdapterTest, ClientObeysMaxConcurrentStreams) { DataSavingVisitor visitor; auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); @@ -6614,6 +6674,84 @@ SpdyFrameType::RST_STREAM})); } +TEST(NgHttp2AdapterTest, ServerQueuesMetadataWithStreamReset) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); + + const std::string initial_frames = + TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}, + /*fin=*/false) + .Serialize(); + + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + + const int64_t initial_result = adapter->ProcessBytes(initial_frames); + EXPECT_EQ(static_cast<size_t>(initial_result), initial_frames.size()); + + auto body = std::make_unique<TestDataFrameSource>(visitor, true); + body->AppendPayload("Here is some data, which will be completely ignored!"); + + int submit_result = adapter->SubmitResponse( + 1, ToHeaders({{":status", "200"}}), std::move(body)); + ASSERT_EQ(0, submit_result); + + auto source = std::make_unique<TestMetadataSource>(ToHeaderBlock(ToHeaders( + {{"query-cost", "is too darn high"}, {"secret-sauce", "hollandaise"}}))); + adapter->SubmitMetadata(1, 16384u, std::move(source)); + adapter->SubmitWindowUpdate(1, 1024); + + const std::string reset_frame = + TestFrameSequence().RstStream(1, Http2ErrorCode::CANCEL).Serialize(); + + EXPECT_CALL(visitor, OnFrameHeader(1, _, RST_STREAM, 0x0)); + EXPECT_CALL(visitor, OnRstStream(1, Http2ErrorCode::CANCEL)); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::CANCEL)); + adapter->ProcessBytes(reset_frame); + + source = std::make_unique<TestMetadataSource>( + ToHeaderBlock(ToHeaders({{"really-important", "information!"}}))); + adapter->SubmitMetadata(1, 16384u, std::move(source)); + + EXPECT_EQ(1, adapter->stream_metadata_size()); + EXPECT_EQ(2, adapter->pending_metadata_count(1)); + + // Server initial SETTINGS and SETTINGS ack. + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + + // nghttp2 apparently allows extension frames to be sent on reset streams. + // The response HEADERS, DATA and WINDOW_UPDATE are all discarded. + EXPECT_CALL(visitor, OnBeforeFrameSent(kMetadataFrameType, 1, _, 0x4)); + EXPECT_CALL(visitor, OnFrameSent(kMetadataFrameType, 1, _, 0x4, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(kMetadataFrameType, 1, _, 0x4)); + EXPECT_CALL(visitor, OnFrameSent(kMetadataFrameType, 1, _, 0x4, 0)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), + EqualsFrames({SpdyFrameType::SETTINGS, + static_cast<SpdyFrameType>(kMetadataFrameType), + static_cast<SpdyFrameType>(kMetadataFrameType)})); + + EXPECT_EQ(0, adapter->stream_metadata_size()); + EXPECT_EQ(0, adapter->pending_metadata_count(1)); +} + TEST(NgHttp2AdapterTest, ServerStartsShutdown) { DataSavingVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);