From 4fa98b22110b27cdff02577bdf5d0d06163a66fa Mon Sep 17 00:00:00 2001 From: Tianyu <72890320+tyxia@users.noreply.github.com> Date: Wed, 6 Mar 2024 10:09:24 -0500 Subject: [PATCH] grpc: Add support for max frame length in gPRC frame decoding (#32511) --------- Signed-off-by: tyxia --- source/common/grpc/codec.cc | 12 +++- source/common/grpc/codec.h | 8 +++ test/common/grpc/codec_test.cc | 101 +++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 1 deletion(-) diff --git a/source/common/grpc/codec.cc b/source/common/grpc/codec.cc index 668258642a68..eb24b4918407 100644 --- a/source/common/grpc/codec.cc +++ b/source/common/grpc/codec.cc @@ -32,13 +32,17 @@ void Encoder::prependFrameHeader(uint8_t flags, Buffer::Instance& buffer, uint32 } bool Decoder::decode(Buffer::Instance& input, std::vector& output) { + // Make sure those flags are set to initial state. decoding_error_ = false; + is_frame_oversized_ = false; output_ = &output; inspect(input); output_ = nullptr; - if (decoding_error_) { + + if (decoding_error_ || is_frame_oversized_) { return false; } + input.drain(input.length()); return true; } @@ -102,6 +106,12 @@ uint64_t FrameInspector::inspect(const Buffer::Instance& data) { case State::FhLen3: length_as_bytes_[3] = c; length_ = absl::big_endian::Load32(length_as_bytes_); + // Compares the frame length against maximum length when `max_frame_length_` is configured, + if (max_frame_length_ != 0 && length_ > max_frame_length_) { + // Set the flag to indicate the over-limit error and return. + is_frame_oversized_ = true; + return delta; + } frameDataStart(); if (length_ == 0) { frameDataEnd(); diff --git a/source/common/grpc/codec.h b/source/common/grpc/codec.h index 6140442fb84c..3b6692c96a97 100644 --- a/source/common/grpc/codec.h +++ b/source/common/grpc/codec.h @@ -113,6 +113,11 @@ class FrameInspector { uint8_t length_as_bytes_[4]; }; uint64_t count_{0}; + // Default value 0 means there is no limitation on maximum frame length. + uint32_t max_frame_length_{0}; + // When `max_frame_length_` is configured, this flag will be true if frame length is larger than + // `max_frame_length_`. + bool is_frame_oversized_{false}; }; class Decoder : public FrameInspector { @@ -134,6 +139,9 @@ class Decoder : public FrameInspector { // Indicates whether it has buffered any partial data. bool hasBufferedData() const { return state_ != State::FhFlag; } + // Configures the maximum frame length. + void setMaxFrameLength(uint32_t max_frame_length) { max_frame_length_ = max_frame_length; } + protected: bool frameStart(uint8_t) override; void frameDataStart() override; diff --git a/test/common/grpc/codec_test.cc b/test/common/grpc/codec_test.cc index c028559bc828..ce722bdd5c09 100644 --- a/test/common/grpc/codec_test.cc +++ b/test/common/grpc/codec_test.cc @@ -221,6 +221,107 @@ TEST(GrpcCodecTest, decodeMultipleFrame) { } } +TEST(GrpcCodecTest, decodeSingleFrameOverLimit) { + helloworld::HelloRequest request; + std::string test_str = std::string(64 * 1024, 'a'); + request.set_name(test_str); + + Buffer::OwnedImpl buffer; + std::array header; + Encoder encoder; + encoder.newFrame(GRPC_FH_DEFAULT, request.ByteSize(), header); + buffer.add(header.data(), 5); + buffer.add(request.SerializeAsString()); + size_t size = buffer.length(); + + std::vector frames; + // Configure decoder with 32kb max_frame_length. + Decoder decoder; + decoder.setMaxFrameLength(32 * 1024); + + // The decoder doesn't successfully decode due to oversized frame. + EXPECT_FALSE(decoder.decode(buffer, frames)); + EXPECT_EQ(buffer.length(), size); +} + +TEST(GrpcCodecTest, decodeSingleFrameWithMultiBuffersOverLimit) { + std::vector buffers(2); + std::array header; + + uint32_t max_length = 32 * 1024; + uint32_t single_buffer_length = 18 * 1024; + std::string req_str = std::string(single_buffer_length, 'a'); + + // First buffer is valid (i.e. within total_frame_length limit). + helloworld::HelloRequest request; + request.set_name(req_str); + // Second buffer itself is valid but results in the total frame size exceeding the limit. + helloworld::HelloRequest request_2; + request_2.set_name(req_str); + + Encoder encoder; + // Total frame consists of two buffers, request and request_2. + encoder.newFrame(GRPC_FH_DEFAULT, request.ByteSize() + request_2.ByteSize(), header); + + buffers[0].add(header.data(), 5); + buffers[0].add(request.SerializeAsString()); + buffers[1].add(header.data(), 5); + buffers[1].add(request_2.SerializeAsString()); + + size_t size = buffers[0].length() + buffers[1].length(); + std::vector frames = {}; + Decoder decoder; + decoder.setMaxFrameLength(max_length); + + // Both decoding attempts failed due to the total frame size exceeding the limit. + for (uint32_t i = 0; i < buffers.size(); ++i) { + EXPECT_FALSE(decoder.decode(buffers[i], frames)); + } + + EXPECT_EQ(frames.size(), 0); + // Buffer does not get drained due to it returning false. + EXPECT_EQ(buffers[0].length() + buffers[1].length(), size); +} + +TEST(GrpcCodecTest, decodeMultipleFramesOverLimit) { + Buffer::OwnedImpl buffer; + std::array header; + Encoder encoder; + + // First frame is valid (i.e. within max_frame_length limit). + helloworld::HelloRequest request; + request.set_name("hello"); + encoder.newFrame(GRPC_FH_DEFAULT, request.ByteSize(), header); + buffer.add(header.data(), 5); + buffer.add(request.SerializeAsString()); + + // Second frame is invalid (i.e. exceeds max_frame_length). + helloworld::HelloRequest overlimit_request; + std::string test_str = std::string(64 * 1024, 'a'); + overlimit_request.set_name(test_str); + encoder.newFrame(GRPC_FH_DEFAULT, overlimit_request.ByteSize(), header); + buffer.add(header.data(), 5); + buffer.add(overlimit_request.SerializeAsString()); + + size_t size = buffer.length(); + + std::vector frames; + Decoder decoder; + decoder.setMaxFrameLength(32 * 1024); + + EXPECT_FALSE(decoder.decode(buffer, frames)); + // When the decoder doesn't successfully decode, it puts valid frames up until + // an oversized frame into output frame vector. + ASSERT_EQ(frames.size(), 1); + // First frame is successfully decoded. + EXPECT_EQ(frames[0].length_, request.ByteSize()); + // Buffer does not get drained due to it returning false. + EXPECT_EQ(buffer.length(), size); + // Only part of the buffer represented a valid frame. Thus, the frame length should not equal the + // buffer length. + EXPECT_NE(frames[0].length_, size); +} + TEST(GrpcCodecTest, FrameInspectorTest) { { Buffer::OwnedImpl buffer;