Change Http2VisitorInterface::OnInvalidFrame() to accept an InvalidFrameError enum.
This CL introduces a new enum class InvalidFrameError in Http2VisitorInterface
and converts Http2VisitorInterface::OnInvalidFrame() to use an
InvalidFrameError instead of an integer error code. This change improves
clarity in the method and will allow OgHttp2Session to call OnInvalidFrame()
more readily, e.g., in header validation.
PiperOrigin-RevId: 407223283
diff --git a/http2/adapter/callback_visitor.cc b/http2/adapter/callback_visitor.cc
index 99c1223..49a76e7 100644
--- a/http2/adapter/callback_visitor.cc
+++ b/http2/adapter/callback_visitor.cc
@@ -1,6 +1,7 @@
#include "http2/adapter/callback_visitor.h"
#include "absl/strings/escaping.h"
+#include "http2/adapter/http2_util.h"
#include "http2/adapter/nghttp2_util.h"
#include "third_party/nghttp2/src/lib/includes/nghttp2/nghttp2.h"
#include "common/quiche_endian.h"
@@ -378,12 +379,15 @@
return 0;
}
-bool CallbackVisitor::OnInvalidFrame(Http2StreamId stream_id, int error_code) {
- QUICHE_VLOG(1) << "OnInvalidFrame(" << stream_id << ", " << error_code << ")";
+bool CallbackVisitor::OnInvalidFrame(Http2StreamId stream_id,
+ InvalidFrameError error) {
+ QUICHE_VLOG(1) << "OnInvalidFrame(" << stream_id << ", "
+ << InvalidFrameErrorToString(error) << ")";
QUICHE_DCHECK_EQ(stream_id, current_frame_.hd.stream_id);
if (callbacks_->on_invalid_frame_recv_callback) {
- return 0 == callbacks_->on_invalid_frame_recv_callback(
- nullptr, ¤t_frame_, error_code, user_data_);
+ return 0 ==
+ callbacks_->on_invalid_frame_recv_callback(
+ nullptr, ¤t_frame_, ToNgHttp2ErrorCode(error), user_data_);
}
return true;
}
diff --git a/http2/adapter/callback_visitor.h b/http2/adapter/callback_visitor.h
index 4abfe94..3bef25c 100644
--- a/http2/adapter/callback_visitor.h
+++ b/http2/adapter/callback_visitor.h
@@ -58,7 +58,8 @@
size_t length, uint8_t flags) override;
int OnFrameSent(uint8_t frame_type, Http2StreamId stream_id, size_t length,
uint8_t flags, uint32_t error_code) override;
- bool OnInvalidFrame(Http2StreamId stream_id, int error_code) override;
+ bool OnInvalidFrame(Http2StreamId stream_id,
+ InvalidFrameError error) override;
void OnBeginMetadataForStream(Http2StreamId stream_id,
size_t payload_length) override;
bool OnMetadataForStream(Http2StreamId stream_id,
diff --git a/http2/adapter/http2_util.cc b/http2/adapter/http2_util.cc
index 1ceff5c..138bde7 100644
--- a/http2/adapter/http2_util.cc
+++ b/http2/adapter/http2_util.cc
@@ -5,6 +5,7 @@
namespace {
using ConnectionError = Http2VisitorInterface::ConnectionError;
+using InvalidFrameError = Http2VisitorInterface::InvalidFrameError;
} // anonymous namespace
@@ -93,5 +94,25 @@
}
}
+absl::string_view InvalidFrameErrorToString(
+ Http2VisitorInterface::InvalidFrameError error) {
+ switch (error) {
+ case InvalidFrameError::kProtocol:
+ return "Protocol";
+ case InvalidFrameError::kRefusedStream:
+ return "RefusedStream";
+ case InvalidFrameError::kHttpHeader:
+ return "HttpHeader";
+ case InvalidFrameError::kHttpMessaging:
+ return "HttpMessaging";
+ case InvalidFrameError::kFlowControl:
+ return "FlowControl";
+ case InvalidFrameError::kStreamClosed:
+ return "StreamClosed";
+ }
+ QUICHE_LOG(ERROR) << "Unknown InvalidFrameError " << static_cast<int>(error);
+ return "UnknownError";
+}
+
} // namespace adapter
} // namespace http2
diff --git a/http2/adapter/http2_util.h b/http2/adapter/http2_util.h
index be6b877..3e25b8c 100644
--- a/http2/adapter/http2_util.h
+++ b/http2/adapter/http2_util.h
@@ -17,6 +17,9 @@
QUICHE_EXPORT_PRIVATE absl::string_view ConnectionErrorToString(
Http2VisitorInterface::ConnectionError error);
+QUICHE_EXPORT_PRIVATE absl::string_view InvalidFrameErrorToString(
+ Http2VisitorInterface::InvalidFrameError error);
+
} // namespace adapter
} // namespace http2
diff --git a/http2/adapter/http2_visitor_interface.h b/http2/adapter/http2_visitor_interface.h
index 7bffb39..5622452 100644
--- a/http2/adapter/http2_visitor_interface.h
+++ b/http2/adapter/http2_visitor_interface.h
@@ -184,11 +184,25 @@
size_t length, uint8_t flags,
uint32_t error_code) = 0;
- // Called when the connection receives an invalid frame. |error_code| is a
- // negative integer error code generated by the library. A return value of
+ // Called when the connection receives an invalid frame. A return value of
// false will result in the connection entering an error state, with no
// further frame processing possible.
- virtual bool OnInvalidFrame(Http2StreamId stream_id, int error_code) = 0;
+ enum class InvalidFrameError {
+ // The frame contains a general protocol error.
+ kProtocol,
+ // The frame would have caused a new (invalid) stream to be opened.
+ kRefusedStream,
+ // The frame contains an invalid header field.
+ kHttpHeader,
+ // The frame contains a violation in HTTP messaging rules.
+ kHttpMessaging,
+ // The frame causes a flow control error.
+ kFlowControl,
+ // The frame is on an already closed stream or has an invalid stream ID.
+ kStreamClosed,
+ };
+ virtual bool OnInvalidFrame(Http2StreamId stream_id,
+ InvalidFrameError error) = 0;
// Called when the connection receives the beginning of a METADATA frame
// (which may itself be the middle of a logical metadata block). The metadata
diff --git a/http2/adapter/mock_http2_visitor.h b/http2/adapter/mock_http2_visitor.h
index a66fe79..7454d2f 100644
--- a/http2/adapter/mock_http2_visitor.h
+++ b/http2/adapter/mock_http2_visitor.h
@@ -103,8 +103,8 @@
uint8_t flags, uint32_t error_code),
(override));
- MOCK_METHOD(bool, OnInvalidFrame, (Http2StreamId stream_id, int error_code),
- (override));
+ MOCK_METHOD(bool, OnInvalidFrame,
+ (Http2StreamId stream_id, InvalidFrameError error), (override));
MOCK_METHOD(void,
OnBeginMetadataForStream,
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc
index 03e2629..4754050 100644
--- a/http2/adapter/nghttp2_adapter_test.cc
+++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -1,6 +1,7 @@
#include "http2/adapter/nghttp2_adapter.h"
#include "http2/adapter/http2_protocol.h"
+#include "http2/adapter/http2_visitor_interface.h"
#include "http2/adapter/mock_http2_visitor.h"
#include "http2/adapter/nghttp2_test_utils.h"
#include "http2/adapter/oghttp2_util.h"
@@ -556,7 +557,9 @@
visitor,
OnErrorDebug("Invalid HTTP header field was received: frame type: 1, "
"stream: 1, name: [:bad-status], value: [9000]"));
- EXPECT_CALL(visitor, OnInvalidFrame(1, -531));
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
// Bad status trailer will cause a PROTOCOL_ERROR. The header is never
// delivered in an OnHeaderForStream callback.
diff --git a/http2/adapter/nghttp2_callbacks.cc b/http2/adapter/nghttp2_callbacks.cc
index c3abeef..8564d6a 100644
--- a/http2/adapter/nghttp2_callbacks.cc
+++ b/http2/adapter/nghttp2_callbacks.cc
@@ -212,8 +212,8 @@
void* user_data) {
QUICHE_CHECK_NE(user_data, nullptr);
auto* visitor = static_cast<Http2VisitorInterface*>(user_data);
- const bool result =
- visitor->OnInvalidFrame(frame->hd.stream_id, lib_error_code);
+ const bool result = visitor->OnInvalidFrame(
+ frame->hd.stream_id, ToInvalidFrameError(lib_error_code));
return result ? 0 : NGHTTP2_ERR_CALLBACK_FAILURE;
}
diff --git a/http2/adapter/nghttp2_util.cc b/http2/adapter/nghttp2_util.cc
index 4bfced6..65ee06b 100644
--- a/http2/adapter/nghttp2_util.cc
+++ b/http2/adapter/nghttp2_util.cc
@@ -15,6 +15,8 @@
namespace {
+using InvalidFrameError = Http2VisitorInterface::InvalidFrameError;
+
void DeleteCallbacks(nghttp2_session_callbacks* callbacks) {
if (callbacks) {
nghttp2_session_callbacks_del(callbacks);
@@ -122,6 +124,44 @@
return static_cast<Http2ErrorCode>(wire_error_code);
}
+int ToNgHttp2ErrorCode(InvalidFrameError error) {
+ switch (error) {
+ case InvalidFrameError::kProtocol:
+ return NGHTTP2_ERR_PROTO;
+ case InvalidFrameError::kRefusedStream:
+ return NGHTTP2_ERR_REFUSED_STREAM;
+ case InvalidFrameError::kHttpHeader:
+ return NGHTTP2_ERR_HTTP_HEADER;
+ case InvalidFrameError::kHttpMessaging:
+ return NGHTTP2_ERR_HTTP_MESSAGING;
+ case InvalidFrameError::kFlowControl:
+ return NGHTTP2_ERR_FLOW_CONTROL;
+ case InvalidFrameError::kStreamClosed:
+ return NGHTTP2_ERR_STREAM_CLOSED;
+ }
+ QUICHE_LOG(ERROR) << "Unknown InvalidFrameError " << static_cast<int>(error);
+ return NGHTTP2_ERR_PROTO;
+}
+
+InvalidFrameError ToInvalidFrameError(int error) {
+ switch (error) {
+ case NGHTTP2_ERR_PROTO:
+ return InvalidFrameError::kProtocol;
+ case NGHTTP2_ERR_REFUSED_STREAM:
+ return InvalidFrameError::kRefusedStream;
+ case NGHTTP2_ERR_HTTP_HEADER:
+ return InvalidFrameError::kHttpHeader;
+ case NGHTTP2_ERR_HTTP_MESSAGING:
+ return InvalidFrameError::kHttpMessaging;
+ case NGHTTP2_ERR_FLOW_CONTROL:
+ return InvalidFrameError::kFlowControl;
+ case NGHTTP2_ERR_STREAM_CLOSED:
+ return InvalidFrameError::kStreamClosed;
+ }
+ QUICHE_LOG(ERROR) << "Unknown error " << error;
+ return InvalidFrameError::kProtocol;
+}
+
class Nghttp2DataFrameSource : public DataFrameSource {
public:
Nghttp2DataFrameSource(nghttp2_data_provider provider,
diff --git a/http2/adapter/nghttp2_util.h b/http2/adapter/nghttp2_util.h
index 78d4702..57702a3 100644
--- a/http2/adapter/nghttp2_util.h
+++ b/http2/adapter/nghttp2_util.h
@@ -10,6 +10,7 @@
#include "absl/types/span.h"
#include "http2/adapter/data_source.h"
#include "http2/adapter/http2_protocol.h"
+#include "http2/adapter/http2_visitor_interface.h"
#include "third_party/nghttp2/src/lib/includes/nghttp2/nghttp2.h"
#include "spdy/core/spdy_header_block.h"
@@ -56,6 +57,11 @@
// based on the RFC 7540 Section 7 suggestion.
Http2ErrorCode ToHttp2ErrorCode(uint32_t wire_error_code);
+// Converts between the integer error code used by nghttp2 and the corresponding
+// InvalidFrameError value.
+int ToNgHttp2ErrorCode(Http2VisitorInterface::InvalidFrameError error);
+Http2VisitorInterface::InvalidFrameError ToInvalidFrameError(int error);
+
// Transforms a nghttp2_data_provider into a DataFrameSource. Assumes that
// |provider| uses the zero-copy nghttp2_data_source_read_callback API. Unsafe
// otherwise.
diff --git a/http2/adapter/recording_http2_visitor.cc b/http2/adapter/recording_http2_visitor.cc
index 7e12e3d..74db309 100644
--- a/http2/adapter/recording_http2_visitor.cc
+++ b/http2/adapter/recording_http2_visitor.cc
@@ -141,9 +141,9 @@
}
bool RecordingHttp2Visitor::OnInvalidFrame(Http2StreamId stream_id,
- int error_code) {
- events_.push_back(
- absl::StrFormat("OnInvalidFrame %d %d", stream_id, error_code));
+ InvalidFrameError error) {
+ events_.push_back(absl::StrFormat("OnInvalidFrame %d %s", stream_id,
+ InvalidFrameErrorToString(error)));
return true;
}
diff --git a/http2/adapter/recording_http2_visitor.h b/http2/adapter/recording_http2_visitor.h
index 4cbc15b..6008725 100644
--- a/http2/adapter/recording_http2_visitor.h
+++ b/http2/adapter/recording_http2_visitor.h
@@ -56,7 +56,8 @@
size_t length, uint8_t flags) override;
int OnFrameSent(uint8_t frame_type, Http2StreamId stream_id, size_t length,
uint8_t flags, uint32_t error_code) override;
- bool OnInvalidFrame(Http2StreamId stream_id, int error_code) override;
+ bool OnInvalidFrame(Http2StreamId stream_id,
+ InvalidFrameError error) override;
void OnBeginMetadataForStream(Http2StreamId stream_id,
size_t payload_length) override;
bool OnMetadataForStream(Http2StreamId stream_id,
