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,