This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.0.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit ac91476c7fa1ba58364e8108b441596759017af9 Author: Masakazu Kitajo <[email protected]> AuthorDate: Wed Jul 31 15:07:37 2024 -0600 Remove H3 frame sequence number tracking (#11632) * Remove H3 frame sequence number tracking * Remove redundant free_MIOBuffer call (cherry picked from commit aaff5a9c922f4fbf1c3eacbf5864491d9cdd30c8) --- include/proxy/http3/Http3FrameCounter.h | 3 +- include/proxy/http3/Http3FrameHandler.h | 4 +- include/proxy/http3/Http3HeaderVIOAdaptor.h | 3 +- include/proxy/http3/Http3ProtocolEnforcer.h | 6 ++- include/proxy/http3/Http3SettingsHandler.h | 3 +- include/proxy/http3/Http3StreamDataVIOAdaptor.h | 3 +- src/proxy/http3/Http3FrameCounter.cc | 3 +- src/proxy/http3/Http3FrameDispatcher.cc | 4 +- src/proxy/http3/Http3HeaderVIOAdaptor.cc | 2 +- src/proxy/http3/Http3ProtocolEnforcer.cc | 9 ++-- src/proxy/http3/Http3SettingsHandler.cc | 2 +- src/proxy/http3/Http3StreamDataVIOAdaptor.cc | 3 +- src/proxy/http3/test/Mock.h | 3 +- src/proxy/http3/test/test_Http3FrameDispatcher.cc | 54 +++++++++-------------- 14 files changed, 42 insertions(+), 60 deletions(-) diff --git a/include/proxy/http3/Http3FrameCounter.h b/include/proxy/http3/Http3FrameCounter.h index 4a03c87d7a..ead94eb28f 100644 --- a/include/proxy/http3/Http3FrameCounter.h +++ b/include/proxy/http3/Http3FrameCounter.h @@ -33,8 +33,7 @@ public: // Http3FrameHandler std::vector<Http3FrameType> interests() override; - Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame, int32_t frame_seq = -1, - Http3StreamType s_type = Http3StreamType::UNKNOWN) override; + Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame, Http3StreamType s_type = Http3StreamType::UNKNOWN) override; uint64_t get_count(uint64_t type) const; diff --git a/include/proxy/http3/Http3FrameHandler.h b/include/proxy/http3/Http3FrameHandler.h index 8d297882ba..2a8bb3ff4b 100644 --- a/include/proxy/http3/Http3FrameHandler.h +++ b/include/proxy/http3/Http3FrameHandler.h @@ -32,6 +32,6 @@ class Http3FrameHandler public: virtual ~Http3FrameHandler(){}; virtual std::vector<Http3FrameType> interests() = 0; - virtual Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame, int32_t frame_seq = -1, - Http3StreamType s_type = Http3StreamType::UNKNOWN) = 0; + virtual Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame, + Http3StreamType s_type = Http3StreamType::UNKNOWN) = 0; }; diff --git a/include/proxy/http3/Http3HeaderVIOAdaptor.h b/include/proxy/http3/Http3HeaderVIOAdaptor.h index 24d2e79f04..ca8b3da0e6 100644 --- a/include/proxy/http3/Http3HeaderVIOAdaptor.h +++ b/include/proxy/http3/Http3HeaderVIOAdaptor.h @@ -35,8 +35,7 @@ public: // Http3FrameHandler std::vector<Http3FrameType> interests() override; - Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame, int32_t frame_seq = -1, - Http3StreamType s_type = Http3StreamType::UNKNOWN) override; + Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame, Http3StreamType s_type = Http3StreamType::UNKNOWN) override; bool is_complete(); int event_handler(int event, Event *data); diff --git a/include/proxy/http3/Http3ProtocolEnforcer.h b/include/proxy/http3/Http3ProtocolEnforcer.h index 328143155e..ee291bdeca 100644 --- a/include/proxy/http3/Http3ProtocolEnforcer.h +++ b/include/proxy/http3/Http3ProtocolEnforcer.h @@ -33,6 +33,8 @@ public: // Http3FrameHandler std::vector<Http3FrameType> interests() override; - Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame, int32_t frame_seq = -1, - Http3StreamType s_type = Http3StreamType::UNKNOWN) override; + Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame, Http3StreamType s_type = Http3StreamType::UNKNOWN) override; + +private: + bool _is_first_frame_received_on_control = false; }; diff --git a/include/proxy/http3/Http3SettingsHandler.h b/include/proxy/http3/Http3SettingsHandler.h index 48340e7352..51545ee07f 100644 --- a/include/proxy/http3/Http3SettingsHandler.h +++ b/include/proxy/http3/Http3SettingsHandler.h @@ -33,8 +33,7 @@ public: // Http3FrameHandler std::vector<Http3FrameType> interests() override; - Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame, int32_t frame_seq = -1, - Http3StreamType s_type = Http3StreamType::UNKNOWN) override; + Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame, Http3StreamType s_type = Http3StreamType::UNKNOWN) override; private: // TODO: clarify Http3Session I/F for Http3SettingsHandler and Http3App diff --git a/include/proxy/http3/Http3StreamDataVIOAdaptor.h b/include/proxy/http3/Http3StreamDataVIOAdaptor.h index e13e61313c..4eff081d9d 100644 --- a/include/proxy/http3/Http3StreamDataVIOAdaptor.h +++ b/include/proxy/http3/Http3StreamDataVIOAdaptor.h @@ -35,8 +35,7 @@ public: // Http3FrameHandler std::vector<Http3FrameType> interests() override; - Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame, int32_t frame_seq = -1, - Http3StreamType s_type = Http3StreamType::UNKNOWN) override; + Http3ErrorUPtr handle_frame(std::shared_ptr<const Http3Frame> frame, Http3StreamType s_type = Http3StreamType::UNKNOWN) override; // Http3StreamDataVIOAdaptor void finalize(); diff --git a/src/proxy/http3/Http3FrameCounter.cc b/src/proxy/http3/Http3FrameCounter.cc index ae89868076..1ac83210d2 100644 --- a/src/proxy/http3/Http3FrameCounter.cc +++ b/src/proxy/http3/Http3FrameCounter.cc @@ -35,8 +35,7 @@ Http3FrameCounter::interests() } Http3ErrorUPtr -Http3FrameCounter::handle_frame(std::shared_ptr<const Http3Frame> frame, int32_t /* frame_seq ATS_UNUSED */, - Http3StreamType /* s_type ATS_UNUSED */) +Http3FrameCounter::handle_frame(std::shared_ptr<const Http3Frame> frame, Http3StreamType /* s_type ATS_UNUSED */) { Http3ErrorUPtr error = Http3ErrorUPtr(nullptr); Http3FrameType f_type = frame->type(); diff --git a/src/proxy/http3/Http3FrameDispatcher.cc b/src/proxy/http3/Http3FrameDispatcher.cc index 1b6b3f32bb..bce8fefdcc 100644 --- a/src/proxy/http3/Http3FrameDispatcher.cc +++ b/src/proxy/http3/Http3FrameDispatcher.cc @@ -52,7 +52,6 @@ Http3FrameDispatcher::on_read_ready(QUICStreamId stream_id, Http3StreamType stre { Http3ErrorUPtr error = Http3ErrorUPtr(nullptr); nread = 0; - uint32_t frame_count = 0; while (true) { // Read a length of Type field and hopefully a length of Length field too @@ -110,7 +109,6 @@ Http3FrameDispatcher::on_read_ready(QUICStreamId stream_id, Http3StreamType stre break; } this->_bytes_to_skip = this->_current_frame->total_length(); - ++frame_count; } auto skip = std::min(static_cast<uint64_t>(reader.read_avail()), this->_bytes_to_skip); @@ -125,7 +123,7 @@ Http3FrameDispatcher::on_read_ready(QUICStreamId stream_id, Http3StreamType stre this->_current_frame->total_length() - _bytes_to_skip, this->_current_frame->total_length()); std::vector<Http3FrameHandler *> handlers = this->_handlers[static_cast<uint8_t>(type)]; for (auto h : handlers) { - error = h->handle_frame(this->_current_frame, frame_count - 1, stream_type); + error = h->handle_frame(this->_current_frame, stream_type); if (error && error->cls != Http3ErrorClass::UNDEFINED) { return error; } diff --git a/src/proxy/http3/Http3HeaderVIOAdaptor.cc b/src/proxy/http3/Http3HeaderVIOAdaptor.cc index 3fb501158c..8b9cac2992 100644 --- a/src/proxy/http3/Http3HeaderVIOAdaptor.cc +++ b/src/proxy/http3/Http3HeaderVIOAdaptor.cc @@ -54,7 +54,7 @@ Http3HeaderVIOAdaptor::interests() } Http3ErrorUPtr -Http3HeaderVIOAdaptor::handle_frame(std::shared_ptr<const Http3Frame> frame, int32_t /* frame_seq */, Http3StreamType /* s_type */) +Http3HeaderVIOAdaptor::handle_frame(std::shared_ptr<const Http3Frame> frame, Http3StreamType /* s_type */) { ink_assert(frame->type() == Http3FrameType::HEADERS); const Http3HeadersFrame *hframe = dynamic_cast<const Http3HeadersFrame *>(frame.get()); diff --git a/src/proxy/http3/Http3ProtocolEnforcer.cc b/src/proxy/http3/Http3ProtocolEnforcer.cc index ee2b26addf..9f7e8a8963 100644 --- a/src/proxy/http3/Http3ProtocolEnforcer.cc +++ b/src/proxy/http3/Http3ProtocolEnforcer.cc @@ -34,15 +34,15 @@ Http3ProtocolEnforcer::interests() } Http3ErrorUPtr -Http3ProtocolEnforcer::handle_frame(std::shared_ptr<const Http3Frame> frame, int32_t frame_seq, Http3StreamType s_type) +Http3ProtocolEnforcer::handle_frame(std::shared_ptr<const Http3Frame> frame, Http3StreamType s_type) { Http3ErrorUPtr error = Http3ErrorUPtr(nullptr); Http3FrameType f_type = frame->type(); if (s_type == Http3StreamType::CONTROL) { - if (frame_seq == 0 && f_type != Http3FrameType::SETTINGS) { + if (!this->_is_first_frame_received_on_control && f_type != Http3FrameType::SETTINGS) { error = std::make_unique<Http3Error>(Http3ErrorClass::CONNECTION, Http3ErrorCode::H3_MISSING_SETTINGS, "first frame of the control stream must be SETTINGS frame"); - } else if (frame_seq != 0 && f_type == Http3FrameType::SETTINGS) { + } else if (this->_is_first_frame_received_on_control && f_type == Http3FrameType::SETTINGS) { error = std::make_unique<Http3Error>(Http3ErrorClass::CONNECTION, Http3ErrorCode::H3_FRAME_UNEXPECTED, "only one SETTINGS frame is allowed per the control stream"); } else if (f_type == Http3FrameType::DATA || f_type == Http3FrameType::HEADERS || f_type == Http3FrameType::X_RESERVED_1 || @@ -51,6 +51,9 @@ Http3ProtocolEnforcer::handle_frame(std::shared_ptr<const Http3Frame> frame, int error_msg.append(" frame is not allowed on control stream"); error = std::make_unique<Http3Error>(Http3ErrorClass::CONNECTION, Http3ErrorCode::H3_FRAME_UNEXPECTED, error_msg.c_str()); } + if (!this->_is_first_frame_received_on_control) { + this->_is_first_frame_received_on_control = true; + } } else { if (f_type == Http3FrameType::X_RESERVED_1 || f_type == Http3FrameType::X_RESERVED_2 || f_type == Http3FrameType::X_RESERVED_3) { diff --git a/src/proxy/http3/Http3SettingsHandler.cc b/src/proxy/http3/Http3SettingsHandler.cc index b00e1fb866..7bc4c7ede9 100644 --- a/src/proxy/http3/Http3SettingsHandler.cc +++ b/src/proxy/http3/Http3SettingsHandler.cc @@ -39,7 +39,7 @@ Http3SettingsHandler::interests() } Http3ErrorUPtr -Http3SettingsHandler::handle_frame(std::shared_ptr<const Http3Frame> frame, int32_t /* frame_seq */, Http3StreamType /* s_type */) +Http3SettingsHandler::handle_frame(std::shared_ptr<const Http3Frame> frame, Http3StreamType /* s_type */) { ink_assert(frame->type() == Http3FrameType::SETTINGS); diff --git a/src/proxy/http3/Http3StreamDataVIOAdaptor.cc b/src/proxy/http3/Http3StreamDataVIOAdaptor.cc index d4ffcb3a21..7f19c277d9 100644 --- a/src/proxy/http3/Http3StreamDataVIOAdaptor.cc +++ b/src/proxy/http3/Http3StreamDataVIOAdaptor.cc @@ -38,8 +38,7 @@ Http3StreamDataVIOAdaptor::interests() } Http3ErrorUPtr -Http3StreamDataVIOAdaptor::handle_frame(std::shared_ptr<const Http3Frame> frame, int32_t /* frame_seq */, - Http3StreamType /* s_type */) +Http3StreamDataVIOAdaptor::handle_frame(std::shared_ptr<const Http3Frame> frame, Http3StreamType /* s_type */) { ink_assert(frame->type() == Http3FrameType::DATA); const Http3DataFrame *dframe = dynamic_cast<const Http3DataFrame *>(frame.get()); diff --git a/src/proxy/http3/test/Mock.h b/src/proxy/http3/test/Mock.h index 5c95c61cce..d31d50fb7e 100644 --- a/src/proxy/http3/test/Mock.h +++ b/src/proxy/http3/test/Mock.h @@ -39,8 +39,7 @@ public: } Http3ErrorUPtr - handle_frame(std::shared_ptr<const Http3Frame> /* frame ATS_UNUSED */, int32_t /* frame_seq */, - Http3StreamType /* s_type */) override + handle_frame(std::shared_ptr<const Http3Frame> /* frame ATS_UNUSED */, Http3StreamType /* s_type */) override { this->total_frame_received++; return Http3ErrorUPtr(nullptr); diff --git a/src/proxy/http3/test/test_Http3FrameDispatcher.cc b/src/proxy/http3/test/test_Http3FrameDispatcher.cc index e81175efe1..4062c73d89 100644 --- a/src/proxy/http3/test/test_Http3FrameDispatcher.cc +++ b/src/proxy/http3/test/test_Http3FrameDispatcher.cc @@ -258,45 +258,31 @@ TEST_CASE("control stream tests", "[http3]") CHECK(nread == sizeof(input)); } - free_MIOBuffer(buf); -} - -// This test needs to run without an enforcer due to a frame counting bug. -// Add a ProtocolEnforcer handler to reproduce. -TEST_CASE("padding should not be interpreted as a DATA frame", "[http3]") -{ - Http3FrameDispatcher http3FrameDispatcher; - Http3MockFrameHandler handler; + SECTION("padding should not be interpreted as a DATA frame", "[http3]") + { + uint8_t input[] = { + 0x40, 0x04, // Type + 0x03, // Length + 0x06, // Identifier + 0x44, 0x00, // Value + }; - http3FrameDispatcher.add_handler(&handler); + // Initial state + CHECK(handler.total_frame_received == 0); + CHECK(nread == 0); - MIOBuffer *buf = new_MIOBuffer(BUFFER_SIZE_INDEX_512); - IOBufferReader *reader = buf->alloc_reader(); - uint64_t nread = 0; - Http3ErrorUPtr error = Http3ErrorUPtr(nullptr); + int total_nread{}; + for (uint8_t *it{input}; it < input + sizeof(input); ++it) { + buf->write(it, 1); + error = http3FrameDispatcher.on_read_ready(0, Http3StreamType::CONTROL, *reader, nread); + total_nread += nread; + CHECK(!error); + } - uint8_t input[] = { - 0x40, 0x04, // Type - 0x03, // Length - 0x06, // Identifier - 0x44, 0x00, // Value - }; - - // Initial state - CHECK(handler.total_frame_received == 0); - CHECK(nread == 0); - - int total_nread{}; - for (uint8_t *it{input}; it < input + sizeof(input); ++it) { - buf->write(it, 1); - error = http3FrameDispatcher.on_read_ready(0, Http3StreamType::CONTROL, *reader, nread); - total_nread += nread; - CHECK(!error); + CHECK(handler.total_frame_received == 1); + CHECK(total_nread == 6); } - CHECK(handler.total_frame_received == 1); - CHECK(total_nread == 6); - free_MIOBuffer(buf); }
