[Core] Backport:1.56 Improve HPACK (#33603)

Cherry-pick https://github.com/grpc/grpc/pull/33597

BUILD

@@ -3699,23 +3699,47 @@ grpc_cc_library(
         "//src/core:ext/transport/chttp2/transport/hpack_parser_table.h",
     ],
     external_deps = [
+        "absl/functional:function_ref",
         "absl/status",
         "absl/strings",
-        "absl/strings:str_format",
     ],
     deps = [
         "gpr",
         "gpr_platform",
         "grpc_base",
         "grpc_trace",
+        "hpack_parse_result",
         "http_trace",
-        "//src/core:error",
         "//src/core:hpack_constants",
         "//src/core:no_destruct",
         "//src/core:slice",
     ],
 )
 
+grpc_cc_library(
+    name = "hpack_parse_result",
+    srcs = [
+        "//src/core:ext/transport/chttp2/transport/hpack_parse_result.cc",
+    ],
+    hdrs = [
+        "//src/core:ext/transport/chttp2/transport/hpack_parse_result.h",
+    ],
+    external_deps = [
+        "absl/status",
+        "absl/strings",
+        "absl/strings:str_format",
+        "absl/types:optional",
+    ],
+    deps = [
+        "gpr",
+        "grpc_base",
+        "//src/core:error",
+        "//src/core:hpack_constants",
+        "//src/core:slice",
+        "//src/core:status_helper",
+    ],
+)
+
 grpc_cc_library(
     name = "hpack_parser",
     srcs = [
@@ -3728,7 +3752,6 @@ grpc_cc_library(
         "absl/base:core_headers",
         "absl/status",
         "absl/strings",
-        "absl/strings:str_format",
         "absl/types:optional",
         "absl/types:span",
         "absl/types:variant",
@@ -3740,16 +3763,17 @@ grpc_cc_library(
         "grpc_base",
         "grpc_public_hdrs",
         "grpc_trace",
+        "hpack_parse_result",
         "hpack_parser_table",
         "stats",
         "//src/core:decode_huff",
         "//src/core:error",
         "//src/core:hpack_constants",
+        "//src/core:match",
         "//src/core:random_early_detection",
         "//src/core:slice",
         "//src/core:slice_refcount",
         "//src/core:stats_data",
-        "//src/core:status_helper",
     ],
 )
 

CMakeLists.txt

@@ -1677,6 +1677,7 @@ add_library(grpc
   src/core/ext/transport/chttp2/transport/frame_window_update.cc
   src/core/ext/transport/chttp2/transport/hpack_encoder.cc
   src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc
+  src/core/ext/transport/chttp2/transport/hpack_parse_result.cc
   src/core/ext/transport/chttp2/transport/hpack_parser.cc
   src/core/ext/transport/chttp2/transport/hpack_parser_table.cc
   src/core/ext/transport/chttp2/transport/http2_settings.cc
@@ -2696,6 +2697,7 @@ add_library(grpc_unsecure
   src/core/ext/transport/chttp2/transport/frame_window_update.cc
   src/core/ext/transport/chttp2/transport/hpack_encoder.cc
   src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc
+  src/core/ext/transport/chttp2/transport/hpack_parse_result.cc
   src/core/ext/transport/chttp2/transport/hpack_parser.cc
   src/core/ext/transport/chttp2/transport/hpack_parser_table.cc
   src/core/ext/transport/chttp2/transport/http2_settings.cc
@@ -11486,6 +11488,7 @@ add_executable(frame_test
   src/core/ext/transport/chttp2/transport/decode_huff.cc
   src/core/ext/transport/chttp2/transport/hpack_encoder.cc
   src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc
+  src/core/ext/transport/chttp2/transport/hpack_parse_result.cc
   src/core/ext/transport/chttp2/transport/hpack_parser.cc
   src/core/ext/transport/chttp2/transport/hpack_parser_table.cc
   src/core/ext/transport/chttp2/transport/http_trace.cc

Makefile

@@ -1058,6 +1058,7 @@ LIBGRPC_SRC = \
     src/core/ext/transport/chttp2/transport/frame_window_update.cc \
     src/core/ext/transport/chttp2/transport/hpack_encoder.cc \
     src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc \
+    src/core/ext/transport/chttp2/transport/hpack_parse_result.cc \
     src/core/ext/transport/chttp2/transport/hpack_parser.cc \
     src/core/ext/transport/chttp2/transport/hpack_parser_table.cc \
     src/core/ext/transport/chttp2/transport/http2_settings.cc \
@@ -1931,6 +1932,7 @@ LIBGRPC_UNSECURE_SRC = \
     src/core/ext/transport/chttp2/transport/frame_window_update.cc \
     src/core/ext/transport/chttp2/transport/hpack_encoder.cc \
     src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc \
+    src/core/ext/transport/chttp2/transport/hpack_parse_result.cc \
     src/core/ext/transport/chttp2/transport/hpack_parser.cc \
     src/core/ext/transport/chttp2/transport/hpack_parser_table.cc \
     src/core/ext/transport/chttp2/transport/http2_settings.cc \

build_autogenerated.yaml

@@ -299,6 +299,7 @@ libs:
   - src/core/ext/transport/chttp2/transport/hpack_constants.h
   - src/core/ext/transport/chttp2/transport/hpack_encoder.h
   - src/core/ext/transport/chttp2/transport/hpack_encoder_table.h
+  - src/core/ext/transport/chttp2/transport/hpack_parse_result.h
   - src/core/ext/transport/chttp2/transport/hpack_parser.h
   - src/core/ext/transport/chttp2/transport/hpack_parser_table.h
   - src/core/ext/transport/chttp2/transport/http2_settings.h
@@ -1107,6 +1108,7 @@ libs:
   - src/core/ext/transport/chttp2/transport/frame_window_update.cc
   - src/core/ext/transport/chttp2/transport/hpack_encoder.cc
   - src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc
+  - src/core/ext/transport/chttp2/transport/hpack_parse_result.cc
   - src/core/ext/transport/chttp2/transport/hpack_parser.cc
   - src/core/ext/transport/chttp2/transport/hpack_parser_table.cc
   - src/core/ext/transport/chttp2/transport/http2_settings.cc
@@ -1999,6 +2001,7 @@ libs:
   - src/core/ext/transport/chttp2/transport/hpack_constants.h
   - src/core/ext/transport/chttp2/transport/hpack_encoder.h
   - src/core/ext/transport/chttp2/transport/hpack_encoder_table.h
+  - src/core/ext/transport/chttp2/transport/hpack_parse_result.h
   - src/core/ext/transport/chttp2/transport/hpack_parser.h
   - src/core/ext/transport/chttp2/transport/hpack_parser_table.h
   - src/core/ext/transport/chttp2/transport/http2_settings.h
@@ -2412,6 +2415,7 @@ libs:
   - src/core/ext/transport/chttp2/transport/frame_window_update.cc
   - src/core/ext/transport/chttp2/transport/hpack_encoder.cc
   - src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc
+  - src/core/ext/transport/chttp2/transport/hpack_parse_result.cc
   - src/core/ext/transport/chttp2/transport/hpack_parser.cc
   - src/core/ext/transport/chttp2/transport/hpack_parser_table.cc
   - src/core/ext/transport/chttp2/transport/http2_settings.cc
@@ -7457,6 +7461,7 @@ targets:
   - src/core/ext/transport/chttp2/transport/hpack_constants.h
   - src/core/ext/transport/chttp2/transport/hpack_encoder.h
   - src/core/ext/transport/chttp2/transport/hpack_encoder_table.h
+  - src/core/ext/transport/chttp2/transport/hpack_parse_result.h
   - src/core/ext/transport/chttp2/transport/hpack_parser.h
   - src/core/ext/transport/chttp2/transport/hpack_parser_table.h
   - src/core/ext/transport/chttp2/transport/http_trace.h
@@ -7737,6 +7742,7 @@ targets:
   - src/core/ext/transport/chttp2/transport/decode_huff.cc
   - src/core/ext/transport/chttp2/transport/hpack_encoder.cc
   - src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc
+  - src/core/ext/transport/chttp2/transport/hpack_parse_result.cc
   - src/core/ext/transport/chttp2/transport/hpack_parser.cc
   - src/core/ext/transport/chttp2/transport/hpack_parser_table.cc
   - src/core/ext/transport/chttp2/transport/http_trace.cc

config.m4

@@ -137,6 +137,7 @@ if test "$PHP_GRPC" != "no"; then
     src/core/ext/transport/chttp2/transport/frame_window_update.cc \
     src/core/ext/transport/chttp2/transport/hpack_encoder.cc \
     src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc \
+    src/core/ext/transport/chttp2/transport/hpack_parse_result.cc \
     src/core/ext/transport/chttp2/transport/hpack_parser.cc \
     src/core/ext/transport/chttp2/transport/hpack_parser_table.cc \
     src/core/ext/transport/chttp2/transport/http2_settings.cc \

config.w32

@@ -102,6 +102,7 @@ if (PHP_GRPC != "no") {
     "src\\core\\ext\\transport\\chttp2\\transport\\frame_window_update.cc " +
     "src\\core\\ext\\transport\\chttp2\\transport\\hpack_encoder.cc " +
     "src\\core\\ext\\transport\\chttp2\\transport\\hpack_encoder_table.cc " +
+    "src\\core\\ext\\transport\\chttp2\\transport\\hpack_parse_result.cc " +
     "src\\core\\ext\\transport\\chttp2\\transport\\hpack_parser.cc " +
     "src\\core\\ext\\transport\\chttp2\\transport\\hpack_parser_table.cc " +
     "src\\core\\ext\\transport\\chttp2\\transport\\http2_settings.cc " +

gRPC-C++.podspec

@@ -368,6 +368,7 @@ Pod::Spec.new do |s|
                       'src/core/ext/transport/chttp2/transport/hpack_constants.h',
                       'src/core/ext/transport/chttp2/transport/hpack_encoder.h',
                       'src/core/ext/transport/chttp2/transport/hpack_encoder_table.h',
+                      'src/core/ext/transport/chttp2/transport/hpack_parse_result.h',
                       'src/core/ext/transport/chttp2/transport/hpack_parser.h',
                       'src/core/ext/transport/chttp2/transport/hpack_parser_table.h',
                       'src/core/ext/transport/chttp2/transport/http2_settings.h',
@@ -1406,6 +1407,7 @@ Pod::Spec.new do |s|
                               'src/core/ext/transport/chttp2/transport/hpack_constants.h',
                               'src/core/ext/transport/chttp2/transport/hpack_encoder.h',
                               'src/core/ext/transport/chttp2/transport/hpack_encoder_table.h',
+                              'src/core/ext/transport/chttp2/transport/hpack_parse_result.h',
                               'src/core/ext/transport/chttp2/transport/hpack_parser.h',
                               'src/core/ext/transport/chttp2/transport/hpack_parser_table.h',
                               'src/core/ext/transport/chttp2/transport/http2_settings.h',

gRPC-Core.podspec

@@ -394,6 +394,8 @@ Pod::Spec.new do |s|
                       'src/core/ext/transport/chttp2/transport/hpack_encoder.h',
                       'src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc',
                       'src/core/ext/transport/chttp2/transport/hpack_encoder_table.h',
+                      'src/core/ext/transport/chttp2/transport/hpack_parse_result.cc',
+                      'src/core/ext/transport/chttp2/transport/hpack_parse_result.h',
                       'src/core/ext/transport/chttp2/transport/hpack_parser.cc',
                       'src/core/ext/transport/chttp2/transport/hpack_parser.h',
                       'src/core/ext/transport/chttp2/transport/hpack_parser_table.cc',
@@ -2133,6 +2135,7 @@ Pod::Spec.new do |s|
                               'src/core/ext/transport/chttp2/transport/hpack_constants.h',
                               'src/core/ext/transport/chttp2/transport/hpack_encoder.h',
                               'src/core/ext/transport/chttp2/transport/hpack_encoder_table.h',
+                              'src/core/ext/transport/chttp2/transport/hpack_parse_result.h',
                               'src/core/ext/transport/chttp2/transport/hpack_parser.h',
                               'src/core/ext/transport/chttp2/transport/hpack_parser_table.h',
                               'src/core/ext/transport/chttp2/transport/http2_settings.h',

grpc.gemspec

@@ -300,6 +300,8 @@ Gem::Specification.new do |s|
   s.files += %w( src/core/ext/transport/chttp2/transport/hpack_encoder.h )
   s.files += %w( src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc )
   s.files += %w( src/core/ext/transport/chttp2/transport/hpack_encoder_table.h )
+  s.files += %w( src/core/ext/transport/chttp2/transport/hpack_parse_result.cc )
+  s.files += %w( src/core/ext/transport/chttp2/transport/hpack_parse_result.h )
   s.files += %w( src/core/ext/transport/chttp2/transport/hpack_parser.cc )
   s.files += %w( src/core/ext/transport/chttp2/transport/hpack_parser.h )
   s.files += %w( src/core/ext/transport/chttp2/transport/hpack_parser_table.cc )

grpc.gyp

@@ -361,6 +361,7 @@
         'src/core/ext/transport/chttp2/transport/frame_window_update.cc',
         'src/core/ext/transport/chttp2/transport/hpack_encoder.cc',
         'src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc',
+        'src/core/ext/transport/chttp2/transport/hpack_parse_result.cc',
         'src/core/ext/transport/chttp2/transport/hpack_parser.cc',
         'src/core/ext/transport/chttp2/transport/hpack_parser_table.cc',
         'src/core/ext/transport/chttp2/transport/http2_settings.cc',
@@ -1173,6 +1174,7 @@
         'src/core/ext/transport/chttp2/transport/frame_window_update.cc',
         'src/core/ext/transport/chttp2/transport/hpack_encoder.cc',
         'src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc',
+        'src/core/ext/transport/chttp2/transport/hpack_parse_result.cc',
         'src/core/ext/transport/chttp2/transport/hpack_parser.cc',
         'src/core/ext/transport/chttp2/transport/hpack_parser_table.cc',
         'src/core/ext/transport/chttp2/transport/http2_settings.cc',

package.xml

@@ -282,6 +282,8 @@
     <file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/hpack_encoder.h" role="src" />
     <file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc" role="src" />
     <file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/hpack_encoder_table.h" role="src" />
+    <file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/hpack_parse_result.cc" role="src" />
+    <file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/hpack_parse_result.h" role="src" />
     <file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/hpack_parser.cc" role="src" />
     <file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/hpack_parser.h" role="src" />
     <file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/hpack_parser_table.cc" role="src" />

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

@@ -0,0 +1,176 @@
+// Copyright 2023 gRPC authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <grpc/support/port_platform.h>
+
+#include "src/core/ext/transport/chttp2/transport/hpack_parse_result.h"
+
+#include <stddef.h>
+
+#include <initializer_list>
+
+#include "absl/strings/str_format.h"
+
+#include "src/core/ext/transport/chttp2/transport/hpack_constants.h"
+#include "src/core/lib/gprpp/status_helper.h"
+#include "src/core/lib/iomgr/error.h"
+#include "src/core/lib/slice/slice.h"
+
+namespace grpc_core {
+
+namespace {
+class MetadataSizeLimitExceededEncoder {
+ public:
+  explicit MetadataSizeLimitExceededEncoder(std::string& summary)
+      : summary_(summary) {}
+
+  void Encode(const Slice& key, const Slice& value) {
+    AddToSummary(key.as_string_view(), value.size());
+  }
+
+  template <typename Key, typename Value>
+  void Encode(Key, const Value& value) {
+    AddToSummary(Key::key(), EncodedSizeOfKey(Key(), value));
+  }
+
+ private:
+  void AddToSummary(absl::string_view key,
+                    size_t value_length) GPR_ATTRIBUTE_NOINLINE {
+    absl::StrAppend(&summary_, " ", key, ":",
+                    hpack_constants::SizeForEntry(key.size(), value_length),
+                    "B");
+  }
+  std::string& summary_;
+};
+
+absl::Status MakeStreamError(absl::Status error) {
+  GPR_DEBUG_ASSERT(!error.ok());
+  return grpc_error_set_int(std::move(error), StatusIntProperty::kStreamId, 0);
+}
+}  // namespace
+
+absl::Status HpackParseResult::Materialize() const {
+  if (materialized_status_.has_value()) return *materialized_status_;
+  materialized_status_ = BuildMaterialized();
+  return *materialized_status_;
+}
+
+absl::Status HpackParseResult::BuildMaterialized() const {
+  switch (status_.get()) {
+    case HpackParseStatus::kOk:
+      return absl::OkStatus();
+    case HpackParseStatus::kEof:
+      Crash("Materialize() called on EOF");
+      break;
+    case HpackParseStatus::kMovedFrom:
+      Crash("Materialize() called on moved-from object");
+      break;
+    case HpackParseStatus::kInvalidMetadata:
+      if (key_.empty()) {
+        return MakeStreamError(absl::InternalError(
+            ValidateMetadataResultToString(validate_metadata_result_)));
+      } else {
+        return MakeStreamError(absl::InternalError(absl::StrCat(
+            ValidateMetadataResultToString(validate_metadata_result_), ": ",
+            key_)));
+      }
+    case HpackParseStatus::kSoftMetadataLimitExceeded:
+    case HpackParseStatus::kHardMetadataLimitExceeded: {
+      const auto& e = metadata_limit_exceeded_;
+      // Collect a summary of sizes so far for debugging
+      // Do not collect contents, for fear of exposing PII.
+      std::string summary;
+      if (e.prior != nullptr) {
+        MetadataSizeLimitExceededEncoder encoder(summary);
+        e.prior->Encode(&encoder);
+      }
+      return MakeStreamError(absl::ResourceExhaustedError(absl::StrCat(
+          "received metadata size exceeds ",
+          status_.get() == HpackParseStatus::kSoftMetadataLimitExceeded
+              ? "soft"
+              : "hard",
+          " limit (", e.frame_length, " vs. ", e.limit, ")",
+          summary.empty() ? "" : "; ", summary)));
+    }
+    case HpackParseStatus::kHardMetadataLimitExceededByKey: {
+      const auto& e = metadata_limit_exceeded_by_atom_;
+      return MakeStreamError(absl::ResourceExhaustedError(
+          absl::StrCat("received metadata size exceeds hard limit (key length ",
+                       e.atom_length, " vs. ", e.limit, ")")));
+    }
+    case HpackParseStatus::kHardMetadataLimitExceededByValue: {
+      const auto& e = metadata_limit_exceeded_by_atom_;
+      return MakeStreamError(absl::ResourceExhaustedError(absl::StrCat(
+          "received metadata size exceeds hard limit (value length ",
+          e.atom_length, " vs. ", e.limit, ")")));
+    }
+    case HpackParseStatus::kMetadataParseError:
+      if (!key_.empty()) {
+        return MakeStreamError(absl::InternalError(
+            absl::StrCat("Error parsing '", key_, "' metadata")));
+      } else {
+        return MakeStreamError(absl::InternalError("Error parsing metadata"));
+      }
+    case HpackParseStatus::kUnbase64Failed:
+      if (!key_.empty()) {
+        return MakeStreamError(absl::InternalError(absl::StrCat(
+            "Error parsing '", key_, "' metadata: illegal base64 encoding")));
+      } else {
+        return MakeStreamError(absl::InternalError(
+            absl::StrCat("Failed base64 decoding metadata")));
+      }
+    case HpackParseStatus::kIncompleteHeaderAtBoundary:
+      return absl::InternalError(
+          "Incomplete header at the end of a header/continuation sequence");
+    case HpackParseStatus::kVarintOutOfRange:
+      return absl::InternalError(absl::StrFormat(
+          "integer overflow in hpack integer decoding: have 0x%08x, "
+          "got byte 0x%02x",
+          varint_out_of_range_.value, varint_out_of_range_.last_byte));
+    case HpackParseStatus::kIllegalTableSizeChange:
+      return absl::InternalError(absl::StrCat(
+          "Attempt to make hpack table ", illegal_table_size_change_.new_size,
+          " bytes when max is ", illegal_table_size_change_.max_size,
+          " bytes"));
+    case HpackParseStatus::kAddBeforeTableSizeUpdated:
+      return absl::InternalError(
+          absl::StrCat("HPACK max table size reduced to ",
+                       illegal_table_size_change_.new_size,
+                       " but not reflected by hpack stream (still at ",
+                       illegal_table_size_change_.max_size, ")"));
+    case HpackParseStatus::kParseHuffFailed:
+      if (!key_.empty()) {
+        return absl::InternalError(
+            absl::StrCat("Failed huffman decoding '", key_, "' metadata"));
+      } else {
+        return absl::InternalError(
+            absl::StrCat("Failed huffman decoding metadata"));
+      }
+      break;
+    case HpackParseStatus::kTooManyDynamicTableSizeChanges:
+      return absl::InternalError(
+          "More than two max table size changes in a single frame");
+    case HpackParseStatus::kMaliciousVarintEncoding:
+      return absl::InternalError(
+          "Malicious varint encoding detected in HPACK stream");
+    case HpackParseStatus::kInvalidHpackIndex:
+      return absl::InternalError(absl::StrFormat(
+          "Invalid HPACK index received (%d)", invalid_hpack_index_));
+    case HpackParseStatus::kIllegalHpackOpCode:
+      return absl::InternalError("Illegal hpack op code");
+  }
+  GPR_UNREACHABLE_CODE(return absl::UnknownError("Should never reach here"));
+}
+
+}  // namespace grpc_core

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

@@ -0,0 +1,325 @@
+// Copyright 2023 gRPC authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef GRPC_SRC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_HPACK_PARSE_RESULT_H
+#define GRPC_SRC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_HPACK_PARSE_RESULT_H
+
+#include <grpc/support/port_platform.h>
+
+#include <stdint.h>
+
+#include <string>
+#include <utility>
+
+#include "absl/status/status.h"
+#include "absl/strings/str_cat.h"
+#include "absl/strings/string_view.h"
+#include "absl/types/optional.h"
+
+#include <grpc/support/log.h>
+
+#include "src/core/lib/gprpp/crash.h"
+#include "src/core/lib/surface/validate_metadata.h"
+#include "src/core/lib/transport/metadata_batch.h"
+
+namespace grpc_core {
+
+// Result of parsing
+// Makes it trivial to identify stream vs connection errors (via a range check)
+enum class HpackParseStatus : uint8_t {
+  ///////////////////////////////////////////////////////
+  // Non-Errors
+
+  // Parsed OK
+  kOk,
+  // Parse reached end of the current frame
+  kEof,
+  // Moved from - used to denote a HpackParseResult that has been moved into a
+  // different object, and so the original should be deemed invalid.
+  kMovedFrom,
+
+  ///////////////////////////////////////////////////////////////////
+  // Stream Errors - result in a stream cancellation
+
+  // Sentinel value used to denote the first error that is a stream error.
+  // All stream errors are hence >= kFirstStreamError and <
+  // kFirstConnectionError.
+  // Should not be used in switch statements, instead the first error message
+  // after this one should be assigned the same value.
+  kFirstStreamError,
+  kInvalidMetadata = kFirstStreamError,
+  // Hard metadata limit exceeded by the total set of metadata
+  kHardMetadataLimitExceeded,
+  kSoftMetadataLimitExceeded,
+  // Hard metadata limit exceeded by a single key string
+  kHardMetadataLimitExceededByKey,
+  // Hard metadata limit exceeded by a single value string
+  kHardMetadataLimitExceededByValue,
+  kMetadataParseError,
+  // Parse failed due to a base64 decode error
+  kUnbase64Failed,
+
+  ///////////////////////////////////////////////////////////////////
+  // Connection Errors - result in the tcp connection closing
+
+  // Sentinel value used to denote the first error that is a connection error.
+  // All connection errors are hence >= kFirstConnectionError.
+  // Should not be used in switch statements, instead the first error message
+  // after this one should be assigned the same value.
+  kFirstConnectionError,
+  // Incomplete header at end of header boundary
+  kIncompleteHeaderAtBoundary = kFirstConnectionError,
+  // Varint out of range
+  kVarintOutOfRange,
+  // Invalid HPACK index
+  kInvalidHpackIndex,
+  // Illegal HPACK table size change
+  kIllegalTableSizeChange,
+  // Trying to add to the hpack table prior to reducing after a settings change
+  kAddBeforeTableSizeUpdated,
+  // Parse failed due to a huffman decode error
+  kParseHuffFailed,
+  // Too many dynamic table size changes in one frame
+  kTooManyDynamicTableSizeChanges,
+  // Maliciously long varint encoding
+  // We don't read past 16 repeated 0x80 prefixes on a varint (all zeros)
+  // because no reasonable varint encoder would emit that (16 is already quite
+  // generous!)
+  // Because we stop reading we don't parse the rest of the bytes and so we
+  // can't recover parsing and would end up with a hpack table desync if we
+  // tried, so this is a connection error.
+  kMaliciousVarintEncoding,
+  // Illegal hpack op code
+  kIllegalHpackOpCode,
+};
+
+inline bool IsStreamError(HpackParseStatus status) {
+  return status >= HpackParseStatus::kFirstStreamError &&
+         status < HpackParseStatus::kFirstConnectionError;
+}
+
+inline bool IsConnectionError(HpackParseStatus status) {
+  return status >= HpackParseStatus::kFirstConnectionError;
+}
+
+inline bool IsEphemeralError(HpackParseStatus status) {
+  switch (status) {
+    case HpackParseStatus::kSoftMetadataLimitExceeded:
+    case HpackParseStatus::kHardMetadataLimitExceeded:
+      return true;
+    default:
+      return false;
+  }
+}
+
+class HpackParseResult {
+ public:
+  HpackParseResult() : HpackParseResult{HpackParseStatus::kOk} {}
+
+  bool ok() const { return status_.get() == HpackParseStatus::kOk; }
+  bool stream_error() const { return IsStreamError(status_.get()); }
+  bool connection_error() const { return IsConnectionError(status_.get()); }
+  bool ephemeral() const { return IsEphemeralError(status_.get()); }
+
+  HpackParseResult PersistentStreamErrorOrOk() const {
+    if (connection_error() || ephemeral()) return HpackParseResult();
+    return *this;
+  }
+
+  static HpackParseResult FromStatus(HpackParseStatus status) {
+    // Most statuses need some payloads, and we only need this functionality
+    // rarely - so allow list the statuses that we can include here.
+    switch (status) {
+      case HpackParseStatus::kUnbase64Failed:
+      case HpackParseStatus::kParseHuffFailed:
+        return HpackParseResult{status};
+      default:
+        Crash(
+            absl::StrCat("Invalid HpackParseStatus for FromStatus: ", status));
+    }
+  }
+
+  static HpackParseResult FromStatusWithKey(HpackParseStatus status,
+                                            absl::string_view key) {
+    auto r = FromStatus(status);
+    r.key_ = std::string(key);
+    return r;
+  }
+
+  static HpackParseResult MetadataParseError(absl::string_view key) {
+    HpackParseResult r{HpackParseStatus::kMetadataParseError};
+    r.key_ = std::string(key);
+    return r;
+  }
+
+  static HpackParseResult AddBeforeTableSizeUpdated(uint32_t current_size,
+                                                    uint32_t max_size) {
+    HpackParseResult p{HpackParseStatus::kAddBeforeTableSizeUpdated};
+    p.illegal_table_size_change_ =
+        IllegalTableSizeChange{current_size, max_size};
+    return p;
+  }
+
+  static HpackParseResult MaliciousVarintEncodingError() {
+    return HpackParseResult{HpackParseStatus::kMaliciousVarintEncoding};
+  }
+
+  static HpackParseResult IllegalHpackOpCode() {
+    return HpackParseResult{HpackParseStatus::kIllegalHpackOpCode};
+  }
+
+  static HpackParseResult InvalidMetadataError(ValidateMetadataResult result,
+                                               absl::string_view key) {
+    GPR_DEBUG_ASSERT(result != ValidateMetadataResult::kOk);
+    HpackParseResult p{HpackParseStatus::kInvalidMetadata};
+    p.key_ = std::string(key);
+    p.validate_metadata_result_ = result;
+    return p;
+  }
+
+  static HpackParseResult IncompleteHeaderAtBoundaryError() {
+    return HpackParseResult{HpackParseStatus::kIncompleteHeaderAtBoundary};
+  }
+
+  static HpackParseResult VarintOutOfRangeError(uint32_t value,
+                                                uint8_t last_byte) {
+    HpackParseResult p{HpackParseStatus::kVarintOutOfRange};
+    p.varint_out_of_range_ = VarintOutOfRange{last_byte, value};
+    return p;
+  }
+
+  static HpackParseResult InvalidHpackIndexError(uint32_t index) {
+    HpackParseResult p{HpackParseStatus::kInvalidHpackIndex};
+    p.invalid_hpack_index_ = index;
+    return p;
+  }
+
+  static HpackParseResult IllegalTableSizeChangeError(uint32_t new_size,
+                                                      uint32_t max_size) {
+    HpackParseResult p{HpackParseStatus::kIllegalTableSizeChange};
+    p.illegal_table_size_change_ = IllegalTableSizeChange{new_size, max_size};
+    return p;
+  }
+
+  static HpackParseResult TooManyDynamicTableSizeChangesError() {
+    return HpackParseResult{HpackParseStatus::kTooManyDynamicTableSizeChanges};
+  }
+
+  static HpackParseResult SoftMetadataLimitExceededError(
+      grpc_metadata_batch* metadata, uint32_t frame_length, uint32_t limit) {
+    HpackParseResult p{HpackParseStatus::kSoftMetadataLimitExceeded};
+    p.metadata_limit_exceeded_ =
+        MetadataLimitExceeded{frame_length, limit, metadata};
+    return p;
+  }
+
+  static HpackParseResult HardMetadataLimitExceededError(
+      grpc_metadata_batch* metadata, uint32_t frame_length, uint32_t limit) {
+    HpackParseResult p{HpackParseStatus::kHardMetadataLimitExceeded};
+    p.metadata_limit_exceeded_ =
+        MetadataLimitExceeded{frame_length, limit, metadata};
+    return p;
+  }
+
+  static HpackParseResult HardMetadataLimitExceededByKeyError(
+      uint32_t key_length, uint32_t limit) {
+    HpackParseResult p{HpackParseStatus::kHardMetadataLimitExceededByKey};
+    p.metadata_limit_exceeded_by_atom_ =
+        MetadataLimitExceededByAtom{key_length, limit};
+    return p;
+  }
+
+  static HpackParseResult HardMetadataLimitExceededByValueError(
+      absl::string_view key, uint32_t value_length, uint32_t limit) {
+    HpackParseResult p{HpackParseStatus::kHardMetadataLimitExceededByValue};
+    p.metadata_limit_exceeded_by_atom_ =
+        MetadataLimitExceededByAtom{value_length, limit};
+    p.key_ = std::string(key);
+    return p;
+  }
+
+  // Compute the absl::Status that goes along with this HpackParseResult.
+  // (may be cached, so this is not thread safe)
+  absl::Status Materialize() const;
+
+ private:
+  explicit HpackParseResult(HpackParseStatus status) : status_(status) {}
+  absl::Status BuildMaterialized() const;
+
+  struct VarintOutOfRange {
+    uint8_t last_byte;
+    uint32_t value;
+  };
+
+  struct MetadataLimitExceeded {
+    uint32_t frame_length;
+    uint32_t limit;
+    grpc_metadata_batch* prior;
+  };
+
+  // atom here means one of either a key or a value - so this is used for when a
+  // metadata limit is consumed by either of these.
+  struct MetadataLimitExceededByAtom {
+    uint32_t atom_length;
+    uint32_t limit;
+  };
+
+  struct IllegalTableSizeChange {
+    uint32_t new_size;
+    uint32_t max_size;
+  };
+
+  class StatusWrapper {
+   public:
+    explicit StatusWrapper(HpackParseStatus status) : status_(status) {}
+
+    StatusWrapper(const StatusWrapper&) = default;
+    StatusWrapper& operator=(const StatusWrapper&) = default;
+    StatusWrapper(StatusWrapper&& other) noexcept
+        : status_(std::exchange(other.status_, HpackParseStatus::kMovedFrom)) {}
+    StatusWrapper& operator=(StatusWrapper&& other) noexcept {
+      status_ = std::exchange(other.status_, HpackParseStatus::kMovedFrom);
+      return *this;
+    }
+
+    HpackParseStatus get() const { return status_; }
+
+   private:
+    HpackParseStatus status_;
+  };
+
+  StatusWrapper status_;
+  union {
+    // Set if status == kInvalidMetadata
+    ValidateMetadataResult validate_metadata_result_;
+    // Set if status == kVarintOutOfRange
+    VarintOutOfRange varint_out_of_range_;
+    // Set if status == kInvalidHpackIndex
+    uint32_t invalid_hpack_index_;
+    // Set if status == kHardMetadataLimitExceeded or
+    // kSoftMetadataLimitExceeded
+    MetadataLimitExceeded metadata_limit_exceeded_;
+    // Set if status == kHardMetadataLimitExceededByKey or
+    // kHardMetadataLimitExceededByValue
+    MetadataLimitExceededByAtom metadata_limit_exceeded_by_atom_;
+    // Set if status == kIllegalTableSizeChange
+    IllegalTableSizeChange illegal_table_size_change_;
+  };
+  std::string key_;
+  mutable absl::optional<absl::Status> materialized_status_;
+};
+
+}  // namespace grpc_core
+
+#endif  // GRPC_SRC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_HPACK_PARSE_RESULT_H

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

@@ -21,16 +21,28 @@
 
 #include <grpc/support/port_platform.h>
 
+#include <stddef.h>
 #include <stdint.h>
 
+#include <string>
+#include <utility>
 #include <vector>
 
+#include "absl/strings/str_cat.h"
+#include "absl/strings/string_view.h"
+#include "absl/types/optional.h"
+#include "absl/types/span.h"
+#include "absl/types/variant.h"
+
 #include <grpc/slice.h>
 
 #include "src/core/ext/transport/chttp2/transport/frame.h"
+#include "src/core/ext/transport/chttp2/transport/hpack_parse_result.h"
 #include "src/core/ext/transport/chttp2/transport/hpack_parser_table.h"
 #include "src/core/lib/backoff/random_early_detection.h"
 #include "src/core/lib/iomgr/error.h"
+#include "src/core/lib/slice/slice.h"
+#include "src/core/lib/slice/slice_refcount.h"
 #include "src/core/lib/transport/metadata_batch.h"
 
 // IWYU pragma: no_include <type_traits>
@@ -92,17 +104,150 @@ class HPackParser {
   void FinishFrame();
 
   // Retrieve the associated hpack table (for tests, debugging)
-  HPackTable* hpack_table() { return &table_; }
+  HPackTable* hpack_table() { return &state_.hpack_table; }
   // Is the current frame a boundary of some sort
   bool is_boundary() const { return boundary_ != Boundary::None; }
   // Is the current frame the end of a stream
   bool is_eof() const { return boundary_ == Boundary::EndOfStream; }
 
+  // How many bytes are buffered (for tests to assert on)
+  size_t buffered_bytes() const { return unparsed_bytes_.size(); }
+
  private:
   // Helper classes: see implementation
   class Parser;
   class Input;
-  class String;
+
+  // Helper to parse a string and turn it into a slice with appropriate memory
+  // management characteristics
+  class String {
+   public:
+    // StringResult carries both a HpackParseStatus and the parsed string
+    struct StringResult;
+
+    String() : value_(absl::Span<const uint8_t>()) {}
+    String(const String&) = delete;
+    String& operator=(const String&) = delete;
+    String(String&& other) noexcept : value_(std::move(other.value_)) {
+      other.value_ = absl::Span<const uint8_t>();
+    }
+    String& operator=(String&& other) noexcept {
+      value_ = std::move(other.value_);
+      other.value_ = absl::Span<const uint8_t>();
+      return *this;
+    }
+
+    // Take the value and leave this empty
+    Slice Take();
+
+    // Return a reference to the value as a string view
+    absl::string_view string_view() const;
+
+    // Parse a non-binary string
+    static StringResult Parse(Input* input, bool is_huff, size_t length);
+
+    // Parse a binary string
+    static StringResult ParseBinary(Input* input, bool is_huff, size_t length);
+
+   private:
+    void AppendBytes(const uint8_t* data, size_t length);
+    explicit String(std::vector<uint8_t> v) : value_(std::move(v)) {}
+    explicit String(absl::Span<const uint8_t> v) : value_(v) {}
+    String(grpc_slice_refcount* r, const uint8_t* begin, const uint8_t* end)
+        : value_(Slice::FromRefcountAndBytes(r, begin, end)) {}
+
+    // Parse some huffman encoded bytes, using output(uint8_t b) to emit each
+    // decoded byte.
+    template <typename Out>
+    static HpackParseStatus ParseHuff(Input* input, uint32_t length,
+                                      Out output);
+
+    // Parse some uncompressed string bytes.
+    static StringResult ParseUncompressed(Input* input, uint32_t length,
+                                          uint32_t wire_size);
+
+    // Turn base64 encoded bytes into not base64 encoded bytes.
+    static StringResult Unbase64(String s);
+
+    // Main loop for Unbase64
+    static absl::optional<std::vector<uint8_t>> Unbase64Loop(
+        const uint8_t* cur, const uint8_t* end);
+
+    absl::variant<Slice, absl::Span<const uint8_t>, std::vector<uint8_t>>
+        value_;
+  };
+
+  // Prefix for a string
+  struct StringPrefix {
+    // Number of bytes in input for string
+    uint32_t length;
+    // Is it huffman compressed
+    bool huff;
+
+    std::string ToString() const {
+      return absl::StrCat(length, " bytes ",
+                          huff ? "huffman compressed" : "uncompressed");
+    }
+  };
+
+  // Current parse state
+  // ┌───┐
+  // │Top│
+  // └┬─┬┘
+  //  │┌▽────────────────┐
+  //  ││ParsingKeyLength │
+  //  │└┬───────────────┬┘
+  //  │┌▽─────────────┐┌▽──────────────┐
+  //  ││ParsingKeyBody││SkippingKeyBody│
+  //  │└┬─────────────┘└───┬───────────┘
+  // ┌▽─▽────────────────┐┌▽──────────────────┐
+  // │ParsingValueLength ││SkippingValueLength│
+  // └┬─────────────────┬┘└┬──────────────────┘
+  // ┌▽───────────────┐┌▽──▽─────────────┐
+  // │ParsingValueBody││SkippingValueBody│
+  // └────────────────┘└─────────────────┘
+  enum class ParseState : uint8_t {
+    // Start of one opcode
+    kTop,
+    // Parsing a literal keys length
+    kParsingKeyLength,
+    // Parsing a literal key
+    kParsingKeyBody,
+    // Skipping a literal key
+    kSkippingKeyBody,
+    // Parsing a literal value length
+    kParsingValueLength,
+    // Parsing a literal value
+    kParsingValueBody,
+    // Reading a literal value length (so we can skip it)
+    kSkippingValueLength,
+    // Skipping a literal value
+    kSkippingValueBody,
+  };
+
+  // Shared state for Parser instances between slices.
+  struct InterSliceState {
+    HPackTable hpack_table;
+    // Error so far for this frame (set by class Input)
+    HpackParseResult frame_error;
+    // Length of frame so far.
+    uint32_t frame_length = 0;
+    // Length of the string being parsed
+    uint32_t string_length;
+    // How many more dynamic table updates are allowed
+    uint8_t dynamic_table_updates_allowed;
+    // Current parse state
+    ParseState parse_state = ParseState::kTop;
+    // RED for overly large metadata sets
+    RandomEarlyDetection metadata_early_detection;
+    // Should the current header be added to the hpack table?
+    bool add_to_table;
+    // Is the string being parsed huffman compressed?
+    bool is_string_huff_compressed;
+    // Is the value being parsed binary?
+    bool is_binary_header;
+    absl::variant<const HPackTable::Memento*, Slice> key;
+  };
 
   grpc_error_handle ParseInput(Input input, bool is_last);
   void ParseInputInner(Input* input);
@@ -114,6 +259,8 @@ class HPackParser {
 
   // Bytes that could not be parsed last parsing round
   std::vector<uint8_t> unparsed_bytes_;
+  // How many bytes would be needed before progress could be made?
+  size_t min_progress_size_ = 0;
   // Buffer kind of boundary
   // TODO(ctiller): see if we can move this argument to Parse, and avoid
   // buffering.
@@ -122,15 +269,9 @@ class HPackParser {
   // TODO(ctiller): see if we can move this argument to Parse, and avoid
   // buffering.
   Priority priority_;
-  uint8_t dynamic_table_updates_allowed_;
-  // Length of frame so far.
-  uint32_t frame_length_;
-  RandomEarlyDetection metadata_early_detection_;
   // Information for logging
   LogInfo log_info_;
-
-  // hpack table
-  HPackTable table_;
+  InterSliceState state_;
 };
 
 }  // namespace grpc_core

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

@@ -25,16 +25,16 @@
 #include <algorithm>
 #include <cstddef>
 #include <cstring>
-#include <initializer_list>
 #include <utility>
 
 #include "absl/status/status.h"
-#include "absl/strings/str_format.h"
+#include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 
 #include <grpc/support/log.h>
 
 #include "src/core/ext/transport/chttp2/transport/hpack_constants.h"
+#include "src/core/ext/transport/chttp2/transport/hpack_parse_result.h"
 #include "src/core/ext/transport/chttp2/transport/http_trace.h"
 #include "src/core/lib/debug/trace.h"
 #include "src/core/lib/slice/slice.h"
@@ -80,6 +80,14 @@ void HPackTable::MementoRingBuffer::Rebuild(uint32_t max_entries) {
   entries_.swap(entries);
 }
 
+void HPackTable::MementoRingBuffer::ForEach(
+    absl::FunctionRef<void(uint32_t, const Memento&)> f) const {
+  uint32_t index = 0;
+  while (auto* m = Lookup(index++)) {
+    f(index, *m);
+  }
+}
+
 // Evict one element from the table
 void HPackTable::EvictOne() {
   auto first_entry = entries_.PopOne();
@@ -100,15 +108,9 @@ void HPackTable::SetMaxBytes(uint32_t max_bytes) {
   max_bytes_ = max_bytes;
 }
 
-grpc_error_handle HPackTable::SetCurrentTableSize(uint32_t bytes) {
-  if (current_table_bytes_ == bytes) {
-    return absl::OkStatus();
-  }
-  if (bytes > max_bytes_) {
-    return absl::InternalError(absl::StrFormat(
-        "Attempt to make hpack table %d bytes when max is %d bytes", bytes,
-        max_bytes_));
-  }
+bool HPackTable::SetCurrentTableSize(uint32_t bytes) {
+  if (current_table_bytes_ == bytes) return true;
+  if (bytes > max_bytes_) return false;
   if (GRPC_TRACE_FLAG_ENABLED(grpc_http_trace)) {
     gpr_log(GPR_INFO, "Update hpack parser table size to %d", bytes);
   }
@@ -119,30 +121,16 @@ grpc_error_handle HPackTable::SetCurrentTableSize(uint32_t bytes) {
   uint32_t new_cap = std::max(hpack_constants::EntriesForBytes(bytes),
                               hpack_constants::kInitialTableEntries);
   entries_.Rebuild(new_cap);
-  return absl::OkStatus();
+  return true;
 }
 
-grpc_error_handle HPackTable::Add(Memento md) {
-  if (current_table_bytes_ > max_bytes_) {
-    return GRPC_ERROR_CREATE(absl::StrFormat(
-        "HPACK max table size reduced to %d but not reflected by hpack "
-        "stream (still at %d)",
-        max_bytes_, current_table_bytes_));
-  }
+bool HPackTable::Add(Memento md) {
+  if (current_table_bytes_ > max_bytes_) return false;
 
   // we can't add elements bigger than the max table size
   if (md.md.transport_size() > current_table_bytes_) {
-    // HPACK draft 10 section 4.4 states:
-    // If the size of the new entry is less than or equal to the maximum
-    // size, that entry is added to the table.  It is not an error to
-    // attempt to add an entry that is larger than the maximum size; an
-    // attempt to add an entry larger than the entire table causes
-    // the table to be emptied of all existing entries, and results in an
-    // empty table.
-    while (entries_.num_entries()) {
-      EvictOne();
-    }
-    return absl::OkStatus();
+    AddLargerThanCurrentTableSize();
+    return true;
   }
 
   // evict entries to ensure no overflow
@@ -154,7 +142,33 @@ grpc_error_handle HPackTable::Add(Memento md) {
   // copy the finalized entry in
   mem_used_ += md.md.transport_size();
   entries_.Put(std::move(md));
-  return absl::OkStatus();
+  return true;
+}
+
+void HPackTable::AddLargerThanCurrentTableSize() {
+  // HPACK draft 10 section 4.4 states:
+  // If the size of the new entry is less than or equal to the maximum
+  // size, that entry is added to the table.  It is not an error to
+  // attempt to add an entry that is larger than the maximum size; an
+  // attempt to add an entry larger than the entire table causes
+  // the table to be emptied of all existing entries, and results in an
+  // empty table.
+  while (entries_.num_entries()) {
+    EvictOne();
+  }
+}
+
+std::string HPackTable::TestOnlyDynamicTableAsString() const {
+  std::string out;
+  entries_.ForEach([&out](uint32_t i, const Memento& m) {
+    if (m.parse_status.ok()) {
+      absl::StrAppend(&out, i, ": ", m.md.DebugString(), "\n");
+    } else {
+      absl::StrAppend(&out, i, ": ", m.parse_status.Materialize().ToString(),
+                      "\n");
+    }
+  });
+  return out;
 }
 
 namespace {
@@ -236,7 +250,7 @@ HPackTable::Memento MakeMemento(size_t i) {
           [](absl::string_view, const Slice&) {
             abort();  // not expecting to see this
           }),
-      absl::OkStatus()};
+      HpackParseResult()};
 }
 
 }  // namespace

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

@@ -23,13 +23,14 @@
 
 #include <stdint.h>
 
+#include <string>
 #include <vector>
 
-#include "absl/status/status.h"
+#include "absl/functional/function_ref.h"
 
 #include "src/core/ext/transport/chttp2/transport/hpack_constants.h"
+#include "src/core/ext/transport/chttp2/transport/hpack_parse_result.h"
 #include "src/core/lib/gprpp/no_destruct.h"
-#include "src/core/lib/iomgr/error.h"
 #include "src/core/lib/transport/metadata_batch.h"
 #include "src/core/lib/transport/parsed_metadata.h"
 
@@ -45,11 +46,12 @@ class HPackTable {
   HPackTable& operator=(const HPackTable&) = delete;
 
   void SetMaxBytes(uint32_t max_bytes);
-  grpc_error_handle SetCurrentTableSize(uint32_t bytes);
+  bool SetCurrentTableSize(uint32_t bytes);
+  uint32_t current_table_size() { return current_table_bytes_; }
 
   struct Memento {
     ParsedMetadata<grpc_metadata_batch> md;
-    absl::Status parse_status;
+    HpackParseResult parse_status;
   };
 
   // Lookup, but don't ref.
@@ -68,7 +70,8 @@ class HPackTable {
   }
 
   // add a table entry to the index
-  grpc_error_handle Add(Memento md) GRPC_MUST_USE_RESULT;
+  bool Add(Memento md) GRPC_MUST_USE_RESULT;
+  void AddLargerThanCurrentTableSize();
 
   // Current entry count in the table.
   uint32_t num_entries() const { return entries_.num_entries(); }
@@ -76,6 +79,13 @@ class HPackTable {
   // Current size of the table.
   uint32_t test_only_table_size() const { return mem_used_; }
 
+  // Maximum allowed size of the table currently
+  uint32_t max_bytes() const { return max_bytes_; }
+  uint32_t current_table_bytes() const { return current_table_bytes_; }
+
+  // Dynamic table entries, stringified
+  std::string TestOnlyDynamicTableAsString() const;
+
  private:
   struct StaticMementos {
     StaticMementos();
@@ -98,6 +108,9 @@ class HPackTable {
     // Lookup the entry at index, or return nullptr if none exists.
     const Memento* Lookup(uint32_t index) const;
 
+    void ForEach(absl::FunctionRef<void(uint32_t dynamic_index, const Memento&)>
+                     f) const;
+
     uint32_t max_entries() const { return max_entries_; }
     uint32_t num_entries() const { return num_entries_; }
 

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

@@ -23,7 +23,6 @@
 
 #include <initializer_list>
 #include <string>
-#include <utility>
 
 #include "absl/base/attributes.h"
 #include "absl/status/status.h"
@@ -470,8 +469,8 @@ static HPackParser::LogInfo hpack_parser_log_info(
 }
 
 static grpc_error_handle init_header_skip_frame_parser(
-    grpc_chttp2_transport* t, HPackParser::Priority priority_type) {
-  bool is_eoh = t->expect_continuation_stream_id != 0;
+    grpc_chttp2_transport* t, HPackParser::Priority priority_type,
+    bool is_eoh) {
   t->parser = grpc_chttp2_transport::Parser{
       "header", grpc_chttp2_header_parser_parse, &t->hpack_parser};
   t->hpack_parser.BeginFrame(
@@ -595,7 +594,7 @@ static grpc_error_handle init_header_frame_parser(grpc_chttp2_transport* t,
       GRPC_CHTTP2_IF_TRACING(
           gpr_log(GPR_ERROR,
                   "grpc_chttp2_stream disbanded before CONTINUATION received"));
-      return init_header_skip_frame_parser(t, priority_type);
+      return init_header_skip_frame_parser(t, priority_type, is_eoh);
     }
     if (t->is_client) {
       if (GPR_LIKELY((t->incoming_stream_id & 1) &&
@@ -605,7 +604,7 @@ static grpc_error_handle init_header_frame_parser(grpc_chttp2_transport* t,
         GRPC_CHTTP2_IF_TRACING(gpr_log(
             GPR_ERROR, "ignoring new grpc_chttp2_stream creation on client"));
       }
-      return init_header_skip_frame_parser(t, priority_type);
+      return init_header_skip_frame_parser(t, priority_type, is_eoh);
     } else if (GPR_UNLIKELY(t->last_new_stream_id >= t->incoming_stream_id)) {
       GRPC_CHTTP2_IF_TRACING(gpr_log(
           GPR_ERROR,
@@ -613,13 +612,13 @@ static grpc_error_handle init_header_frame_parser(grpc_chttp2_transport* t,
           "last grpc_chttp2_stream "
           "id=%d, new grpc_chttp2_stream id=%d",
           t->last_new_stream_id, t->incoming_stream_id));
-      return init_header_skip_frame_parser(t, priority_type);
+      return init_header_skip_frame_parser(t, priority_type, is_eoh);
     } else if (GPR_UNLIKELY((t->incoming_stream_id & 1) == 0)) {
       GRPC_CHTTP2_IF_TRACING(gpr_log(
           GPR_ERROR,
           "ignoring grpc_chttp2_stream with non-client generated index %d",
           t->incoming_stream_id));
-      return init_header_skip_frame_parser(t, priority_type);
+      return init_header_skip_frame_parser(t, priority_type, is_eoh);
     } else if (GPR_UNLIKELY(
                    grpc_chttp2_stream_map_size(&t->stream_map) >=
                    t->settings[GRPC_ACKED_SETTINGS]
@@ -632,7 +631,7 @@ static grpc_error_handle init_header_frame_parser(grpc_chttp2_transport* t,
           "grpc_chttp2_stream request id=%d, last grpc_chttp2_stream id=%d",
           t, std::string(t->peer_string.as_string_view()).c_str(),
           t->incoming_stream_id, t->last_new_stream_id));
-      return init_header_skip_frame_parser(t, priority_type);
+      return init_header_skip_frame_parser(t, priority_type, is_eoh);
     }
     t->last_new_stream_id = t->incoming_stream_id;
     s = t->incoming_stream =
@@ -640,7 +639,7 @@ static grpc_error_handle init_header_frame_parser(grpc_chttp2_transport* t,
     if (GPR_UNLIKELY(s == nullptr)) {
       GRPC_CHTTP2_IF_TRACING(
           gpr_log(GPR_ERROR, "grpc_chttp2_stream not accepted"));
-      return init_header_skip_frame_parser(t, priority_type);
+      return init_header_skip_frame_parser(t, priority_type, is_eoh);
     }
     if (t->channelz_socket != nullptr) {
       t->channelz_socket->RecordStreamStartedFromRemote();
@@ -654,7 +653,7 @@ static grpc_error_handle init_header_frame_parser(grpc_chttp2_transport* t,
     GRPC_CHTTP2_IF_TRACING(gpr_log(
         GPR_ERROR, "skipping already closed grpc_chttp2_stream header"));
     t->incoming_stream = nullptr;
-    return init_header_skip_frame_parser(t, priority_type);
+    return init_header_skip_frame_parser(t, priority_type, is_eoh);
   }
   t->parser = grpc_chttp2_transport::Parser{
       "header", grpc_chttp2_header_parser_parse, &t->hpack_parser};
@@ -687,7 +686,7 @@ static grpc_error_handle init_header_frame_parser(grpc_chttp2_transport* t,
       break;
     case 2:
       gpr_log(GPR_ERROR, "too many header frames received");
-      return init_header_skip_frame_parser(t, priority_type);
+      return init_header_skip_frame_parser(t, priority_type, is_eoh);
   }
   if (frame_type == HPackParser::LogInfo::kTrailers && !t->header_eof) {
     return GRPC_ERROR_CREATE(
@@ -815,8 +814,9 @@ static grpc_error_handle parse_frame_slice(grpc_chttp2_transport* t,
                          &unused)) {
     grpc_chttp2_parsing_become_skip_parser(t);
     if (s) {
-      grpc_chttp2_cancel_stream(t, s, std::exchange(err, absl::OkStatus()));
+      grpc_chttp2_cancel_stream(t, s, err);
     }
+    return absl::OkStatus();
   }
   return err;
 }

src/core/lib/backoff/random_early_detection.h

@@ -43,6 +43,11 @@ class RandomEarlyDetection {
   uint64_t soft_limit() const { return soft_limit_; }
   uint64_t hard_limit() const { return hard_limit_; }
 
+  void SetLimits(uint64_t soft_limit, uint64_t hard_limit) {
+    soft_limit_ = soft_limit;
+    hard_limit_ = hard_limit;
+  }
+
  private:
   // The soft limit is the size at which we start rejecting items with a
   // probability that increases linearly to 1 as the size approaches the hard

src/core/lib/surface/validate_metadata.cc

@@ -21,8 +21,6 @@
 #include "src/core/lib/surface/validate_metadata.h"
 
 #include "absl/status/status.h"
-#include "absl/strings/escaping.h"
-#include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 
 #include <grpc/grpc.h>
@@ -46,32 +44,49 @@ class LegalHeaderKeyBits : public BitSet<256> {
 };
 constexpr LegalHeaderKeyBits g_legal_header_key_bits;
 
-GPR_ATTRIBUTE_NOINLINE
-absl::Status DoesNotConformTo(absl::string_view x, const char* err_desc) {
-  return absl::InternalError(absl::StrCat(err_desc, ": ", x, " (hex ",
-                                          absl::BytesToHexString(x), ")"));
-}
-
-absl::Status ConformsTo(absl::string_view x, const BitSet<256>& legal_bits,
-                        const char* err_desc) {
+ValidateMetadataResult ConformsTo(absl::string_view x,
+                                  const BitSet<256>& legal_bits,
+                                  ValidateMetadataResult error) {
   for (uint8_t c : x) {
     if (!legal_bits.is_set(c)) {
-      return DoesNotConformTo(x, err_desc);
+      return error;
     }
   }
-  return absl::OkStatus();
+  return ValidateMetadataResult::kOk;
 }
+
+absl::Status UpgradeToStatus(ValidateMetadataResult result) {
+  if (result == ValidateMetadataResult::kOk) return absl::OkStatus();
+  return absl::InternalError(ValidateMetadataResultToString(result));
+}
+
 }  // namespace
 
-absl::Status ValidateHeaderKeyIsLegal(absl::string_view key) {
+ValidateMetadataResult ValidateHeaderKeyIsLegal(absl::string_view key) {
   if (key.empty()) {
-    return absl::InternalError("Metadata keys cannot be zero length");
+    return ValidateMetadataResult::kCannotBeZeroLength;
   }
   if (key.size() > UINT32_MAX) {
-    return absl::InternalError(
-        "Metadata keys cannot be larger than UINT32_MAX");
+    return ValidateMetadataResult::kTooLong;
   }
-  return ConformsTo(key, g_legal_header_key_bits, "Illegal header key");
+  return ConformsTo(key, g_legal_header_key_bits,
+                    ValidateMetadataResult::kIllegalHeaderKey);
+}
+
+const char* ValidateMetadataResultToString(ValidateMetadataResult result) {
+  switch (result) {
+    case ValidateMetadataResult::kOk:
+      return "Ok";
+    case ValidateMetadataResult::kCannotBeZeroLength:
+      return "Metadata keys cannot be zero length";
+    case ValidateMetadataResult::kTooLong:
+      return "Metadata keys cannot be larger than UINT32_MAX";
+    case ValidateMetadataResult::kIllegalHeaderKey:
+      return "Illegal header key";
+    case ValidateMetadataResult::kIllegalHeaderValue:
+      return "Illegal header value";
+  }
+  GPR_UNREACHABLE_CODE(return "Unknown");
 }
 
 }  // namespace grpc_core
@@ -82,8 +97,8 @@ static int error2int(grpc_error_handle error) {
 }
 
 grpc_error_handle grpc_validate_header_key_is_legal(const grpc_slice& slice) {
-  return grpc_core::ValidateHeaderKeyIsLegal(
-      grpc_core::StringViewFromSlice(slice));
+  return grpc_core::UpgradeToStatus(grpc_core::ValidateHeaderKeyIsLegal(
+      grpc_core::StringViewFromSlice(slice)));
 }
 
 int grpc_header_key_is_legal(grpc_slice slice) {
@@ -104,9 +119,9 @@ constexpr LegalHeaderNonBinValueBits g_legal_header_non_bin_value_bits;
 
 grpc_error_handle grpc_validate_header_nonbin_value_is_legal(
     const grpc_slice& slice) {
-  return grpc_core::ConformsTo(grpc_core::StringViewFromSlice(slice),
-                               g_legal_header_non_bin_value_bits,
-                               "Illegal header value");
+  return grpc_core::UpgradeToStatus(grpc_core::ConformsTo(
+      grpc_core::StringViewFromSlice(slice), g_legal_header_non_bin_value_bits,
+      grpc_core::ValidateMetadataResult::kIllegalHeaderValue));
 }
 
 int grpc_header_nonbin_value_is_legal(grpc_slice slice) {

src/core/lib/surface/validate_metadata.h

@@ -25,7 +25,6 @@
 
 #include <cstring>
 
-#include "absl/status/status.h"
 #include "absl/strings/string_view.h"
 
 #include <grpc/slice.h>
@@ -35,9 +34,20 @@
 
 namespace grpc_core {
 
-absl::Status ValidateHeaderKeyIsLegal(absl::string_view key);
+enum class ValidateMetadataResult : uint8_t {
+  kOk,
+  kCannotBeZeroLength,
+  kTooLong,
+  kIllegalHeaderKey,
+  kIllegalHeaderValue
+};
 
-}
+const char* ValidateMetadataResultToString(ValidateMetadataResult result);
+
+// Returns nullopt if the key is legal, otherwise returns an error message.
+ValidateMetadataResult ValidateHeaderKeyIsLegal(absl::string_view key);
+
+}  // namespace grpc_core
 
 grpc_error_handle grpc_validate_header_key_is_legal(const grpc_slice& slice);
 grpc_error_handle grpc_validate_header_nonbin_value_is_legal(

src/python/grpcio/grpc_core_dependencies.py

@@ -111,6 +111,7 @@ CORE_SOURCE_FILES = [
     'src/core/ext/transport/chttp2/transport/frame_window_update.cc',
     'src/core/ext/transport/chttp2/transport/hpack_encoder.cc',
     'src/core/ext/transport/chttp2/transport/hpack_encoder_table.cc',
+    'src/core/ext/transport/chttp2/transport/hpack_parse_result.cc',
     'src/core/ext/transport/chttp2/transport/hpack_parser.cc',
     'src/core/ext/transport/chttp2/transport/hpack_parser_table.cc',
     'src/core/ext/transport/chttp2/transport/http2_settings.cc',

test/core/transport/chttp2/BUILD

@@ -26,6 +26,7 @@ grpc_proto_fuzzer(
     corpus = "hpack_parser_corpus",
     proto = "hpack_parser_fuzzer.proto",
     tags = ["no_windows"],
+    uses_polling = False,
     deps = [
         "//:grpc",
         "//test/core/util:grpc_test_util",
@@ -38,6 +39,7 @@ grpc_proto_fuzzer(
     corpus = "hpack_sync_corpus",
     proto = "hpack_sync_fuzzer.proto",
     tags = ["no_windows"],
+    uses_polling = False,
     deps = [
         "//:grpc",
         "//test/core/util:grpc_test_util",
@@ -71,6 +73,7 @@ grpc_fuzzer(
         "absl/status",
     ],
     tags = ["no_windows"],
+    uses_polling = False,
     deps = [
         "//:grpc",
         "//test/core/util:grpc_test_util",

test/core/transport/chttp2/hpack_parser_corpus/crash-06c4093ab80184d59150773f5c19e8e3851aa584

@@ -0,0 +1,5 @@
+frames {
+  max_metadata_length: 4096
+  parse: "\244\244\020\007\360\244\017-bin\213#Z)\244(-\244\016b\244\t\020\007\360\244\017-bin?\360\213c[)(-\177ni\'\'bin!!\t!/\244!?\037\'\360!"
+  absolute_max_metadata_length: 1073741824
+}

test/core/transport/chttp2/hpack_parser_corpus/crash-073bba832e73c8f524714e179236756e4854e08f

@@ -0,0 +1,5 @@
+frames {
+  max_metadata_length: 4096
+  parse: "\244\037!\203\360\361\244\037!\203\333\360\261\360c"
+  absolute_max_metadata_length: 1073741824
+}

test/core/transport/chttp2/hpack_parser_corpus/crash-d47bb8c8c9640faf351068480844e1be20cfdbb7

@@ -0,0 +1,16 @@
+frames {
+  parse: "*\244\020\007\360\244\017-bin\203c\035\037\000\'[\360i(bn-!?\244\037\333\360!(!\\\360\tc"
+  parse: "\244\244\020\007\360\244\017-bin\213c[)(-\'bin\t;!!?\244\037\333\360!\020\007\360{(-bin\360\t!\\\t!\345\037\351\033;?G\355[((!!\\\360"
+}
+frames {
+  max_metadata_length: 12
+  parse: "*\244\020\007\360\244\017-bin\203c\035\037\000\'[\360i(bn-!?\244\037\333\360!(!\\\360\tc"
+  parse: "\244\244\020\007\360\244\017-bin\213c[)(-\'bin\t;!!?\244\037\333\360!\020\007\360{(-bin\360\t!\\\t!\345\037\351\033;?G\355[((!!\\\360"
+  parse: "\244\244\020\007\360\244\017-bin\213c[)(-\'bin\t;!!?\244\037\333\360!\020\007\360{(-bin\360\t!\\\t!\345\037\351\033;?G\355[((!!\\\360"
+  parse: "\244\244\020\007\360\244\017-bin\213c[)(-\'bin\t;!!?\244\037\333\360!\020\007\360{(-bin\360\t!\\\t!\345\037\351\033;?G\355[((!!\\\360"
+  absolute_max_metadata_length: 12
+}
+frames {
+  parse: "*\244\020\007\360\244\017-bin\203c\035\037\000\'[\360i(bn-!?\244\037\333\360!(!\\\360\tc"
+  parse: "\244\244\020\007\360\244\017-bin\213c[)(-\'bin\t;!!?\244\037\333\360!\020\007\360{(-bin\360\t!\\\t!\345\037\351\033;?G\355[((!!\\\360"
+}

test/core/transport/chttp2/hpack_parser_corpus/crash-ff4557ffdb266e8c086c05f3da8f7a1bddd6f906

@@ -0,0 +1,5 @@
+frames {
+  max_metadata_length: 4096
+  parse: "(??\201;[(\244(\'?\244\007"
+  absolute_max_metadata_length: 1073741824
+}

test/core/transport/chttp2/hpack_parser_fuzzer_test.cc

@@ -16,17 +16,23 @@
 //
 //
 
+#include <stdint.h>
+
 #include <algorithm>
 #include <memory>
 #include <string>
 #include <utility>
 
+#include "absl/cleanup/cleanup.h"
+
 #include <grpc/grpc.h>
 #include <grpc/slice.h>
 #include <grpc/support/log.h>
 
 #include "src/core/ext/transport/chttp2/transport/hpack_parser.h"
 #include "src/core/lib/gprpp/ref_counted_ptr.h"
+#include "src/core/lib/gprpp/status_helper.h"
+#include "src/core/lib/iomgr/error.h"
 #include "src/core/lib/iomgr/exec_ctx.h"
 #include "src/core/lib/resource_quota/arena.h"
 #include "src/core/lib/resource_quota/memory_quota.h"
@@ -45,40 +51,55 @@ static void dont_log(gpr_log_func_args* /*args*/) {}
 DEFINE_PROTO_FUZZER(const hpack_parser_fuzzer::Msg& msg) {
   if (squelch) gpr_set_log_function(dont_log);
   grpc_init();
+  auto cleanup = absl::MakeCleanup(grpc_shutdown);
   auto memory_allocator = grpc_core::ResourceQuota::Default()
                               ->memory_quota()
                               ->CreateMemoryAllocator("test-allocator");
   {
     std::unique_ptr<grpc_core::HPackParser> parser(new grpc_core::HPackParser);
+    int max_length = 1024;
+    int absolute_max_length = 1024;
+    bool can_update_max_length = true;
+    bool can_add_priority = true;
     for (int i = 0; i < msg.frames_size(); i++) {
       auto arena = grpc_core::MakeScopedArena(1024, &memory_allocator);
       grpc_core::ExecCtx exec_ctx;
       grpc_metadata_batch b(arena.get());
-
       const auto& frame = msg.frames(i);
+      if (frame.parse_size() == 0) continue;
+
+      // we can only update max length after a frame boundary
+      // so simulate that here
+      if (can_update_max_length) {
+        if (frame.max_metadata_length() != 0) {
+          max_length = std::max(0, frame.max_metadata_length());
+        }
+        if (frame.absolute_max_metadata_length() != 0) {
+          absolute_max_length =
+              std::max(0, frame.absolute_max_metadata_length());
+        }
+        if (absolute_max_length < max_length) {
+          std::swap(absolute_max_length, max_length);
+        }
+      }
+      // priority only makes sense on the first frame of a stream
+      grpc_core::HPackParser::Priority priority =
+          grpc_core::HPackParser::Priority::None;
+      if (can_add_priority && frame.priority()) {
+        priority = grpc_core::HPackParser::Priority::Included;
+      }
       grpc_core::HPackParser::Boundary boundary =
           grpc_core::HPackParser::Boundary::None;
+      can_update_max_length = false;
+      can_add_priority = false;
       if (frame.end_of_headers()) {
         boundary = grpc_core::HPackParser::Boundary::EndOfHeaders;
+        can_update_max_length = true;
       }
       if (frame.end_of_stream()) {
         boundary = grpc_core::HPackParser::Boundary::EndOfStream;
-      }
-      grpc_core::HPackParser::Priority priority =
-          grpc_core::HPackParser::Priority::None;
-      if (frame.priority()) {
-        priority = grpc_core::HPackParser::Priority::Included;
-      }
-      int max_length = 1024;
-      int absolute_max_length = 1024;
-      if (absolute_max_length < max_length) {
-        std::swap(absolute_max_length, max_length);
-      }
-      if (frame.max_metadata_length() != 0) {
-        max_length = frame.max_metadata_length();
-      }
-      if (frame.absolute_max_metadata_length() != 0) {
-        absolute_max_length = frame.absolute_max_metadata_length();
+        can_update_max_length = true;
+        can_add_priority = true;
       }
 
       parser->BeginFrame(
@@ -87,16 +108,33 @@ DEFINE_PROTO_FUZZER(const hpack_parser_fuzzer::Msg& msg) {
               1, grpc_core::HPackParser::LogInfo::kHeaders, false});
       int stop_buffering_ctr =
           std::max(-1, frame.stop_buffering_after_segments());
-      for (const auto& parse : frame.parse()) {
+      for (int idx = 0; idx < frame.parse_size(); idx++) {
+        const auto& parse = frame.parse(idx);
         grpc_slice buffer =
             grpc_slice_from_copied_buffer(parse.data(), parse.size());
-        (void)parser->Parse(buffer, i == msg.frames_size() - 1);
+        auto err = parser->Parse(buffer, idx == frame.parse_size() - 1);
         grpc_slice_unref(buffer);
         stop_buffering_ctr--;
         if (0 == stop_buffering_ctr) parser->StopBufferingFrame();
+        // Ensure we never take on more than four times the absolute limit in
+        // buffer size.
+        // (This is incredibly generous, but having a bound nevertheless means
+        // we don't accidentally flow to infinity, which would be crossing the
+        // streams level bad).
+        GPR_ASSERT(static_cast<int>(parser->buffered_bytes() / 4) <
+                   std::max(1024, absolute_max_length));
+        if (!err.ok()) {
+          intptr_t unused;
+          if (grpc_error_get_int(err, grpc_core::StatusIntProperty::kStreamId,
+                                 &unused)) {
+            // This is a stream error, we ignore it
+          } else {
+            // This is a connection error, we don't try to parse anymore
+            return;
+          }
+        }
       }
       parser->FinishFrame();
     }
   }
-  grpc_shutdown();
 }