Fix COMPRESSION_ERROR on valid HPACK input

proxy/hdrs/HuffmanCodec.cc

@@ -149,46 +149,61 @@ hpack_huffman_fin()
   }
 }
 
+#define MAX_HUFFMAN_CODE_LEN 30
+#define EOS 0x3fffffff
+
 int64_t
 huffman_decode(char *dst_start, const uint8_t *src, uint32_t src_len)
 {
-  char *dst_end             = dst_start;
-  uint8_t shift             = 7;
-  Node *current             = HUFFMAN_TREE_ROOT;
-  int byte_boundary_crossed = 0;
-  bool includes_zero        = false;
+  char *dst_end      = dst_start;
+  uint8_t shift      = 7;
+  Node *current      = HUFFMAN_TREE_ROOT;
+  int nbits          = 0;
+  uint32_t curr_bits = 0;
 
   while (src_len) {
+    if (nbits > 0) {
+      curr_bits <<= 1;
+    }
     if (*src & (1 << shift)) {
+      curr_bits |= 1;
       current = current->right;
     } else {
-      current       = current->left;
-      includes_zero = true;
+      current = current->left;
     }
+    ++nbits;
 
     if (current->leaf_node == true) {
-      *dst_end = current->ascii_code;
+      if (curr_bits == EOS) {
+        return -1;
+      }
+      nbits     = 0;
+      curr_bits = 0;
+      *dst_end  = current->ascii_code;
       ++dst_end;
-      current               = HUFFMAN_TREE_ROOT;
-      byte_boundary_crossed = 0;
-      includes_zero         = false;
+      current = HUFFMAN_TREE_ROOT;
     }
+
     if (shift) {
       --shift;
     } else {
       shift = 7;
       ++src;
       --src_len;
-      ++byte_boundary_crossed;
     }
-    if (byte_boundary_crossed > 3) {
+
+    if (nbits > MAX_HUFFMAN_CODE_LEN) {
       return -1;
     }
   }
-  if (byte_boundary_crossed > 1) {
+
+  if (nbits > 7) {
     return -1;
   }
-  if (includes_zero) {
+
+  // Padding bits must be a prefix of EOS
+  uint8_t mask = (1 << nbits) - 1;
+  if ((mask & curr_bits) != mask) {
     return -1;
   }
 

proxy/hdrs/test_Huffmancode.cc

@@ -136,6 +136,12 @@ values_test()
 
     int bytes        = huffman_decode(dst_start, encoded_mapped.y, encoded_size);
     char ascii_value = i / 2;
+
+    // EOS is treated as invalid so check for an error
+    if (value == 0x3fffffff) {
+      assert(bytes == -1);
+      continue;
+    }
     assert(dst_start[0] == ascii_value);
     assert(bytes == 1);
   }
@@ -171,6 +177,25 @@ encode_test()
   }
 }
 
+void
+decode_errors_test()
+{
+  const static struct {
+    char *input;
+    int input_len;
+  } test_cases[] = {
+    {(char *)"\x00", 1}, {(char *)"\xff", 1}, {(char *)"\x1f\xff", 2}, {(char *)"\xff\xae", 2}, {(char *)"\xff\x9f\xff\xff\xff", 5},
+  };
+
+  for (unsigned int i = 0; i < sizeof(test_cases) / sizeof(test_cases[0]); i++) {
+    int dst_len = 2 * test_cases[i].input_len;
+    char *dst   = (char *)malloc(dst_len);
+    int len     = huffman_decode(dst, (uint8_t *)test_cases[i].input, test_cases[i].input_len);
+    assert(len == -1);
+    free(dst);
+  }
+}
+
 int
 main()
 {
@@ -180,6 +205,7 @@ main()
     random_test();
   }
   values_test();
+  decode_errors_test();
 
   hpack_huffman_fin();
 

proxy/hdrs/unit_tests/test_XPACK.cc

@@ -73,22 +73,28 @@ TEST_CASE("XPACK_String", "[xpack]")
     uint32_t raw_string_len;
     uint8_t *encoded_field;
     int encoded_field_len;
-  } string_test_case[] = {{(char *)"", 0,
-                           (uint8_t *)"\x0"
-                                      "",
-                           1},
-                          {(char *)"custom-key", 10,
-                           (uint8_t *)"\xA"
-                                      "custom-key",
-                           11},
-                          {(char *)"", 0,
-                           (uint8_t *)"\x80"
-                                      "",
-                           1},
-                          {(char *)"custom-key", 10,
-                           (uint8_t *)"\x88"
-                                      "\x25\xa8\x49\xe9\x5b\xa9\x7d\x7f",
-                           9}};
+  } string_test_case[] = {
+    {(char *)"", 0,
+     (uint8_t *)"\x0"
+                "",
+     1},
+    {(char *)"custom-key", 10,
+     (uint8_t *)"\xA"
+                "custom-key",
+     11},
+    {(char *)"", 0,
+     (uint8_t *)"\x80"
+                "",
+     1},
+    {(char *)"custom-key", 10,
+     (uint8_t *)"\x88"
+                "\x25\xa8\x49\xe9\x5b\xa9\x7d\x7f",
+     9},
+    {(char *)"cw Times New Roman_σ=1", 23,
+     (uint8_t *)"\x95"
+                "\x27\x85\x37\x9a\x92\xa1\x4d\x25\xf0\xa6\xd3\xd2\x3a\xa2\xff\xff\xf6\xff\xff\x44\x01",
+     22},
+  };
 
   SECTION("Encoding")
   {