Adds support for sending and receiving the SETTINGS_HEADER_TABLE_SIZE setting in oghttp2. There is some danger in HPACK limit skew between endpoints, depending on when new limits are applied. For that reason, this change uses the following strategy. * When sending the setting, the library applies the new limit upon receipt of an acknowledgement of the setting from the peer. * When receiving the setting, the library applies the limit when sending an acknowledgement to the peer. PiperOrigin-RevId: 413999148
diff --git a/http2/adapter/http2_protocol.cc b/http2/adapter/http2_protocol.cc index 8b80718..0f3c0d7 100644 --- a/http2/adapter/http2_protocol.cc +++ b/http2/adapter/http2_protocol.cc
@@ -25,6 +25,10 @@ } } +bool operator==(const Http2Setting& a, const Http2Setting& b) { + return a.id == b.id && a.value == b.value; +} + absl::string_view Http2SettingsIdToString(uint16_t id) { switch (id) { case Http2KnownSettingsId::HEADER_TABLE_SIZE:
diff --git a/http2/adapter/http2_protocol.h b/http2/adapter/http2_protocol.h index ce5a282..0529bd6 100644 --- a/http2/adapter/http2_protocol.h +++ b/http2/adapter/http2_protocol.h
@@ -39,6 +39,8 @@ uint32_t value; }; +bool operator==(const Http2Setting& a, const Http2Setting& b); + // The maximum possible stream ID. const Http2StreamId kMaxStreamId = 0x7FFFFFFF;
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index 3f25f2b..ef5ec8f 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -557,6 +557,50 @@ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS})); } +TEST(NgHttp2AdapterTest, ClientHandlesHpackHeaderTableSetting) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); + + testing::InSequence s; + + const std::vector<const Header> headers1 = ToHeaders({ + {":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}, + {"x-i-do-not-like", "green eggs and ham"}, + {"x-i-will-not-eat-them", "here or there, in a box, with a fox"}, + {"x-like-them-in-a-house", "no"}, + {"x-like-them-with-a-mouse", "no"}, + }); + + const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr); + ASSERT_GT(stream_id1, 0); + + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0)); + + int result = adapter->Send(); + EXPECT_EQ(0, result); + visitor.Clear(); + + EXPECT_GT(adapter->GetHpackEncoderDynamicTableSize(), 100); + + const std::string stream_frames = + TestFrameSequence().Settings({{HEADER_TABLE_SIZE, 100u}}).Serialize(); + // Server preface (SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 6, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSetting(Http2Setting{HEADER_TABLE_SIZE, 100u})); + + EXPECT_CALL(visitor, OnSettingsEnd()); + + const int64_t stream_result = adapter->ProcessBytes(stream_frames); + EXPECT_EQ(stream_frames.size(), stream_result); + + EXPECT_LE(adapter->GetHpackEncoderDynamicTableSize(), 100); +} + TEST(NgHttp2AdapterTest, ClientHandlesInvalidTrailers) { DataSavingVisitor visitor; auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor);
diff --git a/http2/adapter/oghttp2_adapter.cc b/http2/adapter/oghttp2_adapter.cc index c4c2e7b..3847ef8 100644 --- a/http2/adapter/oghttp2_adapter.cc +++ b/http2/adapter/oghttp2_adapter.cc
@@ -103,10 +103,18 @@ return session_->GetHpackEncoderDynamicTableSize(); } +int OgHttp2Adapter::GetHpackEncoderDynamicTableCapacity() const { + return session_->GetHpackEncoderDynamicTableCapacity(); +} + int OgHttp2Adapter::GetHpackDecoderDynamicTableSize() const { return session_->GetHpackDecoderDynamicTableSize(); } +int OgHttp2Adapter::GetHpackDecoderSizeLimit() const { + return session_->GetHpackDecoderSizeLimit(); +} + Http2StreamId OgHttp2Adapter::GetHighestReceivedStreamId() const { return session_->GetHighestReceivedStreamId(); }
diff --git a/http2/adapter/oghttp2_adapter.h b/http2/adapter/oghttp2_adapter.h index 0aae5ab..d30ed19 100644 --- a/http2/adapter/oghttp2_adapter.h +++ b/http2/adapter/oghttp2_adapter.h
@@ -46,7 +46,9 @@ int GetStreamReceiveWindowSize(Http2StreamId stream_id) const override; int GetReceiveWindowSize() const override; int GetHpackEncoderDynamicTableSize() const override; + int GetHpackEncoderDynamicTableCapacity() const; int GetHpackDecoderDynamicTableSize() const override; + int GetHpackDecoderSizeLimit() const; Http2StreamId GetHighestReceivedStreamId() const override; void MarkDataConsumedForStream(Http2StreamId stream_id, size_t num_bytes) override;
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index 86d5f72..88ebbed 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -870,6 +870,230 @@ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS})); } +TEST(OgHttp2AdapterClientTest, ClientHandlesSmallerHpackHeaderTableSetting) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + testing::InSequence s; + + const std::vector<const Header> headers1 = ToHeaders({ + {":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}, + {"x-i-do-not-like", "green eggs and ham"}, + {"x-i-will-not-eat-them", "here or there, in a box, with a fox"}, + {"x-like-them-in-a-house", "no"}, + {"x-like-them-with-a-mouse", "no"}, + }); + + const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr); + ASSERT_GT(stream_id1, 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)); + + int result = adapter->Send(); + EXPECT_EQ(0, result); + visitor.Clear(); + + EXPECT_GT(adapter->GetHpackEncoderDynamicTableSize(), 100); + + const std::string stream_frames = + TestFrameSequence().Settings({{HEADER_TABLE_SIZE, 100u}}).Serialize(); + // Server preface (SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 6, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSetting(Http2Setting{HEADER_TABLE_SIZE, 100u})); + // Duplicate setting callback due to the way extensions work. + EXPECT_CALL(visitor, OnSetting(Http2Setting{HEADER_TABLE_SIZE, 100u})); + EXPECT_CALL(visitor, OnSettingsEnd()); + + const int64_t stream_result = adapter->ProcessBytes(stream_frames); + EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); + + EXPECT_EQ(adapter->GetHpackEncoderDynamicTableCapacity(), 100); + EXPECT_LE(adapter->GetHpackEncoderDynamicTableSize(), 100); +} + +TEST(OgHttp2AdapterClientTest, ClientHandlesLargerHpackHeaderTableSetting) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + testing::InSequence s; + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + + int result = adapter->Send(); + EXPECT_EQ(0, result); + visitor.Clear(); + + EXPECT_EQ(adapter->GetHpackEncoderDynamicTableCapacity(), 4096); + + const std::string stream_frames = + TestFrameSequence().Settings({{HEADER_TABLE_SIZE, 40960u}}).Serialize(); + // Server preface (SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 6, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSetting(Http2Setting{HEADER_TABLE_SIZE, 40960u})); + // Duplicate setting callback due to the way extensions work. + EXPECT_CALL(visitor, OnSetting(Http2Setting{HEADER_TABLE_SIZE, 40960u})); + EXPECT_CALL(visitor, OnSettingsEnd()); + + const int64_t stream_result = adapter->ProcessBytes(stream_frames); + EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); + + // The increased capacity will not be applied until a SETTINGS ack is + // serialized. + EXPECT_EQ(adapter->GetHpackEncoderDynamicTableCapacity(), 4096); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0)); + + result = adapter->Send(); + EXPECT_EQ(0, result); + visitor.Clear(); + + EXPECT_EQ(adapter->GetHpackEncoderDynamicTableCapacity(), 40960); +} + +TEST(OgHttp2AdapterClientTest, ClientSendsHpackHeaderTableSetting) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + testing::InSequence s; + + const std::vector<const Header> headers1 = ToHeaders({ + {":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}, + }); + + const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr); + ASSERT_GT(stream_id1, 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)); + + int result = adapter->Send(); + EXPECT_EQ(0, result); + visitor.Clear(); + + const std::string stream_frames = + TestFrameSequence() + .ServerPreface() + .SettingsAck() + .Headers( + 1, + {{":status", "200"}, + {"server", "my-fake-server"}, + {"date", "Tue, 6 Apr 2021 12:54:01 GMT"}, + {"x-i-do-not-like", "green eggs and ham"}, + {"x-i-will-not-eat-them", "here or there, in a box, with a fox"}, + {"x-like-them-in-a-house", "no"}, + {"x-like-them-with-a-mouse", "no"}}, + /*fin=*/true) + .Serialize(); + // Server preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + // Server acks client's initial SETTINGS. + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 1)); + EXPECT_CALL(visitor, OnSettingsAck()); + + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(7); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnEndStream(1)); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR)); + + const int64_t stream_result = adapter->ProcessBytes(stream_frames); + EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); + + EXPECT_GT(adapter->GetHpackDecoderSizeLimit(), 100); + + // Submit settings, check decoder table size. + adapter->SubmitSettings({{HEADER_TABLE_SIZE, 100u}}); + EXPECT_GT(adapter->GetHpackDecoderSizeLimit(), 100); + + // Server preface SETTINGS ack + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0)); + // SETTINGS with the new header table size value + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 6, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 6, 0x0, 0)); + + // Because the client has not yet seen an ack from the server for the SETTINGS + // with header table size, it has not applied the new value. + EXPECT_GT(adapter->GetHpackDecoderSizeLimit(), 100); + + result = adapter->Send(); + EXPECT_EQ(0, result); + visitor.Clear(); + + const std::vector<const Header> headers2 = ToHeaders({ + {":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/two"}, + }); + + const int32_t stream_id2 = adapter->SubmitRequest(headers2, nullptr, nullptr); + ASSERT_GT(stream_id2, stream_id1); + + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id2, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id2, _, 0x5, 0)); + + result = adapter->Send(); + EXPECT_EQ(0, result); + visitor.Clear(); + + const std::string response_frames = + TestFrameSequence() + .Headers(stream_id2, + {{":status", "200"}, + {"server", "my-fake-server"}, + {"date", "Tue, 6 Apr 2021 12:54:01 GMT"}}, + /*fin=*/true) + .Serialize(); + + EXPECT_CALL(visitor, OnFrameHeader(stream_id2, _, HEADERS, 5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(stream_id2)); + EXPECT_CALL(visitor, OnHeaderForStream(stream_id2, _, _)).Times(3); + EXPECT_CALL(visitor, OnEndHeadersForStream(stream_id2)); + EXPECT_CALL(visitor, OnEndStream(stream_id2)); + EXPECT_CALL(visitor, + OnCloseStream(stream_id2, Http2ErrorCode::HTTP2_NO_ERROR)); + + const int64_t response_result = adapter->ProcessBytes(response_frames); + EXPECT_EQ(response_frames.size(), static_cast<size_t>(response_result)); + + // Still no ack for the outbound settings. + EXPECT_GT(adapter->GetHpackDecoderSizeLimit(), 100); + + const std::string settings_ack = + TestFrameSequence().SettingsAck().Serialize(); + + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 1)); + EXPECT_CALL(visitor, OnSettingsAck()); + + const int64_t ack_result = adapter->ProcessBytes(settings_ack); + EXPECT_EQ(settings_ack.size(), static_cast<size_t>(ack_result)); + // Ack has finally arrived. + EXPECT_EQ(adapter->GetHpackDecoderSizeLimit(), 100); +} + // TODO(birenroy): Validate headers and re-enable this test. The library should // invoke OnErrorDebug() with an error message for the invalid header. The // library should also invoke OnInvalidFrame() for the invalid HEADERS frame.
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index fb37bdd..c54d335 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -30,6 +30,8 @@ #endif const uint32_t kMaxAllowedMetadataFrameSize = 65536u; +const uint32_t kDefaultHpackTableCapacity = 4096u; +const uint32_t kMaximumHpackTableCapacity = 65536u; // TODO(birenroy): Consider incorporating spdy::FlagsSerializionVisitor here. class FrameAttributeCollector : public spdy::SpdyFrameVisitor { @@ -195,6 +197,13 @@ return status.size() == 3 && status[0] == '1'; } +// Returns the upper bound on HPACK encoder table capacity. If not specified in +// the Options, a reasonable default upper bound is used. +uint32_t HpackCapacityBound(const OgHttp2Session::Options& o) { + return o.max_hpack_encoding_table_capacity.value_or( + kMaximumHpackTableCapacity); +} + } // namespace void OgHttp2Session::PassthroughHeadersHandler::OnHeaderBlockStart() { @@ -334,11 +343,22 @@ return encoder == nullptr ? 0 : encoder->GetDynamicTableSize(); } +int OgHttp2Session::GetHpackEncoderDynamicTableCapacity() const { + const spdy::HpackEncoder* encoder = framer_.GetHpackEncoder(); + return encoder == nullptr ? kDefaultHpackTableCapacity + : encoder->CurrentHeaderTableSizeSetting(); +} + int OgHttp2Session::GetHpackDecoderDynamicTableSize() const { const spdy::HpackDecoderAdapter* decoder = decoder_.GetHpackDecoder(); return decoder == nullptr ? 0 : decoder->GetDynamicTableSize(); } +int OgHttp2Session::GetHpackDecoderSizeLimit() const { + const spdy::HpackDecoderAdapter* decoder = decoder_.GetHpackDecoder(); + return decoder == nullptr ? 0 : decoder->GetCurrentHeaderTableSizeSetting(); +} + int64_t OgHttp2Session::ProcessBytes(absl::string_view bytes) { QUICHE_VLOG(2) << TracePerspectiveAsString(options_.perspective) << " processing [" << absl::CEscape(bytes) << "]"; @@ -528,6 +548,14 @@ visitor_.OnFrameSent(frame_type, stream_id, payload_length, flags, error_code); if (stream_id == 0) { + const bool is_settings_ack = + static_cast<FrameType>(frame_type) == FrameType::SETTINGS && + (flags & 0x01); + if (is_settings_ack && encoder_header_table_capacity_when_acking_) { + framer_.UpdateHeaderEncoderTableSize( + encoder_header_table_capacity_when_acking_.value()); + encoder_header_table_capacity_when_acking_ = absl::nullopt; + } return; } auto iter = queued_frames_.find(stream_id); @@ -906,6 +934,9 @@ void OgHttp2Session::OnSettings() { visitor_.OnSettingsStart(); + auto settings = absl::make_unique<SpdySettingsIR>(); + settings->set_is_ack(true); + EnqueueFrame(std::move(settings)); } void OgHttp2Session::OnSetting(spdy::SpdySettingsId id, uint32_t value) { @@ -916,14 +947,25 @@ max_frame_payload_ = value; } else if (id == MAX_CONCURRENT_STREAMS) { max_outbound_concurrent_streams_ = value; + } else if (id == HEADER_TABLE_SIZE) { + value = std::min(value, HpackCapacityBound(options_)); + if (value < framer_.GetHpackEncoder()->CurrentHeaderTableSizeSetting()) { + // Safe to apply a smaller table capacity immediately. + QUICHE_VLOG(2) << TracePerspectiveAsString(options_.perspective) + << " applying encoder table capacity " << value; + framer_.GetHpackEncoder()->ApplyHeaderTableSizeSetting(value); + } else { + QUICHE_VLOG(2) + << TracePerspectiveAsString(options_.perspective) + << " NOT applying encoder table capacity until writing ack: " + << value; + encoder_header_table_capacity_when_acking_ = value; + } } } void OgHttp2Session::OnSettingsEnd() { visitor_.OnSettingsEnd(); - auto settings = absl::make_unique<SpdySettingsIR>(); - settings->set_is_ack(true); - EnqueueFrame(std::move(settings)); } void OgHttp2Session::OnSettingsAck() { @@ -1169,6 +1211,9 @@ for (const auto id_and_value : settings_map) { if (id_and_value.first == spdy::SETTINGS_MAX_CONCURRENT_STREAMS) { max_inbound_concurrent_streams_ = id_and_value.second; + } else if (id_and_value.first == spdy::SETTINGS_HEADER_TABLE_SIZE) { + decoder_.GetHpackDecoder()->ApplyHeaderTableSizeSetting( + id_and_value.second); } } });
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h index 58f34ec..9f00613 100644 --- a/http2/adapter/oghttp2_session.h +++ b/http2/adapter/oghttp2_session.h
@@ -8,6 +8,7 @@ #include <vector> #include "absl/strings/string_view.h" +#include "absl/types/optional.h" #include "http2/adapter/data_source.h" #include "http2/adapter/event_forwarder.h" #include "http2/adapter/header_validator.h" @@ -37,6 +38,8 @@ public: struct QUICHE_EXPORT_PRIVATE Options { Perspective perspective = Perspective::kClient; + // The maximum HPACK table size to use. + absl::optional<size_t> max_hpack_encoding_table_capacity = absl::nullopt; // Whether to automatically send PING acks when receiving a PING. bool auto_ping_ack = true; // Whether (as server) to send a RST_STREAM NO_ERROR when sending a fin on @@ -102,10 +105,16 @@ // per-entry overhead from the specification. int GetHpackEncoderDynamicTableSize() const; + // Returns the maximum capacity of the HPACK encoder's dynamic table. + int GetHpackEncoderDynamicTableCapacity() const; + // Returns the size of the HPACK decoder's dynamic table, including the // per-entry overhead from the specification. int GetHpackDecoderDynamicTableSize() const; + // Returns the size of the HPACK decoder's most recently applied size limit. + int GetHpackDecoderSizeLimit() const; + // From Http2Session. int64_t ProcessBytes(absl::string_view bytes) override; int Consume(Http2StreamId stream_id, size_t num_bytes) override; @@ -406,6 +415,13 @@ uint32_t max_inbound_concurrent_streams_ = std::numeric_limits<uint32_t>::max(); Options options_; + + // The HPACK encoder header table capacity that will be applied when + // acking SETTINGS from the peer. Only contains a value if the peer advertises + // a larger table capacity than currently used; a smaller value can safely be + // applied immediately upon receipt. + absl::optional<uint32_t> encoder_header_table_capacity_when_acking_; + bool received_goaway_ = false; bool queued_preface_ = false; bool peer_supports_metadata_ = false;
diff --git a/spdy/core/hpack/hpack_encoder.h b/spdy/core/hpack/hpack_encoder.h index 0529297..dbdbb6b 100644 --- a/spdy/core/hpack/hpack_encoder.h +++ b/spdy/core/hpack/hpack_encoder.h
@@ -79,6 +79,7 @@ // SETTINGS_HEADER_TABLE_SIZE update from the remote decoding endpoint. void ApplyHeaderTableSizeSetting(size_t size_setting); + // TODO(birenroy): Rename this GetDynamicTableCapacity(). size_t CurrentHeaderTableSizeSetting() const { return header_table_.settings_size_bound(); }