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 4ede86b756d875662c4c1aef6ec09f7ef70cabb9 Author: JosiahWI <[email protected]> AuthorDate: Fri Jul 26 16:13:42 2024 -0500 Parse frame types consisting of multiple bytes (#11611) The variable integer for the frame type can have up to 8 bytes. Without this patch, ATS uses a 1 byte array to store the frame type, and we experience an assertion failure if we don't have enough bytes to decode the variable integer. This increases the array size to 8 bytes. Fixes #11589. (cherry picked from commit f5d9149992bb26e5d8378c0b49486995052ec402) --- src/proxy/http3/Http3Frame.cc | 10 +-- src/proxy/http3/test/test_Http3FrameDispatcher.cc | 99 ++++++++++++++++++++--- 2 files changed, 92 insertions(+), 17 deletions(-) diff --git a/src/proxy/http3/Http3Frame.cc b/src/proxy/http3/Http3Frame.cc index f2e3b91ca2..9eb99fb1b7 100644 --- a/src/proxy/http3/Http3Frame.cc +++ b/src/proxy/http3/Http3Frame.cc @@ -33,7 +33,9 @@ ClassAllocator<Http3SettingsFrame> http3SettingsFrameAllocator("http3SettingsFra namespace { -constexpr int HEADER_OVERHEAD = 10; // This should work as long as a payload length is less than 64 bits +// The frame type is a variable integer as defined in QUIC (RFC 9000). +constexpr int FRAME_TYPE_MAX_BYTES = 8; +constexpr int HEADER_OVERHEAD = 10; // This should work as long as a payload length is less than 64 bits DbgCtl dbg_ctl_http3_frame_factory{"http3_frame_factory"}; @@ -506,8 +508,7 @@ Http3FrameFactory::create(IOBufferReader &reader) ts::Http3Config::scoped_config params; Http3Frame *frame = nullptr; - // FIXME Frame type can be longer than 1 byte - uint8_t type_buf[1]; + uint8_t type_buf[FRAME_TYPE_MAX_BYTES]; reader.memcpy(type_buf, sizeof(type_buf)); Http3FrameType type = Http3Frame::type(type_buf, sizeof(type_buf)); @@ -536,8 +537,7 @@ Http3FrameFactory::create(IOBufferReader &reader) std::shared_ptr<Http3Frame> Http3FrameFactory::fast_create(IOBufferReader &reader) { - // FIXME Frame type can be longer than 1 byte - uint8_t type_buf[1]; + uint8_t type_buf[FRAME_TYPE_MAX_BYTES]{}; reader.memcpy(type_buf, sizeof(type_buf)); Http3FrameType type = Http3Frame::type(type_buf, sizeof(type_buf)); if (type == Http3FrameType::UNKNOWN) { diff --git a/src/proxy/http3/test/test_Http3FrameDispatcher.cc b/src/proxy/http3/test/test_Http3FrameDispatcher.cc index fd38ca8042..fc7711f7cc 100644 --- a/src/proxy/http3/test/test_Http3FrameDispatcher.cc +++ b/src/proxy/http3/test/test_Http3FrameDispatcher.cc @@ -29,6 +29,17 @@ TEST_CASE("Http3FrameHandler dispatch", "[http3]") { + Http3FrameDispatcher http3FrameDispatcher; + Http3MockFrameHandler handler; + Http3ProtocolEnforcer enforcer; + http3FrameDispatcher.add_handler(&handler); + http3FrameDispatcher.add_handler(&enforcer); + + MIOBuffer *buf = new_MIOBuffer(BUFFER_SIZE_INDEX_512); + IOBufferReader *reader = buf->alloc_reader(); + uint64_t nread = 0; + Http3ErrorUPtr error = Http3ErrorUPtr(nullptr); + SECTION("Test good case") { uint8_t input[] = {// 1st frame (HEADERS) @@ -38,17 +49,6 @@ TEST_CASE("Http3FrameHandler dispatch", "[http3]") // 3rd frame (incomplete) 0xff}; - Http3FrameDispatcher http3FrameDispatcher; - Http3MockFrameHandler handler; - Http3ProtocolEnforcer enforcer; - http3FrameDispatcher.add_handler(&handler); - http3FrameDispatcher.add_handler(&enforcer); - - MIOBuffer *buf = new_MIOBuffer(BUFFER_SIZE_INDEX_512); - IOBufferReader *reader = buf->alloc_reader(); - uint64_t nread = 0; - Http3ErrorUPtr error = Http3ErrorUPtr(nullptr); - buf->write(input, sizeof(input)); // Initial state @@ -59,9 +59,45 @@ TEST_CASE("Http3FrameHandler dispatch", "[http3]") CHECK(!error); CHECK(handler.total_frame_received == 1); CHECK(nread == 12); + } - free_MIOBuffer(buf); + SECTION("Test good case with a multibyte frame type encoding") + { + uint8_t input[] = {// 1st frame (HEADERS) + 0xc0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x04, 0x11, 0x22, 0x33, 0x44, + // 2nd frame (DATA) + 0x00, 0x04, 0xaa, 0xbb, 0xcc, 0xdd, + // 3rd frame (incomplete) + 0xff}; + + // Initial state + CHECK(handler.total_frame_received == 0); + CHECK(nread == 0); + + SECTION("Write everything at once") + { + buf->write(input, sizeof(input)); + error = http3FrameDispatcher.on_read_ready(0, Http3StreamType::UNKNOWN, *reader, nread); + CHECK(!error); + CHECK(handler.total_frame_received == 1); + CHECK(nread == 19); + } + + SECTION("Write one byte at a time") + { + int total_nread{}; + for (uint8_t *it{input}; it < input + sizeof(input); ++it) { + buf->write(it, 1); + error = http3FrameDispatcher.on_read_ready(0, Http3StreamType::UNKNOWN, *reader, nread); + total_nread += nread; + CHECK(!error); + } + CHECK(handler.total_frame_received == 5); + CHECK(total_nread == 19); + } } + + free_MIOBuffer(buf); } TEST_CASE("control stream tests", "[http3]") @@ -276,6 +312,45 @@ TEST_CASE("control stream tests", "[http3]") } } +// 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; + + http3FrameDispatcher.add_handler(&handler); + + MIOBuffer *buf = new_MIOBuffer(BUFFER_SIZE_INDEX_512); + IOBufferReader *reader = buf->alloc_reader(); + uint64_t nread = 0; + Http3ErrorUPtr error = Http3ErrorUPtr(nullptr); + + 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); + + free_MIOBuffer(buf); +} + TEST_CASE("ignore unknown frames", "[http3]") { SECTION("ignore unkown frame")
