From afe5f6d2a4e31ded8667af0268ed570bf24a7a8c Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 27 Oct 2023 00:50:10 -0700 Subject: [PATCH] [chttp2] Fix bug in timeout encoding (#34751) Just the right combination of timeout values could lead to a large amount of growth in the timeout cache, leading to high memory usage and high cpu utilization on the client attempting that encoding. Fix: cap the number of cached timeout values we'll consider to a very small number (I'm choosing five). Add a fuzzer that ensures that we're actually respecting the limits of inaccuracy we're imposing upon ourselves. --------- Co-authored-by: ctiller --- fuzztest/core/transport/chttp2/BUILD | 14 ++++ .../chttp2/hpack_encoder_timeout_test.cc | 79 +++++++++++++++++++ .../chttp2/transport/hpack_encoder.cc | 26 +++--- .../chttp2/transport/hpack_encoder.h | 10 ++- .../chttp2/transport/hpack_encoder_table.h | 1 + src/core/lib/transport/timeout_encoding.h | 4 +- 6 files changed, 113 insertions(+), 21 deletions(-) create mode 100644 fuzztest/core/transport/chttp2/hpack_encoder_timeout_test.cc diff --git a/fuzztest/core/transport/chttp2/BUILD b/fuzztest/core/transport/chttp2/BUILD index b95ff53e774..84405836a4c 100644 --- a/fuzztest/core/transport/chttp2/BUILD +++ b/fuzztest/core/transport/chttp2/BUILD @@ -24,3 +24,17 @@ 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 = [ + "fuzztest", + "fuzztest_main", + "gtest", + ], + deps = [ + "//:hpack_encoder", + "//:hpack_parser", + ], +) diff --git a/fuzztest/core/transport/chttp2/hpack_encoder_timeout_test.cc b/fuzztest/core/transport/chttp2/hpack_encoder_timeout_test.cc new file mode 100644 index 00000000000..37a686c8b7b --- /dev/null +++ b/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 +#include +#include +#include +#include +#include + +#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 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 diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc index 69f2c92dbc6..ed615d5acd6 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc +++ b/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, diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.h b/src/core/ext/transport/chttp2/transport/hpack_encoder.h index 12e2df3eb6c..500126932e1 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder.h +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.h @@ -276,8 +276,10 @@ class Compressor { }; 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 previous_timeouts_; + static constexpr const size_t kNumPreviousValues = 5; + PreviousTimeout previous_timeouts_[kNumPreviousValues]; + uint32_t next_previous_value_ = 0; }; template diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder_table.h b/src/core/ext/transport/chttp2/transport/hpack_encoder_table.h index 623e50bf620..212ee1b1277 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder_table.h +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder_table.h @@ -59,6 +59,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_; } diff --git a/src/core/lib/transport/timeout_encoding.h b/src/core/lib/transport/timeout_encoding.h index 3b011fcb487..6e0eab972f0 100644 --- a/src/core/lib/transport/timeout_encoding.h +++ b/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 ParseTimeout(const Slice& text);