Fix long http2 header values (#30379 An error occurred )

* Fix long HTTP2 header values

* Add unit test

* Update for comments

* Add corpus

* Automated change: Fix sanity tests

* Add comments

* Replace `std::size_t` with `size_t`

* Automated change: Fix sanity tests

* Update

* Update

Co-authored-by: ralphchung <ralphchung@users.noreply.github.com>

src/core/ext/transport/chttp2/transport/hpack_constants.h

@@ -17,7 +17,8 @@
 
 #include <grpc/support/port_platform.h>
 
-#include <stdint.h>
+#include <cstddef>
+#include <cstdint>
 
 namespace grpc_core {
 namespace hpack_constants {
@@ -33,6 +34,11 @@ static constexpr uint32_t EntriesForBytes(uint32_t bytes) noexcept {
   return (bytes + kEntryOverhead - 1) / kEntryOverhead;
 }
 
+static constexpr size_t SizeForEntry(size_t key_length,
+                                     size_t value_length) noexcept {
+  return key_length + value_length + kEntryOverhead;
+}
+
 static constexpr uint32_t kInitialTableEntries =
     EntriesForBytes(kInitialTableSize);
 }  // namespace hpack_constants

src/core/ext/transport/chttp2/transport/hpack_encoder.cc

@@ -431,13 +431,15 @@ void HPackCompressor::Framer::Encode(HttpSchemeMetadata,
 }
 
 void HPackCompressor::Framer::Encode(GrpcTraceBinMetadata, const Slice& slice) {
-  EncodeIndexedKeyWithBinaryValue(&compressor_->grpc_trace_bin_index_,
-                                  "grpc-trace-bin", slice.Ref());
+  EncodeRepeatingSliceValue(GrpcTraceBinMetadata::key(), slice,
+                            &compressor_->grpc_trace_bin_index_,
+                            HPackEncoderTable::MaxEntrySize());
 }
 
 void HPackCompressor::Framer::Encode(GrpcTagsBinMetadata, const Slice& slice) {
-  EncodeIndexedKeyWithBinaryValue(&compressor_->grpc_tags_bin_index_,
-                                  "grpc-tags-bin", slice.Ref());
+  EncodeRepeatingSliceValue(GrpcTagsBinMetadata::key(), slice,
+                            &compressor_->grpc_tags_bin_index_,
+                            HPackEncoderTable::MaxEntrySize());
 }
 
 void HPackCompressor::Framer::Encode(HttpStatusMetadata, uint32_t status) {
@@ -521,6 +523,18 @@ void HPackCompressor::Framer::EncodeIndexedKeyWithBinaryValue(
   }
 }
 
+void HPackCompressor::Framer::EncodeRepeatingSliceValue(
+    const absl::string_view& key, const Slice& slice, uint32_t* index,
+    size_t max_compression_size) {
+  if (hpack_constants::SizeForEntry(key.size(), slice.size()) >
+      max_compression_size) {
+    EmitLitHdrWithBinaryStringKeyNotIdx(Slice::FromStaticString(key),
+                                        slice.Ref());
+  } else {
+    EncodeIndexedKeyWithBinaryValue(index, key, slice.Ref());
+  }
+}
+
 void HPackCompressor::Framer::Encode(GrpcTimeoutMetadata, Timestamp deadline) {
   Timeout timeout = Timeout::FromDuration(deadline - ExecCtx::Get()->Now());
   for (auto it = compressor_->previous_timeouts_.begin();
@@ -553,7 +567,9 @@ void HPackCompressor::Framer::Encode(GrpcTimeoutMetadata, Timestamp deadline) {
 }
 
 void HPackCompressor::Framer::Encode(UserAgentMetadata, const Slice& slice) {
-  if (slice.length() > HPackEncoderTable::MaxEntrySize()) {
+  if (hpack_constants::SizeForEntry(UserAgentMetadata::key().size(),
+                                    slice.size()) >
+      HPackEncoderTable::MaxEntrySize()) {
     EmitLitHdrWithNonBinaryStringKeyNotIdx(
         Slice::FromStaticString(UserAgentMetadata::key()), slice.Ref());
     return;
@@ -562,9 +578,10 @@ void HPackCompressor::Framer::Encode(UserAgentMetadata, const Slice& slice) {
     compressor_->user_agent_ = slice.Ref();
     compressor_->user_agent_index_ = 0;
   }
-  EncodeAlwaysIndexed(
-      &compressor_->user_agent_index_, "user-agent", slice.Ref(),
-      10 /* user-agent */ + slice.size() + hpack_constants::kEntryOverhead);
+  EncodeAlwaysIndexed(&compressor_->user_agent_index_, UserAgentMetadata::key(),
+                      slice.Ref(),
+                      hpack_constants::SizeForEntry(
+                          UserAgentMetadata::key().size(), slice.size()));
 }
 
 void HPackCompressor::Framer::Encode(GrpcStatusMetadata,

src/core/ext/transport/chttp2/transport/hpack_encoder.h

@@ -154,6 +154,10 @@ class HPackCompressor {
     void EncodeIndexedKeyWithBinaryValue(uint32_t* index, absl::string_view key,
                                          Slice value);
 
+    void EncodeRepeatingSliceValue(const absl::string_view& key,
+                                   const Slice& slice, uint32_t* index,
+                                   size_t max_compression_size);
+
     size_t CurrentFrameSize() const;
     void Add(Slice slice);
     uint8_t* AddTiny(size_t len);

src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc

@@ -17,7 +17,6 @@
 #include "src/core/ext/transport/chttp2/transport/hpack_encoder_table.h"
 
 #include <algorithm>
-#include <cstdint>
 
 #include <grpc/support/log.h>
 

src/core/ext/transport/chttp2/transport/hpack_encoder_table.h

@@ -20,6 +20,8 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <limits>
+
 #include "absl/container/inlined_vector.h"
 
 #include "src/core/ext/transport/chttp2/transport/hpack_constants.h"
@@ -30,9 +32,13 @@ namespace grpc_core {
 // sizes.
 class HPackEncoderTable {
  public:
+  using EntrySize = uint16_t;
+
   HPackEncoderTable() : elem_size_(hpack_constants::kInitialTableEntries) {}
 
-  static constexpr size_t MaxEntrySize() { return 65535; }
+  static constexpr size_t MaxEntrySize() {
+    return std::numeric_limits<EntrySize>::max();
+  }
 
   // Reserve space in table for the new element, evict entries if needed.
   // Return the new index of the element. Return 0 to indicate not adding to
@@ -65,7 +71,7 @@ class HPackEncoderTable {
   uint32_t table_elems_ = 0;
   uint32_t table_size_ = 0;
   // The size of each element in the HPACK table.
-  absl::InlinedVector<uint16_t, hpack_constants::kInitialTableEntries>
+  absl::InlinedVector<EntrySize, hpack_constants::kInitialTableEntries>
       elem_size_;
 };
 

test/core/transport/chttp2/hpack_encoder_test.cc

@@ -21,8 +21,10 @@
 #include <stdio.h>
 #include <string.h>
 
+#include <memory>
 #include <string>
 
+#include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
 #include "absl/strings/str_cat.h"
@@ -152,73 +154,155 @@ static void CrashOnAppendError(absl::string_view, const grpc_core::Slice&) {
   abort();
 }
 
-/* verify that the output generated by encoding the stream matches the
-   hexstring passed in */
-static void verify(const verify_params params, const char* expected,
-                   size_t nheaders, ...) {
-  grpc_slice_buffer output;
-  grpc_slice merged;
-  grpc_slice expect = parse_hexstring(expected);
-  size_t i;
-  va_list l;
+grpc_slice EncodeHeaderIntoBytes(
+    bool is_eof,
+    const std::vector<std::pair<std::string, std::string>>& header_fields) {
+  std::unique_ptr<grpc_core::HPackCompressor> compressor =
+      std::make_unique<grpc_core::HPackCompressor>();
+
   auto arena = grpc_core::MakeScopedArena(1024, g_memory_allocator);
   grpc_metadata_batch b(arena.get());
 
-  va_start(l, nheaders);
-  for (i = 0; i < nheaders; i++) {
-    char* key = va_arg(l, char*);
-    char* value = va_arg(l, char*);
-    b.Append(key, grpc_core::Slice::FromStaticString(value),
+  for (const auto& field : header_fields) {
+    b.Append(field.first,
+             grpc_core::Slice::FromStaticString(field.second.c_str()),
              CrashOnAppendError);
   }
-  va_end(l);
 
-  grpc_slice_buffer_init(&output);
-
-  grpc_transport_one_way_stats stats;
-  stats = {};
+  grpc_transport_one_way_stats stats = {};
   grpc_core::HPackCompressor::EncodeHeaderOptions hopt{
-      0xdeadbeef,                      /* stream_id */
-      params.eof,                      /* is_eof */
-      params.use_true_binary_metadata, /* use_true_binary_metadata */
-      16384,                           /* max_frame_size */
-      &stats                           /* stats */
+      0xdeadbeef, /* stream_id */
+      is_eof,     /* is_eof */
+      false,      /* use_true_binary_metadata */
+      16384,      /* max_frame_size */
+      &stats      /* stats */
   };
-  g_compressor->EncodeHeaders(hopt, b, &output);
-  verify_frames(output, params.eof);
-  merged = grpc_slice_merge(output.slices, output.count);
+  grpc_slice_buffer output;
+  grpc_slice_buffer_init(&output);
+
+  compressor->EncodeHeaders(hopt, b, &output);
+  verify_frames(output, is_eof);
+
+  grpc_slice ret = grpc_slice_merge(output.slices, output.count);
   grpc_slice_buffer_destroy_internal(&output);
 
-  if (!grpc_slice_eq(merged, expect)) {
-    char* expect_str = grpc_dump_slice(expect, GPR_DUMP_HEX | GPR_DUMP_ASCII);
-    char* got_str = grpc_dump_slice(merged, GPR_DUMP_HEX | GPR_DUMP_ASCII);
-    gpr_log(GPR_ERROR, "mismatched output for %s", expected);
-    gpr_log(GPR_ERROR, "EXPECT: %s", expect_str);
-    gpr_log(GPR_ERROR, "GOT:    %s", got_str);
-    gpr_free(expect_str);
-    gpr_free(got_str);
-    EXPECT_TRUE(false);
-  }
+  return ret;
+}
 
-  grpc_slice_unref_internal(merged);
-  grpc_slice_unref_internal(expect);
+/* verify that the output generated by encoding the stream matches the
+   hexstring passed in */
+static void verify(
+    bool is_eof, const char* expected,
+    const std::vector<std::pair<std::string, std::string>>& header_fields) {
+  const grpc_core::Slice merged(EncodeHeaderIntoBytes(is_eof, header_fields));
+  const grpc_core::Slice expect(parse_hexstring(expected));
+
+  EXPECT_EQ(merged, expect);
 }
 
 TEST(HpackEncoderTest, TestBasicHeaders) {
   grpc_core::ExecCtx exec_ctx;
   g_compressor = new grpc_core::HPackCompressor();
 
-  verify_params params = {
-      false,
-      false,
-  };
-  verify(params, "000005 0104 deadbeef 00 0161 0161", 1, "a", "a");
-  verify(params, "00000a 0104 deadbeef 00 0161 0161 00 0162 0163", 2, "a", "a",
-         "b", "c");
+  verify(false, "000005 0104 deadbeef 00 0161 0161", {{"a", "a"}});
+  verify(false, "00000a 0104 deadbeef 00 0161 0161 00 0162 0163",
+         {{"a", "a"}, {"b", "c"}});
 
   delete g_compressor;
 }
 
+MATCHER(HasLiteralHeaderFieldNewNameFlagIncrementalIndexing, "") {
+  constexpr size_t kHttp2FrameHeaderSize = 9u;
+  /// Reference: https://httpwg.org/specs/rfc7541.html#rfc.section.6.2.1
+  /// The first byte of a literal header field with incremental indexing should
+  /// be 0x40.
+  constexpr uint8_t kLiteralHeaderFieldNewNameFlagIncrementalIndexing = 0x40;
+  return (GRPC_SLICE_START_PTR(arg)[kHttp2FrameHeaderSize] ==
+          kLiteralHeaderFieldNewNameFlagIncrementalIndexing);
+}
+
+MATCHER(HasLiteralHeaderFieldNewNameFlagNoIndexing, "") {
+  constexpr size_t kHttp2FrameHeaderSize = 9u;
+  /// Reference: https://httpwg.org/specs/rfc7541.html#rfc.section.6.2.2
+  /// The first byte of a literal header field without indexing should be 0x0.
+  constexpr uint8_t kLiteralHeaderFieldNewNameFlagNoIndexing = 0x00;
+  return (GRPC_SLICE_START_PTR(arg)[kHttp2FrameHeaderSize] ==
+          kLiteralHeaderFieldNewNameFlagNoIndexing);
+}
+
+TEST(HpackEncoderTest, GrpcTraceBinMetadataIndexing) {
+  grpc_core::ExecCtx exec_ctx;
+
+  const grpc_slice encoded_header = EncodeHeaderIntoBytes(
+      false, {{grpc_core::GrpcTraceBinMetadata::key().data(), "value"}});
+  EXPECT_THAT(encoded_header,
+              HasLiteralHeaderFieldNewNameFlagIncrementalIndexing());
+
+  grpc_slice_unref_internal(encoded_header);
+}
+
+TEST(HpackEncoderTest, GrpcTraceBinMetadataNoIndexing) {
+  grpc_core::ExecCtx exec_ctx;
+
+  /// needs to be greater than `HPackEncoderTable::MaxEntrySize()`
+  constexpr size_t long_value_size = 70000u;
+  const grpc_slice encoded_header = EncodeHeaderIntoBytes(
+      false, {{grpc_core::GrpcTraceBinMetadata::key().data(),
+               std::string(long_value_size, 'a')}});
+  EXPECT_THAT(encoded_header, HasLiteralHeaderFieldNewNameFlagNoIndexing());
+
+  grpc_slice_unref_internal(encoded_header);
+}
+
+TEST(HpackEncoderTest, TestGrpcTagsBinMetadataIndexing) {
+  grpc_core::ExecCtx exec_ctx;
+
+  const grpc_slice encoded_header = EncodeHeaderIntoBytes(
+      false,
+      {{grpc_core::GrpcTagsBinMetadata::key().data(), std::string("value")}});
+  EXPECT_THAT(encoded_header,
+              HasLiteralHeaderFieldNewNameFlagIncrementalIndexing());
+
+  grpc_slice_unref_internal(encoded_header);
+}
+
+TEST(HpackEncoderTest, TestGrpcTagsBinMetadataNoIndexing) {
+  grpc_core::ExecCtx exec_ctx;
+
+  /// needs to be greater than `HPackEncoderTable::MaxEntrySize()`
+  constexpr size_t long_value_size = 70000u;
+  const grpc_slice encoded_header = EncodeHeaderIntoBytes(
+      false, {{grpc_core::GrpcTagsBinMetadata::key().data(),
+               std::string(long_value_size, 'a')}});
+  EXPECT_THAT(encoded_header, HasLiteralHeaderFieldNewNameFlagNoIndexing());
+
+  grpc_slice_unref_internal(encoded_header);
+}
+
+TEST(HpackEncoderTest, UserAgentMetadataIndexing) {
+  grpc_core::ExecCtx exec_ctx;
+
+  const grpc_slice encoded_header = EncodeHeaderIntoBytes(
+      false, {{grpc_core::UserAgentMetadata::key().data(), "value"}});
+  EXPECT_THAT(encoded_header,
+              HasLiteralHeaderFieldNewNameFlagIncrementalIndexing());
+
+  grpc_slice_unref_internal(encoded_header);
+}
+
+TEST(HpackEncoderTest, UserAgentMetadataNoIndexing) {
+  grpc_core::ExecCtx exec_ctx;
+
+  /// needs to be greater than `HPackEncoderTable::MaxEntrySize()`
+  constexpr size_t long_value_size = 70000u;
+  const grpc_slice encoded_header =
+      EncodeHeaderIntoBytes(false, {{grpc_core::UserAgentMetadata::key().data(),
+                                     std::string(long_value_size, 'a')}});
+  EXPECT_THAT(encoded_header, HasLiteralHeaderFieldNewNameFlagNoIndexing());
+
+  grpc_slice_unref_internal(encoded_header);
+}
+
 static void verify_continuation_headers(const char* key, const char* value,
                                         bool is_eof) {
   auto arena = grpc_core::MakeScopedArena(1024, g_memory_allocator);