diff --git a/src/core/ext/transport/chaotic_good/chaotic_good_transport.h b/src/core/ext/transport/chaotic_good/chaotic_good_transport.h index b42724d40b7..3230d6f4252 100644 --- a/src/core/ext/transport/chaotic_good/chaotic_good_transport.h +++ b/src/core/ext/transport/chaotic_good/chaotic_good_transport.h @@ -52,7 +52,9 @@ class ChaoticGoodTransport : public RefCounted { } auto WriteFrame(const FrameInterface& frame) { - auto buffers = frame.Serialize(&encoder_); + bool saw_encoding_errors = false; + auto buffers = frame.Serialize(&encoder_, saw_encoding_errors); + // ignore encoding errors: they will be logged separately already if (GRPC_TRACE_FLAG_ENABLED(chaotic_good)) { gpr_log(GPR_INFO, "CHAOTIC_GOOD: WriteFrame to:%s %s", ResolvedAddressToString(control_endpoint_.GetPeerAddress()) diff --git a/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc b/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc index 57e92d9131b..0f71d509ba4 100644 --- a/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc +++ b/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc @@ -114,7 +114,10 @@ auto ChaoticGoodConnector::DataEndpointWriteSettingsFrame( frame.headers = SettingsMetadata{SettingsMetadata::ConnectionType::kData, self->connection_id_, kDataAlignmentBytes} .ToMetadataBatch(); - auto write_buffer = frame.Serialize(&self->hpack_compressor_); + bool saw_encoding_errors = false; + auto write_buffer = + frame.Serialize(&self->hpack_compressor_, saw_encoding_errors); + // ignore encoding errors: they will be logged separately already return self->data_endpoint_.Write(std::move(write_buffer.control)); } @@ -215,7 +218,10 @@ auto ChaoticGoodConnector::ControlEndpointWriteSettingsFrame( frame.headers = SettingsMetadata{SettingsMetadata::ConnectionType::kControl, absl::nullopt, absl::nullopt} .ToMetadataBatch(); - auto write_buffer = frame.Serialize(&self->hpack_compressor_); + bool saw_encoding_errors = false; + auto write_buffer = + frame.Serialize(&self->hpack_compressor_, saw_encoding_errors); + // ignore encoding errors: they will be logged separately already return self->control_endpoint_.Write(std::move(write_buffer.control)); } diff --git a/src/core/ext/transport/chaotic_good/frame.cc b/src/core/ext/transport/chaotic_good/frame.cc index e54465c29d3..bbfb25d1b30 100644 --- a/src/core/ext/transport/chaotic_good/frame.cc +++ b/src/core/ext/transport/chaotic_good/frame.cc @@ -218,10 +218,12 @@ absl::Status SettingsFrame::Deserialize(HPackParser* parser, return deserializer.Finish(); } -BufferPair SettingsFrame::Serialize(HPackCompressor* encoder) const { +BufferPair SettingsFrame::Serialize(HPackCompressor* encoder, + bool& saw_encoding_errors) const { FrameSerializer serializer(FrameType::kSettings, 0); if (headers.get() != nullptr) { - encoder->EncodeRawHeaders(*headers.get(), serializer.AddHeaders()); + saw_encoding_errors |= + !encoder->EncodeRawHeaders(*headers.get(), serializer.AddHeaders()); } return serializer.Finish(); } @@ -275,11 +277,13 @@ absl::Status ClientFragmentFrame::Deserialize(HPackParser* parser, return deserializer.Finish(); } -BufferPair ClientFragmentFrame::Serialize(HPackCompressor* encoder) const { +BufferPair ClientFragmentFrame::Serialize(HPackCompressor* encoder, + bool& saw_encoding_errors) const { CHECK_NE(stream_id, 0u); FrameSerializer serializer(FrameType::kFragment, stream_id); if (headers.get() != nullptr) { - encoder->EncodeRawHeaders(*headers.get(), serializer.AddHeaders()); + saw_encoding_errors |= + !encoder->EncodeRawHeaders(*headers.get(), serializer.AddHeaders()); } if (message.has_value()) { serializer.AddMessage(message.value()); @@ -354,17 +358,20 @@ absl::Status ServerFragmentFrame::Deserialize(HPackParser* parser, return deserializer.Finish(); } -BufferPair ServerFragmentFrame::Serialize(HPackCompressor* encoder) const { +BufferPair ServerFragmentFrame::Serialize(HPackCompressor* encoder, + bool& saw_encoding_errors) const { CHECK_NE(stream_id, 0u); FrameSerializer serializer(FrameType::kFragment, stream_id); if (headers.get() != nullptr) { - encoder->EncodeRawHeaders(*headers.get(), serializer.AddHeaders()); + saw_encoding_errors |= + !encoder->EncodeRawHeaders(*headers.get(), serializer.AddHeaders()); } if (message.has_value()) { serializer.AddMessage(message.value()); } if (trailers.get() != nullptr) { - encoder->EncodeRawHeaders(*trailers.get(), serializer.AddTrailers()); + saw_encoding_errors |= + !encoder->EncodeRawHeaders(*trailers.get(), serializer.AddTrailers()); } return serializer.Finish(); } @@ -399,7 +406,7 @@ absl::Status CancelFrame::Deserialize(HPackParser*, const FrameHeader& header, return deserializer.Finish(); } -BufferPair CancelFrame::Serialize(HPackCompressor*) const { +BufferPair CancelFrame::Serialize(HPackCompressor*, bool&) const { CHECK_NE(stream_id, 0u); FrameSerializer serializer(FrameType::kCancel, stream_id); return serializer.Finish(); diff --git a/src/core/ext/transport/chaotic_good/frame.h b/src/core/ext/transport/chaotic_good/frame.h index 2d820e85cce..d521a483101 100644 --- a/src/core/ext/transport/chaotic_good/frame.h +++ b/src/core/ext/transport/chaotic_good/frame.h @@ -55,9 +55,15 @@ class FrameInterface { const FrameHeader& header, absl::BitGenRef bitsrc, Arena* arena, BufferPair buffers, FrameLimits limits) = 0; - virtual BufferPair Serialize(HPackCompressor* encoder) const = 0; + virtual BufferPair Serialize(HPackCompressor* encoder, + bool& saw_encoding_errors) const = 0; virtual std::string ToString() const = 0; + template + friend void AbslStringify(Sink& sink, const FrameInterface& frame) { + sink.Append(frame.ToString()); + } + protected: static bool EqVal(const grpc_metadata_batch& a, const grpc_metadata_batch& b) { @@ -72,11 +78,16 @@ class FrameInterface { ~FrameInterface() = default; }; +inline std::ostream& operator<<(std::ostream& os, const FrameInterface& frame) { + return os << frame.ToString(); +} + struct SettingsFrame final : public FrameInterface { absl::Status Deserialize(HPackParser* parser, const FrameHeader& header, absl::BitGenRef bitsrc, Arena* arena, BufferPair buffers, FrameLimits limits) override; - BufferPair Serialize(HPackCompressor* encoder) const override; + BufferPair Serialize(HPackCompressor* encoder, + bool& saw_encoding_errors) const override; ClientMetadataHandle headers; std::string ToString() const override; @@ -110,7 +121,8 @@ struct ClientFragmentFrame final : public FrameInterface { absl::Status Deserialize(HPackParser* parser, const FrameHeader& header, absl::BitGenRef bitsrc, Arena* arena, BufferPair buffers, FrameLimits limits) override; - BufferPair Serialize(HPackCompressor* encoder) const override; + BufferPair Serialize(HPackCompressor* encoder, + bool& saw_encoding_errors) const override; std::string ToString() const override; uint32_t stream_id; @@ -128,7 +140,8 @@ struct ServerFragmentFrame final : public FrameInterface { absl::Status Deserialize(HPackParser* parser, const FrameHeader& header, absl::BitGenRef bitsrc, Arena* arena, BufferPair buffers, FrameLimits limits) override; - BufferPair Serialize(HPackCompressor* encoder) const override; + BufferPair Serialize(HPackCompressor* encoder, + bool& saw_encoding_errors) const override; std::string ToString() const override; uint32_t stream_id; @@ -146,7 +159,8 @@ struct CancelFrame final : public FrameInterface { absl::Status Deserialize(HPackParser* parser, const FrameHeader& header, absl::BitGenRef bitsrc, Arena* arena, BufferPair buffers, FrameLimits limits) override; - BufferPair Serialize(HPackCompressor* encoder) const override; + BufferPair Serialize(HPackCompressor* encoder, + bool& saw_encoding_errors) const override; std::string ToString() const override; uint32_t stream_id; diff --git a/src/core/ext/transport/chaotic_good/frame_header.h b/src/core/ext/transport/chaotic_good/frame_header.h index 8bfbe3effb1..760c9c0d07d 100644 --- a/src/core/ext/transport/chaotic_good/frame_header.h +++ b/src/core/ext/transport/chaotic_good/frame_header.h @@ -34,6 +34,19 @@ enum class FrameType : uint8_t { kCancel = 0x81, }; +inline std::ostream& operator<<(std::ostream& out, FrameType type) { + switch (type) { + case FrameType::kSettings: + return out << "Settings"; + case FrameType::kFragment: + return out << "Fragment"; + case FrameType::kCancel: + return out << "Cancel"; + default: + return out << "Unknown[" << static_cast(type) << "]"; + } +} + struct FrameHeader { FrameType type = FrameType::kCancel; BitSet<3> flags; diff --git a/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc b/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc index dd606c98bba..6964d4d422c 100644 --- a/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc +++ b/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc @@ -336,7 +336,10 @@ auto ChaoticGoodServerListener::ActiveConnection::HandshakingState:: SettingsMetadata{absl::nullopt, self->connection_->connection_id_, absl::nullopt} .ToMetadataBatch(); - auto write_buffer = frame.Serialize(&self->connection_->hpack_compressor_); + bool saw_encoding_errors = false; + auto write_buffer = frame.Serialize(&self->connection_->hpack_compressor_, + saw_encoding_errors); + // ignore encoding errors: they will be logged separately already return TrySeq( self->connection_->endpoint_.Write(std::move(write_buffer.control)), WaitForDataEndpointSetup(self)); @@ -350,7 +353,10 @@ auto ChaoticGoodServerListener::ActiveConnection::HandshakingState:: SettingsMetadata{absl::nullopt, self->connection_->connection_id_, self->connection_->data_alignment_} .ToMetadataBatch(); - auto write_buffer = frame.Serialize(&self->connection_->hpack_compressor_); + bool saw_encoding_errors = false; + auto write_buffer = frame.Serialize(&self->connection_->hpack_compressor_, + saw_encoding_errors); + // ignore encoding errors: they will be logged separately already return TrySeq( self->connection_->endpoint_.Write(std::move(write_buffer.control)), [self]() mutable { diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc index f8d07dba1e7..2ca62cd2073 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc @@ -377,7 +377,8 @@ void Compressor::EncodeWith( encoder->EmitIndexed(7); // :scheme: https break; case HttpSchemeMetadata::ValueType::kInvalid: - Crash("invalid http scheme encoding"); + LOG(ERROR) << "Not encoding bad http scheme"; + encoder->NoteEncodingError(); break; } } @@ -434,7 +435,8 @@ void Compressor::EncodeWith( Slice::FromStaticString(":method"), Slice::FromStaticString("PUT")); break; case HttpMethodMetadata::ValueType::kInvalid: - Crash("invalid http method encoding"); + LOG(ERROR) << "Not encoding bad http method"; + encoder->NoteEncodingError(); break; } } diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.h b/src/core/ext/transport/chttp2/transport/hpack_encoder.h index 94c520ae55b..b3db3db70b4 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder.h +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.h @@ -82,10 +82,14 @@ class Encoder { const Slice& slice, uint32_t* index, size_t max_compression_size); + void NoteEncodingError() { saw_encoding_errors_ = true; } + bool saw_encoding_errors() const { return saw_encoding_errors_; } + HPackEncoderTable& hpack_table(); private: const bool use_true_binary_metadata_; + bool saw_encoding_errors_ = false; HPackCompressor* const compressor_; SliceBuffer& output_; }; @@ -207,6 +211,7 @@ class Compressor< gpr_log(GPR_ERROR, "%s", absl::StrCat("Not encoding bad ", MetadataTrait::key(), " header") .c_str()); + encoder->NoteEncodingError(); return; } Slice encoded(MetadataTrait::Encode(known_value)); @@ -354,19 +359,21 @@ class HPackCompressor { }; template - void EncodeHeaders(const EncodeHeaderOptions& options, + bool EncodeHeaders(const EncodeHeaderOptions& options, const HeaderSet& headers, grpc_slice_buffer* output) { SliceBuffer raw; hpack_encoder_detail::Encoder encoder( this, options.use_true_binary_metadata, raw); headers.Encode(&encoder); Frame(options, raw, output); + return !encoder.saw_encoding_errors(); } template - void EncodeRawHeaders(const HeaderSet& headers, SliceBuffer& output) { + bool EncodeRawHeaders(const HeaderSet& headers, SliceBuffer& output) { hpack_encoder_detail::Encoder encoder(this, true, output); headers.Encode(&encoder); + return !encoder.saw_encoding_errors(); } private: diff --git a/test/core/transport/chaotic_good/frame_fuzzer.cc b/test/core/transport/chaotic_good/frame_fuzzer.cc index 92dc762e4ac..23530d10a44 100644 --- a/test/core/transport/chaotic_good/frame_fuzzer.cc +++ b/test/core/transport/chaotic_good/frame_fuzzer.cc @@ -55,7 +55,8 @@ FrameLimits FuzzerFrameLimits() { return FrameLimits{1024 * 1024 * 1024, 63}; } template void AssertRoundTrips(const T& input, FrameType expected_frame_type) { HPackCompressor hpack_compressor; - auto serialized = input.Serialize(&hpack_compressor); + bool saw_encoding_errors = false; + auto serialized = input.Serialize(&hpack_compressor, saw_encoding_errors); CHECK(serialized.control.Length() >= 24); // Initial output buffer size is 64 byte. uint8_t header_bytes[24]; @@ -67,7 +68,7 @@ void AssertRoundTrips(const T& input, FrameType expected_frame_type) { } Crash("Failed to parse header"); } - CHECK(header->type == expected_frame_type); + CHECK_EQ(header->type, expected_frame_type); T output; HPackParser hpack_parser; DeterministicBitGen bitgen; @@ -75,7 +76,7 @@ void AssertRoundTrips(const T& input, FrameType expected_frame_type) { absl::BitGenRef(bitgen), GetContext(), std::move(serialized), FuzzerFrameLimits()); CHECK_OK(deser); - CHECK(output == input); + if (!saw_encoding_errors) CHECK_EQ(input, output); } template diff --git a/test/core/transport/chaotic_good/frame_fuzzer_corpus/clusterfuzz-testcase-minimized-frame_fuzzer-4558239670272000 b/test/core/transport/chaotic_good/frame_fuzzer_corpus/clusterfuzz-testcase-minimized-frame_fuzzer-4558239670272000 new file mode 100644 index 00000000000..da23b9563e2 --- /dev/null +++ b/test/core/transport/chaotic_good/frame_fuzzer_corpus/clusterfuzz-testcase-minimized-frame_fuzzer-4558239670272000 @@ -0,0 +1 @@ +control: "\200\001\000\000\022z`:0\000\000\000\000\000\000\000\000\000:status\234@\000\000\000\000\000\000\010\2342393\\3\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\237\23777\\3\377\377\377\0037X1user-agentrol: b0\\0\000" diff --git a/test/core/transport/chaotic_good/frame_test.cc b/test/core/transport/chaotic_good/frame_test.cc index ba61e0b9f0c..8fc0f2b41eb 100644 --- a/test/core/transport/chaotic_good/frame_test.cc +++ b/test/core/transport/chaotic_good/frame_test.cc @@ -34,7 +34,8 @@ FrameLimits TestFrameLimits() { return FrameLimits{1024 * 1024 * 1024, 63}; } template void AssertRoundTrips(const T& input, FrameType expected_frame_type) { HPackCompressor hpack_compressor; - auto serialized = input.Serialize(&hpack_compressor); + bool saw_encoding_errors = false; + auto serialized = input.Serialize(&hpack_compressor, saw_encoding_errors); CHECK_GE(serialized.control.Length(), 24); // Initial output buffer size is 64 byte. uint8_t header_bytes[24]; @@ -43,7 +44,7 @@ void AssertRoundTrips(const T& input, FrameType expected_frame_type) { if (!header.ok()) { Crash("Failed to parse header"); } - CHECK(header->type == expected_frame_type); + CHECK_EQ(header->type, expected_frame_type); T output; HPackParser hpack_parser; absl::BitGen bitgen; @@ -55,7 +56,7 @@ void AssertRoundTrips(const T& input, FrameType expected_frame_type) { output.Deserialize(&hpack_parser, header.value(), absl::BitGenRef(bitgen), arena.get(), std::move(serialized), TestFrameLimits()); CHECK_OK(deser); - CHECK(output == input); + if (!saw_encoding_errors) CHECK_EQ(output, input); } TEST(FrameTest, SettingsFrameRoundTrips) {