Adds a return value to Http2VisitorInterface::OnBeginHeadersForStream().
Also adds tests showing the slight difference in behavior between adapter implementations when OnBeginHeadersForStream() returns false.
PiperOrigin-RevId: 387156180
diff --git a/http2/adapter/callback_visitor.cc b/http2/adapter/callback_visitor.cc
index 3bc7784..7701cc6 100644
--- a/http2/adapter/callback_visitor.cc
+++ b/http2/adapter/callback_visitor.cc
@@ -120,7 +120,7 @@
}
}
-void CallbackVisitor::OnBeginHeadersForStream(Http2StreamId stream_id) {
+bool CallbackVisitor::OnBeginHeadersForStream(Http2StreamId stream_id) {
auto it = GetStreamInfo(stream_id);
if (it->second->received_headers) {
// At least one headers frame has already been received.
@@ -142,12 +142,13 @@
break;
}
}
+ it->second->received_headers = true;
if (callbacks_->on_begin_headers_callback) {
const int result = callbacks_->on_begin_headers_callback(
nullptr, ¤t_frame_, user_data_);
- QUICHE_DCHECK_EQ(0, result);
+ return result == 0;
}
- it->second->received_headers = true;
+ return true;
}
Http2VisitorInterface::OnHeaderResult CallbackVisitor::OnHeaderForStream(
diff --git a/http2/adapter/callback_visitor.h b/http2/adapter/callback_visitor.h
index 8a32862..5cf6304 100644
--- a/http2/adapter/callback_visitor.h
+++ b/http2/adapter/callback_visitor.h
@@ -31,7 +31,7 @@
void OnSetting(Http2Setting setting) override;
void OnSettingsEnd() override;
void OnSettingsAck() override;
- void OnBeginHeadersForStream(Http2StreamId stream_id) override;
+ bool OnBeginHeadersForStream(Http2StreamId stream_id) override;
OnHeaderResult OnHeaderForStream(Http2StreamId stream_id,
absl::string_view name,
absl::string_view value) override;
diff --git a/http2/adapter/http2_visitor_interface.h b/http2/adapter/http2_visitor_interface.h
index feaff14..292021a 100644
--- a/http2/adapter/http2_visitor_interface.h
+++ b/http2/adapter/http2_visitor_interface.h
@@ -77,8 +77,9 @@
virtual void OnSettingsAck() = 0;
// Called when the connection receives the header block for a HEADERS frame on
- // a stream but has not yet parsed individual headers.
- virtual void OnBeginHeadersForStream(Http2StreamId stream_id) = 0;
+ // a stream but has not yet parsed individual headers. Returns false if a
+ // fatal error has occurred.
+ virtual bool OnBeginHeadersForStream(Http2StreamId stream_id) = 0;
// Called when the connection receives the header |key| and |value| for a
// stream. The HTTP/2 pseudo-headers defined in RFC 7540 Sections 8.1.2.3 and
diff --git a/http2/adapter/mock_http2_visitor.h b/http2/adapter/mock_http2_visitor.h
index 264c34e..3daa805 100644
--- a/http2/adapter/mock_http2_visitor.h
+++ b/http2/adapter/mock_http2_visitor.h
@@ -13,6 +13,8 @@
class QUICHE_NO_EXPORT MockHttp2Visitor : public Http2VisitorInterface {
public:
MockHttp2Visitor() {
+ ON_CALL(*this, OnBeginHeadersForStream)
+ .WillByDefault(testing::Return(true));
ON_CALL(*this, OnHeaderForStream).WillByDefault(testing::Return(HEADER_OK));
ON_CALL(*this, OnInvalidFrame).WillByDefault(testing::Return(true));
ON_CALL(*this, OnMetadataEndForStream).WillByDefault(testing::Return(true));
@@ -32,9 +34,7 @@
MOCK_METHOD(void, OnSetting, (Http2Setting setting), (override));
MOCK_METHOD(void, OnSettingsEnd, (), (override));
MOCK_METHOD(void, OnSettingsAck, (), (override));
- MOCK_METHOD(void,
- OnBeginHeadersForStream,
- (Http2StreamId stream_id),
+ MOCK_METHOD(bool, OnBeginHeadersForStream, (Http2StreamId stream_id),
(override));
MOCK_METHOD(OnHeaderResult, OnHeaderForStream,
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc
index b9d9c1d..84f3939 100644
--- a/http2/adapter/nghttp2_adapter_test.cc
+++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -636,6 +636,69 @@
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS}));
}
+TEST(NgHttp2AdapterTest, ClientRejectsHeaders) {
+ 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"}});
+
+ 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;
+
+ 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);
+ absl::string_view data = visitor.data();
+ EXPECT_THAT(data, testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix));
+ data.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix));
+ EXPECT_THAT(data, EqualsFrames({spdy::SpdyFrameType::HEADERS}));
+ visitor.Clear();
+
+ const std::string stream_frames =
+ TestFrameSequence()
+ .ServerPreface()
+ .Headers(1,
+ {{":status", "200"},
+ {"server", "my-fake-server"},
+ {"date", "Tue, 6 Apr 2021 12:54:01 GMT"}},
+ /*fin=*/false)
+ .Data(1, "This is the response body.")
+ .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(1, _, HEADERS, 4));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1))
+ .WillOnce(testing::Return(false));
+ // Rejecting headers leads to a connection error.
+ EXPECT_CALL(visitor, OnConnectionError());
+
+ const ssize_t stream_result = adapter->ProcessBytes(stream_frames);
+ EXPECT_EQ(NGHTTP2_ERR_CALLBACK_FAILURE, stream_result);
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+
+ EXPECT_TRUE(adapter->session().want_write());
+ result = adapter->Send();
+ EXPECT_EQ(0, result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS}));
+}
+
TEST(NgHttp2AdapterTest, ClientSubmitRequest) {
DataSavingVisitor visitor;
auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor);
diff --git a/http2/adapter/nghttp2_callbacks.cc b/http2/adapter/nghttp2_callbacks.cc
index f34cdf0..51b2054 100644
--- a/http2/adapter/nghttp2_callbacks.cc
+++ b/http2/adapter/nghttp2_callbacks.cc
@@ -148,8 +148,8 @@
void* user_data) {
QUICHE_CHECK_NE(user_data, nullptr);
auto* visitor = static_cast<Http2VisitorInterface*>(user_data);
- visitor->OnBeginHeadersForStream(frame->hd.stream_id);
- return 0;
+ const bool result = visitor->OnBeginHeadersForStream(frame->hd.stream_id);
+ return result ? 0 : NGHTTP2_ERR_CALLBACK_FAILURE;
}
int OnHeader(nghttp2_session* /* session */, const nghttp2_frame* frame,
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc
index 87576bb..b8b2a3d 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -297,7 +297,7 @@
spdy::SpdyFrameType::RST_STREAM}));
}
-TEST(NgHttp2AdapterTest, ClientConnectionErrorWhileHandlingHeaders) {
+TEST(OgHttp2AdapterClientTest, ClientConnectionErrorWhileHandlingHeaders) {
DataSavingVisitor visitor;
OgHttp2Adapter::Options options{.perspective = Perspective::kClient};
auto adapter = OgHttp2Adapter::Create(visitor, options);
@@ -373,6 +373,78 @@
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS}));
}
+TEST(OgHttp2AdapterClientTest, ClientRejectsHeaders) {
+ 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 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;
+
+ 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);
+ absl::string_view data = visitor.data();
+ EXPECT_THAT(data, testing::StartsWith(spdy::kHttp2ConnectionHeaderPrefix));
+ data.remove_prefix(strlen(spdy::kHttp2ConnectionHeaderPrefix));
+ EXPECT_THAT(data, EqualsFrames({spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::HEADERS}));
+ visitor.Clear();
+
+ const std::string stream_frames =
+ TestFrameSequence()
+ .ServerPreface()
+ .Headers(1,
+ {{":status", "200"},
+ {"server", "my-fake-server"},
+ {"date", "Tue, 6 Apr 2021 12:54:01 GMT"}},
+ /*fin=*/false)
+ .Data(1, "This is the response body.")
+ .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(1, _, HEADERS, 4));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1))
+ .WillOnce(testing::Return(false));
+ // Rejecting headers leads to a connection error.
+ EXPECT_CALL(visitor, OnConnectionError());
+ // Note: OgHttp2Adapter continues processing bytes until the input is
+ // complete.
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0));
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _));
+ EXPECT_CALL(visitor, OnDataForStream(1, "This is the response body."));
+
+ const ssize_t stream_result = adapter->ProcessBytes(stream_frames);
+ EXPECT_EQ(stream_result, stream_frames.size());
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+
+ EXPECT_TRUE(adapter->session().want_write());
+ result = adapter->Send();
+ EXPECT_EQ(0, result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS}));
+}
+
// 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 1dac781..93195f3 100644
--- a/http2/adapter/oghttp2_session.cc
+++ b/http2/adapter/oghttp2_session.cc
@@ -100,7 +100,10 @@
} // namespace
void OgHttp2Session::PassthroughHeadersHandler::OnHeaderBlockStart() {
- visitor_.OnBeginHeadersForStream(stream_id_);
+ const bool status = visitor_.OnBeginHeadersForStream(stream_id_);
+ if (!status) {
+ result_ = Http2VisitorInterface::HEADER_CONNECTION_ERROR;
+ }
}
void OgHttp2Session::PassthroughHeadersHandler::OnHeader(
diff --git a/http2/adapter/recording_http2_visitor.cc b/http2/adapter/recording_http2_visitor.cc
index 0706c5e..75f3749 100644
--- a/http2/adapter/recording_http2_visitor.cc
+++ b/http2/adapter/recording_http2_visitor.cc
@@ -41,8 +41,9 @@
events_.push_back("OnSettingsAck");
}
-void RecordingHttp2Visitor::OnBeginHeadersForStream(Http2StreamId stream_id) {
+bool RecordingHttp2Visitor::OnBeginHeadersForStream(Http2StreamId stream_id) {
events_.push_back(absl::StrFormat("OnBeginHeadersForStream %d", stream_id));
+ return true;
}
Http2VisitorInterface::OnHeaderResult RecordingHttp2Visitor::OnHeaderForStream(
diff --git a/http2/adapter/recording_http2_visitor.h b/http2/adapter/recording_http2_visitor.h
index 501a476..25b9293 100644
--- a/http2/adapter/recording_http2_visitor.h
+++ b/http2/adapter/recording_http2_visitor.h
@@ -29,7 +29,7 @@
void OnSetting(Http2Setting setting) override;
void OnSettingsEnd() override;
void OnSettingsAck() override;
- void OnBeginHeadersForStream(Http2StreamId stream_id) override;
+ bool OnBeginHeadersForStream(Http2StreamId stream_id) override;
OnHeaderResult OnHeaderForStream(Http2StreamId stream_id,
absl::string_view name,
absl::string_view value) override;
