Adds a boolean return value to Http2VisitorInterface::OnFrameHeader.
A visitor implementation can indicate a fatal error for the current HTTP/2 session by returning false from this method.
PiperOrigin-RevId: 392424604
diff --git a/http2/adapter/callback_visitor.cc b/http2/adapter/callback_visitor.cc
index 981180e..358bfea 100644
--- a/http2/adapter/callback_visitor.cc
+++ b/http2/adapter/callback_visitor.cc
@@ -68,10 +68,8 @@
QUICHE_LOG(ERROR) << "OnConnectionError not implemented";
}
-void CallbackVisitor::OnFrameHeader(Http2StreamId stream_id,
- size_t length,
- uint8_t type,
- uint8_t flags) {
+bool CallbackVisitor::OnFrameHeader(Http2StreamId stream_id, size_t length,
+ uint8_t type, uint8_t flags) {
QUICHE_VLOG(1) << "CallbackVisitor::OnFrameHeader(stream_id: " << stream_id
<< ", len: " << length << ", type: " << int(type)
<< ", flags: " << int(flags) << ")";
@@ -89,9 +87,11 @@
hd.type = type;
hd.flags = flags;
if (callbacks_->on_begin_frame_callback) {
- callbacks_->on_begin_frame_callback(nullptr, &hd, user_data_);
+ const int result =
+ callbacks_->on_begin_frame_callback(nullptr, &hd, user_data_);
+ return result == 0;
}
- return;
+ return true;
}
// The general strategy is to clear |current_frame_| at the start of a new
// frame, accumulate frame information from the various callback events, then
@@ -102,9 +102,11 @@
current_frame_.hd.type = type;
current_frame_.hd.flags = flags;
if (callbacks_->on_begin_frame_callback) {
- callbacks_->on_begin_frame_callback(nullptr, ¤t_frame_.hd,
- user_data_);
+ const int result = callbacks_->on_begin_frame_callback(
+ nullptr, ¤t_frame_.hd, user_data_);
+ return result == 0;
}
+ return true;
}
void CallbackVisitor::OnSettingsStart() {}
diff --git a/http2/adapter/callback_visitor.h b/http2/adapter/callback_visitor.h
index 6a15f0a..eb2e2b4 100644
--- a/http2/adapter/callback_visitor.h
+++ b/http2/adapter/callback_visitor.h
@@ -24,9 +24,7 @@
int64_t OnReadyToSend(absl::string_view serialized) override;
void OnConnectionError() override;
- void OnFrameHeader(Http2StreamId stream_id,
- size_t length,
- uint8_t type,
+ bool OnFrameHeader(Http2StreamId stream_id, size_t length, uint8_t type,
uint8_t flags) override;
void OnSettingsStart() override;
void OnSetting(Http2Setting setting) override;
diff --git a/http2/adapter/http2_visitor_interface.h b/http2/adapter/http2_visitor_interface.h
index 535b174..e04e58c 100644
--- a/http2/adapter/http2_visitor_interface.h
+++ b/http2/adapter/http2_visitor_interface.h
@@ -62,8 +62,10 @@
virtual void OnConnectionError() = 0;
// Called when the header for a frame is received.
- virtual void OnFrameHeader(Http2StreamId /*stream_id*/, size_t /*length*/,
- uint8_t /*type*/, uint8_t /*flags*/) {}
+ virtual bool OnFrameHeader(Http2StreamId /*stream_id*/, size_t /*length*/,
+ uint8_t /*type*/, uint8_t /*flags*/) {
+ return true;
+ }
// Called when a non-ack SETTINGS frame is received.
virtual void OnSettingsStart() = 0;
diff --git a/http2/adapter/mock_http2_visitor.h b/http2/adapter/mock_http2_visitor.h
index 2ca83a4..5498c02 100644
--- a/http2/adapter/mock_http2_visitor.h
+++ b/http2/adapter/mock_http2_visitor.h
@@ -15,6 +15,7 @@
class QUICHE_NO_EXPORT MockHttp2Visitor : public Http2VisitorInterface {
public:
MockHttp2Visitor() {
+ ON_CALL(*this, OnFrameHeader).WillByDefault(testing::Return(true));
ON_CALL(*this, OnBeginHeadersForStream)
.WillByDefault(testing::Return(true));
ON_CALL(*this, OnHeaderForStream).WillByDefault(testing::Return(HEADER_OK));
@@ -27,11 +28,10 @@
MOCK_METHOD(int64_t, OnReadyToSend, (absl::string_view serialized),
(override));
MOCK_METHOD(void, OnConnectionError, (), (override));
- MOCK_METHOD(
- void,
- OnFrameHeader,
- (Http2StreamId stream_id, size_t length, uint8_t type, uint8_t flags),
- (override));
+ MOCK_METHOD(bool, OnFrameHeader,
+ (Http2StreamId stream_id, size_t length, uint8_t type,
+ uint8_t flags),
+ (override));
MOCK_METHOD(void, OnSettingsStart, (), (override));
MOCK_METHOD(void, OnSetting, (Http2Setting setting), (override));
MOCK_METHOD(void, OnSettingsEnd, (), (override));
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc
index ad66439..bc81936 100644
--- a/http2/adapter/nghttp2_adapter_test.cc
+++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -1428,6 +1428,51 @@
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS}));
}
+// Exercises the case when a visitor chooses to reject a frame based solely on
+// the frame header, which is a fatal error for the connection.
+TEST(NgHttp2AdapterTest, ServerRejectsFrameHeader) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
+
+ const std::string frames = TestFrameSequence()
+ .ClientPreface()
+ .Ping(64)
+ .Headers(1,
+ {{":method", "POST"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}},
+ /*fin=*/false)
+ .WindowUpdate(1, 2000)
+ .Data(1, "This is the request body.")
+ .WindowUpdate(0, 2000)
+ .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(0, 8, PING, 0))
+ .WillOnce(testing::Return(false));
+ EXPECT_CALL(visitor, OnConnectionError());
+
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_EQ(-902, result);
+
+ EXPECT_TRUE(adapter->session().want_write());
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+
+ int send_result = adapter->Send();
+ // Some bytes should have been serialized.
+ EXPECT_EQ(0, send_result);
+ // SETTINGS ack
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS}));
+}
+
TEST(NgHttp2AdapterTest, ServerSubmitResponse) {
DataSavingVisitor visitor;
auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
diff --git a/http2/adapter/nghttp2_callbacks.cc b/http2/adapter/nghttp2_callbacks.cc
index e22d437..17fb1a0 100644
--- a/http2/adapter/nghttp2_callbacks.cc
+++ b/http2/adapter/nghttp2_callbacks.cc
@@ -39,8 +39,11 @@
void* user_data) {
QUICHE_CHECK_NE(user_data, nullptr);
auto* visitor = static_cast<Http2VisitorInterface*>(user_data);
- visitor->OnFrameHeader(header->stream_id, header->length, header->type,
- header->flags);
+ bool result = visitor->OnFrameHeader(header->stream_id, header->length,
+ header->type, header->flags);
+ if (!result) {
+ return NGHTTP2_ERR_CALLBACK_FAILURE;
+ }
if (header->type == NGHTTP2_DATA) {
visitor->OnBeginDataForStream(header->stream_id, header->length);
} else if (header->type == kMetadataFrameType) {
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc
index 75f6121..4cffe2e 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -1063,6 +1063,56 @@
spdy::SpdyFrameType::SETTINGS}));
}
+// Exercises the case when a visitor chooses to reject a frame based solely on
+// the frame header, which is a fatal error for the connection.
+TEST(OgHttp2AdapterServerTest, ServerRejectsFrameHeader) {
+ DataSavingVisitor visitor;
+ OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
+
+ const std::string frames = TestFrameSequence()
+ .ClientPreface()
+ .Ping(64)
+ .Headers(1,
+ {{":method", "POST"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}},
+ /*fin=*/false)
+ .WindowUpdate(1, 2000)
+ .Data(1, "This is the request body.")
+ .WindowUpdate(0, 2000)
+ .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(0, 8, PING, 0))
+ .WillOnce(testing::Return(false));
+ EXPECT_CALL(visitor, OnConnectionError());
+
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_GT(result, 0);
+ EXPECT_LT(result, static_cast<int64_t>(frames.size()));
+
+ EXPECT_TRUE(adapter->session().want_write());
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+
+ int send_result = adapter->Send();
+ // Some bytes should have been serialized.
+ EXPECT_EQ(0, send_result);
+ // SETTINGS and SETTINGS ack
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::SETTINGS}));
+}
+
} // namespace
} // namespace test
} // namespace adapter
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc
index b3505b8..39703a4 100644
--- a/http2/adapter/oghttp2_session.cc
+++ b/http2/adapter/oghttp2_session.cc
@@ -585,7 +585,10 @@
uint8_t flags) {
highest_received_stream_id_ = std::max(static_cast<Http2StreamId>(stream_id),
highest_received_stream_id_);
- visitor_.OnFrameHeader(stream_id, length, type, flags);
+ const bool result = visitor_.OnFrameHeader(stream_id, length, type, flags);
+ if (!result) {
+ decoder_.StopProcessing();
+ }
}
void OgHttp2Session::OnDataFrameHeader(spdy::SpdyStreamId stream_id,
diff --git a/http2/adapter/recording_http2_visitor.cc b/http2/adapter/recording_http2_visitor.cc
index b66d5e0..767f3a7 100644
--- a/http2/adapter/recording_http2_visitor.cc
+++ b/http2/adapter/recording_http2_visitor.cc
@@ -16,12 +16,12 @@
events_.push_back("OnConnectionError");
}
-void RecordingHttp2Visitor::OnFrameHeader(Http2StreamId stream_id,
- size_t length,
- uint8_t type,
+bool RecordingHttp2Visitor::OnFrameHeader(Http2StreamId stream_id,
+ size_t length, uint8_t type,
uint8_t flags) {
events_.push_back(absl::StrFormat("OnFrameHeader %d %d %d %d", stream_id,
length, type, flags));
+ return true;
}
void RecordingHttp2Visitor::OnSettingsStart() {
diff --git a/http2/adapter/recording_http2_visitor.h b/http2/adapter/recording_http2_visitor.h
index 9627445..ee06b1e 100644
--- a/http2/adapter/recording_http2_visitor.h
+++ b/http2/adapter/recording_http2_visitor.h
@@ -22,9 +22,7 @@
// From Http2VisitorInterface
int64_t OnReadyToSend(absl::string_view serialized) override;
void OnConnectionError() override;
- void OnFrameHeader(Http2StreamId stream_id,
- size_t length,
- uint8_t type,
+ bool OnFrameHeader(Http2StreamId stream_id, size_t length, uint8_t type,
uint8_t flags) override;
void OnSettingsStart() override;
void OnSetting(Http2Setting setting) override;
