QPACK: RFC 9204 static-table compliance, decode robustness, and cleanup

src/proxy/http3/Http3Config.cc

@@ -23,6 +23,8 @@
 
 #include "proxy/http3/Http3Config.h"
 
+#include "tscore/Diags.h"
+
 int ts::Http3Config::_config_id = 0;
 
 //
@@ -36,6 +38,16 @@ ts::Http3ConfigParams::initialize()
   RecEstablishStaticConfigUInt32(this->_qpack_blocked_streams, "proxy.config.http3.qpack_blocked_streams");
   RecEstablishStaticConfigUInt32(this->_num_placeholders, "proxy.config.http3.num_placeholders");
   RecEstablishStaticConfigUInt32(this->_max_settings, "proxy.config.http3.max_settings");
+
+  // The QPACK dynamic table is not implemented: XpackDynamicTable never stores
+  // entries (its capacity is fixed at zero and never raised). Advertising a
+  // non-zero table size or blocked-stream count would tell a peer it may
+  // insert entries our decoder silently drops and then reference them, which
+  // breaks decoding. Refuse to start until the dynamic table is implemented.
+  if (this->_header_table_size != 0 || this->_qpack_blocked_streams != 0) {
+    Fatal("HTTP/3 QPACK dynamic table is not implemented; proxy.config.http3.header_table_size and "
+          "proxy.config.http3.qpack_blocked_streams must be 0");
+  }
 }
 
 uint32_t

src/proxy/http3/QPACK.cc

@@ -27,6 +27,10 @@
 #include "tscore/ink_defs.h"
 #include "tscore/ink_memory.h"
 
+#include <algorithm>
+#include <array>
+#include <string_view>
+
 #define QPACKDebug(fmt, ...)   Dbg(dbg_ctl_qpack, "[%s] " fmt, this->_qc->cids().data(), ##__VA_ARGS__)
 #define QPACKDTDebug(fmt, ...) Dbg(dbg_ctl_qpack, "" fmt, ##__VA_ARGS__)
 
@@ -69,7 +73,7 @@ const QPACK::Header QPACK::StaticTable::STATIC_HEADER_FIELDS[] = {
   {":status",                          "503"                                                  },
   {"accept",                           "*/*"                                                  },
   {"accept",                           "application/dns-message"                              },
-  {"accept-encoding",                  "gzip, deflate, br, zstd"                              },
+  {"accept-encoding",                  "gzip, deflate, br"                                    },
   {"accept-ranges",                    "bytes"                                                },
   {"access-control-allow-headers",     "cache-control"                                        },
   {"access-control-allow-headers",     "content-type"                                         },
@@ -80,7 +84,6 @@ const QPACK::Header QPACK::StaticTable::STATIC_HEADER_FIELDS[] = {
   {"cache-control",                    "no-cache"                                             },
   {"cache-control",                    "no-store"                                             },
   {"cache-control",                    "public, max-age=31536000"                             },
-  {"content-encoding",                 "zstd"                                                 },
   {"content-encoding",                 "br"                                                   },
   {"content-encoding",                 "gzip"                                                 },
   {"content-type",                     "application/dns-message"                              },
@@ -283,11 +286,20 @@ QPACK::decode(uint64_t stream_id, const uint8_t *header_block, size_t header_blo
 
   uint64_t tmp = 0;
   int64_t  ret = xpack_decode_integer(tmp, header_block, header_block + header_block_len, 8);
-  if (ret < 0 && tmp > 0xFFFF) {
+  if (ret < 0 || tmp > 0xFFFF) {
     return -1;
   }
   uint16_t largest_reference = tmp;
 
+  // A non-zero Required Insert Count references the dynamic table. We only
+  // store dynamic entries once a non-zero table capacity is negotiated (and we
+  // currently always advertise zero), so when the table capacity is zero such
+  // a reference can never be satisfied. Treat it as a decoding failure instead
+  // of queuing the stream as blocked forever.
+  if (largest_reference != 0 && this->_dynamic_table.maximum_size() == 0) {
+    return -1;
+  }
+
   if (largest_reference != 0 && (this->_dynamic_table.is_empty() || this->_dynamic_table.largest_index() < largest_reference)) {
     // Blocked
     if (this->_add_to_blocked_list(
@@ -913,21 +925,21 @@ QPACK::_decode_literal_header_field_with_postbase_name_ref(int16_t base_index, c
 int
 QPACK::_decode_header(const uint8_t *header_block, size_t header_block_len, HTTPHdr &hdr)
 {
-  const uint8_t *pos        = header_block;
-  size_t         remain_len = header_block_len;
-  int64_t        ret;
+  const uint8_t       *pos = header_block;
+  const uint8_t *const end = header_block + header_block_len;
+  int64_t              ret;
 
   // Decode Header Data Prefix
-  uint64_t tmp;
-  if ((ret = xpack_decode_integer(tmp, pos, pos + remain_len, 8)) < 0 && tmp > 0xFFFF) {
+  uint64_t tmp = 0;
+  if ((ret = xpack_decode_integer(tmp, pos, end, 8)) < 0 || tmp > 0xFFFF) {
     return -1;
   }
   pos                        += ret;
   uint16_t largest_reference  = tmp;
 
-  uint64_t delta_base_index;
+  uint64_t delta_base_index = 0;
   uint16_t base_index;
-  if ((ret = xpack_decode_integer(delta_base_index, pos, pos + remain_len, 7)) < 0 && delta_base_index < 0xFFFF) {
+  if ((ret = xpack_decode_integer(delta_base_index, pos, end, 7)) < 0 || delta_base_index > 0xFFFF) {
     return -2;
   }
 
@@ -944,7 +956,8 @@ QPACK::_decode_header(const uint8_t *header_block, size_t header_block_len, HTTP
   uint32_t decoded_header_list_size = 0;
 
   // Decode Instructions
-  while (pos < header_block + header_block_len) {
+  while (pos < end) {
+    size_t   remain_len = end - pos;
     uint32_t header_len = 0;
 
     if (pos[0] & 0x80) { // Index Header Field
@@ -1226,26 +1239,42 @@ QPACK::StaticTable::lookup(uint16_t index, const char **name, size_t *name_len,
 const XpackLookupResult
 QPACK::StaticTable::lookup(const char *name, size_t name_len, const char *value, size_t value_len)
 {
+  // The static table (RFC 9204 Appendix A) is not ordered by name and its
+  // indices are part of the wire encoding, so it cannot be reordered. Keep an
+  // auxiliary index sorted by name (ties broken by ascending table index) so a
+  // lookup scans only the entries sharing the requested name instead of all of
+  // them. Returns the same index the former linear scan did: the sole exact
+  // match, or the highest-indexed name match otherwise.
+  static const auto name_order = [] {
+    std::array<uint16_t, sizeof(STATIC_HEADER_FIELDS) / sizeof(STATIC_HEADER_FIELDS[0])> order;
+    for (uint16_t i = 0; i < order.size(); ++i) {
+      order[i] = i;
+    }
+    std::sort(order.begin(), order.end(), [](uint16_t l, uint16_t r) {
+      const std::string_view ln{STATIC_HEADER_FIELDS[l].name, STATIC_HEADER_FIELDS[l].name_len};
+      const std::string_view rn{STATIC_HEADER_FIELDS[r].name, STATIC_HEADER_FIELDS[r].name_len};
+      return ln != rn ? ln < rn : l < r;
+    });
+    return order;
+  }();
+
+  const std::string_view target{name, name_len};
+  auto                   it = std::lower_bound(name_order.begin(), name_order.end(), target, [](uint16_t idx, std::string_view t) {
+    return std::string_view{STATIC_HEADER_FIELDS[idx].name, STATIC_HEADER_FIELDS[idx].name_len} < t;
+  });
+
   XpackLookupResult::MatchType match_type      = XpackLookupResult::MatchType::NONE;
-  uint16_t                     i               = 0;
   uint16_t                     candidate_index = 0;
-  int                          n               = countof(STATIC_HEADER_FIELDS);
-
-  for (; i < n; ++i) {
-    const Header &h = STATIC_HEADER_FIELDS[i];
-    if (h.name_len == name_len) {
-      if (memcmp(name, h.name, name_len) == 0) {
-        candidate_index = i;
-        if (value_len == h.value_len && memcmp(value, h.value, value_len) == 0) {
-          // Exact match
-          match_type = XpackLookupResult::MatchType::EXACT;
-          break;
-        } else {
-          // Name match -- Keep it for no exact matches
-          match_type = XpackLookupResult::MatchType::NAME;
-        }
-      }
+  for (; it != name_order.end(); ++it) {
+    const Header &h = STATIC_HEADER_FIELDS[*it];
+    if (h.name_len != name_len || memcmp(h.name, name, name_len) != 0) {
+      break; // Past the run of entries sharing this name.
+    }
+    candidate_index = *it;
+    if (value_len == h.value_len && memcmp(value, h.value, value_len) == 0) {
+      return {candidate_index, XpackLookupResult::MatchType::EXACT};
     }
+    match_type = XpackLookupResult::MatchType::NAME;
   }
   return {candidate_index, match_type};
 }

src/proxy/http3/test/test_QPACK.cc

@@ -469,3 +469,83 @@ TEST_CASE("Decoding", "[qpack-decode]")
     }
   }
 }
+
+// Encode an "Indexed Header Field" (RFC 9204 4.5.2) that references a
+// static-table entry, returning the number of bytes written.
+static int
+append_static_indexed_field(uint8_t *buf, const uint8_t *buf_end, uint16_t index)
+{
+  int64_t n  = xpack_encode_integer(buf, buf_end, index, 6);
+  buf[0]    |= 0xC0; // '1' Indexed Header Field + 'T'=1 static table
+  return static_cast<int>(n);
+}
+
+// The QPACK static table (RFC 9204 Appendix A) is normative: its 99 indices are
+// wire values shared with every peer and must not be reordered. These cases
+// span the range a previous change shifted by inserting a non-standard entry;
+// decoding each index must yield the exact RFC name and value.
+TEST_CASE("QPACK static table conforms to RFC 9204", "[qpack]")
+{
+  struct {
+    uint16_t    index;
+    const char *name;
+    const char *value;
+  } const cases[] = {
+    {31, "accept-encoding",  "gzip, deflate, br"       },
+    {42, "content-encoding", "br"                      },
+    {52, "content-type",     "text/html; charset=utf-8"},
+    {71, ":status",          "500"                     },
+    {98, "x-frame-options",  "sameorigin"              },
+  };
+
+  QUICApplicationDriver  driver;
+  QPACK                 *qpack         = new QPACK(driver.get_connection(), UINT32_MAX, 0, 0);
+  TestQPACKEventHandler *event_handler = new TestQPACKEventHandler();
+
+  HTTPHdr hdr;
+  hdr.create(HTTPType::RESPONSE);
+
+  uint8_t block[256];
+  int     len  = 0;
+  block[len++] = 0x00; // Required Insert Count = 0
+  block[len++] = 0x00; // S = 0, Delta Base = 0
+  for (auto const &c : cases) {
+    len += append_static_indexed_field(block + len, block + sizeof(block), c.index);
+  }
+
+  // decode() populates the header set synchronously; only the completion event
+  // is scheduled, so the fields can be inspected as soon as it returns.
+  int ret = qpack->decode(1, block, len, hdr, event_handler, eventProcessor.all_ethreads[0]);
+  REQUIRE(ret == 0);
+
+  for (auto const &c : cases) {
+    MIMEField *field = hdr.field_find(std::string_view{c.name});
+    REQUIRE(field != nullptr);
+    CHECK(field->value_get() == std::string_view{c.value});
+  }
+
+  hdr.destroy();
+}
+
+// ATS advertises a zero-capacity QPACK dynamic table, so a header block whose
+// Required Insert Count is non-zero references entries that can never exist.
+// decode() must fail rather than queue the stream as blocked forever.
+TEST_CASE("QPACK decode rejects a dynamic reference with no dynamic table", "[qpack]")
+{
+  QUICApplicationDriver  driver;
+  QPACK                 *qpack         = new QPACK(driver.get_connection(), UINT32_MAX, 0, 0);
+  TestQPACKEventHandler *event_handler = new TestQPACKEventHandler();
+
+  HTTPHdr hdr;
+  hdr.create(HTTPType::REQUEST);
+
+  uint8_t block[8];
+  int     len   = 0;
+  len          += xpack_encode_integer(block + len, block + sizeof(block), 1, 8); // Required Insert Count = 1
+  block[len++]  = 0x00;                                                           // S = 0, Delta Base = 0
+
+  int ret = qpack->decode(1, block, len, hdr, event_handler, eventProcessor.all_ethreads[0]);
+  CHECK(ret < 0);
+
+  hdr.destroy();
+}