This is an automated email from the ASF dual-hosted git repository.
jvanderzee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new f5d9149992 Parse frame types consisting of multiple bytes (#11611)
f5d9149992 is described below
commit f5d9149992bb26e5d8378c0b49486995052ec402
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.
---
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 bdda7cb2f1..e81175efe1 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]")
@@ -225,6 +261,45 @@ TEST_CASE("control stream tests", "[http3]")
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;
+
+ 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")
