[chttp2] Reland: Fix bug in timeout encoding (#34812)

fuzztest/core/transport/chttp2/BUILD

@@ -24,3 +24,25 @@ grpc_fuzz_test(
     ],
     deps = ["//src/core:write_size_policy"],
 )
+
+grpc_fuzz_test(
+    name = "hpack_encoder_timeout_test",
+    srcs = ["hpack_encoder_timeout_test.cc"],
+    external_deps = [
+        "absl/random",
+        "fuzztest",
+        "fuzztest_main",
+        "gtest",
+    ],
+    deps = [
+        "//:grpc_base",
+        "//:hpack_encoder",
+        "//:hpack_parser",
+        "//:ref_counted_ptr",
+        "//src/core:arena",
+        "//src/core:memory_quota",
+        "//src/core:resource_quota",
+        "//src/core:slice_buffer",
+        "//src/core:time",
+    ],
+)

fuzztest/core/transport/chttp2/hpack_encoder_timeout_test.cc

@@ -0,0 +1,79 @@
+// 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.
+
+// Test to verify Fuzztest integration
+
+#include <grpc/event_engine/memory_allocator.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <vector>
+#include <memory>
+#include <optional>
+
+#include "absl/random/random.h"
+#include "fuzztest/fuzztest.h"
+#include "gtest/gtest.h"
+#include "src/core/ext/transport/chttp2/transport/hpack_encoder.h"
+#include "src/core/ext/transport/chttp2/transport/hpack_parser.h"
+#include "src/core/lib/resource_quota/memory_quota.h"
+#include "src/core/lib/resource_quota/resource_quota.h"
+#include "src/core/lib/gprpp/ref_counted_ptr.h"
+#include "src/core/lib/gprpp/time.h"
+#include "src/core/lib/resource_quota/arena.h"
+#include "src/core/lib/slice/slice_buffer.h"
+#include "src/core/lib/transport/metadata_batch.h"
+
+namespace grpc_core {
+
+void EncodeTimeouts(std::vector<uint32_t> timeouts) {
+  absl::BitGen bitgen;
+  ScopedTimeCache time_cache;
+  time_cache.TestOnlySetNow(Timestamp::ProcessEpoch());
+  hpack_encoder_detail::TimeoutCompressorImpl timeout_compressor;
+  HPackCompressor compressor;
+  HPackParser parser;
+  MemoryAllocator memory_allocator = MemoryAllocator(
+      ResourceQuota::Default()->memory_quota()->CreateMemoryAllocator("test"));
+  auto arena = MakeScopedArena(1024, &memory_allocator);
+  for (size_t i = 0; i < timeouts.size(); i++) {
+    SliceBuffer encoded;
+    hpack_encoder_detail::Encoder encoder(&compressor, false, encoded);
+    timeout_compressor.EncodeWith(
+        "grpc-timeout",
+        Timestamp::ProcessEpoch() + Duration::Milliseconds(timeouts[i]),
+        &encoder);
+    grpc_metadata_batch b(arena.get());
+    const uint32_t kMetadataSizeLimit = 3u * 1024 * 1024 * 1024;
+    parser.BeginFrame(
+        &b, kMetadataSizeLimit, kMetadataSizeLimit, HPackParser::Boundary::None,
+        HPackParser::Priority::None,
+        HPackParser::LogInfo{1, HPackParser::LogInfo::kHeaders, false});
+    for (size_t j = 0; j < encoded.Count(); j++) {
+      EXPECT_TRUE(parser
+                      .Parse(encoded.c_slice_at(j), j == encoded.Count() - 1,
+                             bitgen, nullptr)
+                      .ok());
+    }
+    auto parsed = b.get(GrpcTimeoutMetadata());
+    ASSERT_TRUE(parsed.has_value());
+    EXPECT_GE(*parsed,
+              Timestamp::ProcessEpoch() + Duration::Milliseconds(timeouts[i]));
+    EXPECT_LE(*parsed, Timestamp::ProcessEpoch() +
+                           Duration::Milliseconds(timeouts[i]) * 1.05 +
+                           Duration::Milliseconds(1));
+  }
+}
+FUZZ_TEST(MyTestSuite, EncodeTimeouts);
+
+}  // namespace grpc_core

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

@@ -475,31 +475,25 @@ void Encoder::EncodeRepeatingSliceValue(const absl::string_view& key,
 
 void TimeoutCompressorImpl::EncodeWith(absl::string_view key,
                                        Timestamp deadline, Encoder* encoder) {
-  Timeout timeout = Timeout::FromDuration(deadline - Timestamp::Now());
+  const Timeout timeout = Timeout::FromDuration(deadline - Timestamp::Now());
   auto& table = encoder->hpack_table();
-  for (auto it = previous_timeouts_.begin(); it != previous_timeouts_.end();
-       ++it) {
-    double ratio = timeout.RatioVersus(it->timeout);
+  for (size_t i = 0; i < kNumPreviousValues; i++) {
+    const auto& previous = previous_timeouts_[i];
+    if (!table.ConvertableToDynamicIndex(previous.index)) continue;
+    const double ratio = timeout.RatioVersus(previous.timeout);
     // If the timeout we're sending is shorter than a previous timeout, but
     // within 3% of it, we'll consider sending it.
-    if (ratio > -3 && ratio <= 0 &&
-        table.ConvertableToDynamicIndex(it->index)) {
-      encoder->EmitIndexed(table.DynamicIndex(it->index));
-      // Put this timeout to the front of the queue - forces common timeouts to
-      // be considered earlier.
-      std::swap(*it, *previous_timeouts_.begin());
+    if (ratio > -3 && ratio <= 0) {
+      encoder->EmitIndexed(table.DynamicIndex(previous.index));
       return;
     }
   }
-  // Clean out some expired timeouts.
-  while (!previous_timeouts_.empty() &&
-         !table.ConvertableToDynamicIndex(previous_timeouts_.back().index)) {
-    previous_timeouts_.pop_back();
-  }
   Slice encoded = timeout.Encode();
   uint32_t index = encoder->EmitLitHdrWithNonBinaryStringKeyIncIdx(
       Slice::FromStaticString(key), std::move(encoded));
-  previous_timeouts_.push_back(PreviousTimeout{timeout, index});
+  uint32_t i = next_previous_value_;
+  ++next_previous_value_;
+  previous_timeouts_[i % kNumPreviousValues] = PreviousTimeout{timeout, index};
 }
 
 Encoder::Encoder(HPackCompressor* compressor, bool use_true_binary_metadata,

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

@@ -276,8 +276,10 @@ class Compressor<MetadataTrait, SmallSetOfValuesCompressor> {
 };
 
 struct PreviousTimeout {
-  Timeout timeout;
-  uint32_t index;
+  Timeout timeout = Timeout::FromDuration(Duration::Zero());
+  // Dynamic table index of a previously sent timeout
+  // 0 is guaranteed not in the dynamic table so is a safe initializer
+  uint32_t index = 0;
 };
 
 class TimeoutCompressorImpl {
@@ -285,7 +287,9 @@ class TimeoutCompressorImpl {
   void EncodeWith(absl::string_view key, Timestamp deadline, Encoder* encoder);
 
  private:
-  std::vector<PreviousTimeout> previous_timeouts_;
+  static constexpr const size_t kNumPreviousValues = 5;
+  PreviousTimeout previous_timeouts_[kNumPreviousValues];
+  uint32_t next_previous_value_ = 0;
 };
 
 template <typename MetadataTrait>

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

@@ -58,6 +58,7 @@ class HPackEncoderTable {
            table_elems_ - index;
   }
   // Check if an element index is convertable to a dynamic index
+  // Note that 0 is always not convertable
   bool ConvertableToDynamicIndex(uint32_t index) const {
     return index > tail_remote_index_;
   }

src/core/lib/transport/timeout_encoding.h

@@ -61,8 +61,8 @@ class Timeout {
   static Timeout FromMinutes(int64_t minutes);
   static Timeout FromHours(int64_t hours);
 
-  uint16_t value_;
-  Unit unit_;
+  uint16_t value_ = 0;
+  Unit unit_ = Unit::kNanoseconds;
 };
 
 absl::optional<Duration> ParseTimeout(const Slice& text);

tools/distrib/fix_build_deps.py

@@ -406,6 +406,7 @@ for dirname in [
     "test/core/transport/chaotic_good",
     "fuzztest",
     "fuzztest/core/channel",
+    "fuzztest/core/transport/chttp2",
 ]:
     parsing_path = dirname
     exec(