From 4a3092a29cf0cf53595c137c7b20909255f78923 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Thu, 1 Aug 2024 13:02:21 -0700 Subject: [PATCH] [v1.59] [chttp2] Fix a bug in hpack error handling (#37367) PiperOrigin-RevId: 657234128 PiperOrigin-RevId: 658458047 --- .../chttp2/transport/hpack_parser.cc | 63 +++++++------ .../transport/chttp2/transport/hpack_parser.h | 2 + .../transport/chttp2/hpack_parser_test.cc | 89 ++++++++++++++++--- .../transport/chttp2/hpack_sync_fuzzer.cc | 62 +++++++++++++ .../transport/chttp2/hpack_sync_fuzzer.proto | 3 + 5 files changed, 179 insertions(+), 40 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser.cc b/src/core/ext/transport/chttp2/transport/hpack_parser.cc index 31bf46456f1d0..f2fe80c504e58 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_parser.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_parser.cc @@ -91,12 +91,14 @@ constexpr Base64InverseTable kBase64InverseTable; class HPackParser::Input { public: Input(grpc_slice_refcount* current_slice_refcount, const uint8_t* begin, - const uint8_t* end, absl::BitGenRef bitsrc, HpackParseResult& error) + const uint8_t* end, absl::BitGenRef bitsrc, + HpackParseResult& frame_error, HpackParseResult& field_error) : current_slice_refcount_(current_slice_refcount), begin_(begin), end_(end), frontier_(begin), - error_(error), + frame_error_(frame_error), + field_error_(field_error), bitsrc_(bitsrc) {} // If input is backed by a slice, retrieve its refcount. If not, return @@ -215,14 +217,18 @@ class HPackParser::Input { // Check if we saw an EOF bool eof_error() const { - return min_progress_size_ != 0 || error_.connection_error(); + return min_progress_size_ != 0 || frame_error_.connection_error(); + } + + // Reset the field error to be ok + void ClearFieldError() { + if (field_error_.ok()) return; + field_error_ = HpackParseResult(); } // Minimum number of bytes to unstuck the current parse size_t min_progress_size() const { return min_progress_size_; } - bool has_error() const { return !error_.ok(); } - // Set the current error - tweaks the error to include a stream id so that // chttp2 does not close the connection. // Intended for errors that are specific to a stream and recoverable. @@ -246,10 +252,7 @@ class HPackParser::Input { // read prior to being able to get further in this parse. void UnexpectedEOF(size_t min_progress_size) { GPR_ASSERT(min_progress_size > 0); - if (min_progress_size_ != 0 || error_.connection_error()) { - GPR_DEBUG_ASSERT(eof_error()); - return; - } + if (eof_error()) return; // Set min progress size, taking into account bytes parsed already but not // consumed. min_progress_size_ = min_progress_size + (begin_ - frontier_); @@ -302,13 +305,18 @@ class HPackParser::Input { // Do not use this directly, instead use SetErrorAndContinueParsing or // SetErrorAndStopParsing. void SetError(HpackParseResult error) { - if (!error_.ok() || min_progress_size_ > 0) { - if (error.connection_error() && !error_.connection_error()) { - error_ = std::move(error); // connection errors dominate + SetErrorFor(frame_error_, error); + SetErrorFor(field_error_, std::move(error)); + } + + void SetErrorFor(HpackParseResult& error, HpackParseResult new_error) { + if (!error.ok() || min_progress_size_ > 0) { + if (new_error.connection_error() && !error.connection_error()) { + error = std::move(new_error); // connection errors dominate } return; } - error_ = std::move(error); + error = std::move(new_error); } // Refcount if we are backed by a slice @@ -320,7 +328,8 @@ class HPackParser::Input { // Frontier denotes the first byte past successfully processed input const uint8_t* frontier_; // Current error - HpackParseResult& error_; + HpackParseResult& frame_error_; + HpackParseResult& field_error_; // If the error was EOF, we flag it here by noting how many more bytes would // be needed to make progress size_t min_progress_size_ = 0; @@ -597,6 +606,7 @@ class HPackParser::Parser { bool ParseTop() { GPR_DEBUG_ASSERT(state_.parse_state == ParseState::kTop); auto cur = *input_->Next(); + input_->ClearFieldError(); switch (cur >> 4) { // Literal header not indexed - First byte format: 0000xxxx // Literal header never indexed - First byte format: 0001xxxx @@ -702,7 +712,7 @@ class HPackParser::Parser { break; } gpr_log( - GPR_DEBUG, "HTTP:%d:%s:%s: %s%s", log_info_.stream_id, type, + GPR_INFO, "HTTP:%d:%s:%s: %s%s", log_info_.stream_id, type, log_info_.is_client ? "CLI" : "SVR", memento.md.DebugString().c_str(), memento.parse_status == nullptr ? "" @@ -951,11 +961,10 @@ class HPackParser::Parser { state_.string_length) : String::Parse(input_, state_.is_string_huff_compressed, state_.string_length); - HpackParseResult& status = state_.frame_error; absl::string_view key_string; if (auto* s = absl::get_if(&state_.key)) { key_string = s->as_string_view(); - if (status.ok()) { + if (state_.field_error.ok()) { auto r = ValidateKey(key_string); if (r != ValidateMetadataResult::kOk) { input_->SetErrorAndContinueParsing( @@ -965,7 +974,7 @@ class HPackParser::Parser { } else { const auto* memento = absl::get(state_.key); key_string = memento->md.key(); - if (status.ok() && memento->parse_status != nullptr) { + if (state_.field_error.ok() && memento->parse_status != nullptr) { input_->SetErrorAndContinueParsing(*memento->parse_status); } } @@ -992,16 +1001,16 @@ class HPackParser::Parser { key_string.size() + value.wire_size + hpack_constants::kEntryOverhead; auto md = grpc_metadata_batch::Parse( key_string, std::move(value_slice), state_.add_to_table, transport_size, - [key_string, &status, this](absl::string_view message, const Slice&) { - if (!status.ok()) return; + [key_string, this](absl::string_view message, const Slice&) { + if (!state_.field_error.ok()) return; input_->SetErrorAndContinueParsing( HpackParseResult::MetadataParseError(key_string)); gpr_log(GPR_ERROR, "Error parsing '%s' metadata: %s", std::string(key_string).c_str(), std::string(message).c_str()); }); - HPackTable::Memento memento{std::move(md), - status.PersistentStreamErrorOrNullptr()}; + HPackTable::Memento memento{ + std::move(md), state_.field_error.PersistentStreamErrorOrNullptr()}; input_->UpdateFrontier(); state_.parse_state = ParseState::kTop; if (state_.add_to_table) { @@ -1163,13 +1172,13 @@ grpc_error_handle HPackParser::Parse( std::vector buffer = std::move(unparsed_bytes_); return ParseInput( Input(nullptr, buffer.data(), buffer.data() + buffer.size(), bitsrc, - state_.frame_error), + state_.frame_error, state_.field_error), is_last, call_tracer); } - return ParseInput( - Input(slice.refcount, GRPC_SLICE_START_PTR(slice), - GRPC_SLICE_END_PTR(slice), bitsrc, state_.frame_error), - is_last, call_tracer); + return ParseInput(Input(slice.refcount, GRPC_SLICE_START_PTR(slice), + GRPC_SLICE_END_PTR(slice), bitsrc, state_.frame_error, + state_.field_error), + is_last, call_tracer); } grpc_error_handle HPackParser::ParseInput( diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser.h b/src/core/ext/transport/chttp2/transport/hpack_parser.h index 6b28daad61d4b..8f7132092958c 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_parser.h +++ b/src/core/ext/transport/chttp2/transport/hpack_parser.h @@ -236,6 +236,8 @@ class HPackParser { HPackTable hpack_table; // Error so far for this frame (set by class Input) HpackParseResult frame_error; + // Error so far for this field (set by class Input) + HpackParseResult field_error; // Length of frame so far. uint32_t frame_length = 0; // Length of the string being parsed diff --git a/test/core/transport/chttp2/hpack_parser_test.cc b/test/core/transport/chttp2/hpack_parser_test.cc index 3772d909b9b8b..d5b9c6cb68da2 100644 --- a/test/core/transport/chttp2/hpack_parser_test.cc +++ b/test/core/transport/chttp2/hpack_parser_test.cc @@ -440,19 +440,82 @@ INSTANTIATE_TEST_SUITE_P( Test{"Base64LegalEncoding", {}, {}, - {// Binary metadata: created using: - // tools/codegen/core/gen_header_frame.py - // --compression inc --no_framing --output hexstr - // < test/core/transport/chttp2/bad-base64.headers - {"4009612e622e632d62696e1c6c75636b696c7920666f722075732c206974" - "27732074756573646179", - absl::InternalError("Error parsing 'a.b.c-bin' metadata: " - "illegal base64 encoding"), - 0}, - {"be", - absl::InternalError("Error parsing 'a.b.c-bin' metadata: " - "illegal base64 encoding"), - 0}}}, + { + // Binary metadata: created using: + // tools/codegen/core/gen_header_frame.py + // --compression inc --no_framing --output hexstr + // < test/core/transport/chttp2/bad-base64.headers + {"4009612e622e632d62696e1c6c75636b696c7920666f722075732c206974" + "27732074756573646179", + absl::InternalError("Error parsing 'a.b.c-bin' metadata: " + "illegal base64 encoding"), + 0}, + {"be", + absl::InternalError("Error parsing 'a.b.c-bin' metadata: " + "illegal base64 encoding"), + kEndOfHeaders}, + {"82", ":method: GET\n", 0}, + }}, + Test{"Base64LegalEncodingWorksAfterFailure", + {}, + {}, + { + // Binary metadata: created using: + // tools/codegen/core/gen_header_frame.py + // --compression inc --no_framing --output hexstr + // < test/core/transport/chttp2/bad-base64.headers + {"4009612e622e632d62696e1c6c75636b696c7920666f722075732c206974" + "27732074756573646179", + absl::InternalError("Error parsing 'a.b.c-bin' metadata: " + "illegal base64 encoding"), + 0}, + {"be", + absl::InternalError("Error parsing 'a.b.c-bin' metadata: " + "illegal base64 encoding"), + 0}, + {"400e636f6e74656e742d6c656e6774680135", + absl::InternalError("Error parsing 'a.b.c-bin' metadata: " + "illegal base64 encoding"), + kEndOfHeaders}, + {"be", "content-length: 5\n", 0}, + }}, + Test{"Base64LegalEncodingWorksAfterFailure2", + {}, + {}, + { + {// Generated with: tools/codegen/core/gen_header_frame.py + // --compression inc --output hexstr --no_framing < + // test/core/transport/chttp2/MiXeD-CaSe.headers + "400a4d695865442d436153651073686f756c64206e6f74207061727365", + absl::InternalError("Illegal header key: MiXeD-CaSe"), 0}, + // Binary metadata: created using: + // tools/codegen/core/gen_header_frame.py + // --compression inc --no_framing --output hexstr + // < test/core/transport/chttp2/bad-base64.headers + {"4009612e622e632d62696e1c6c75636b696c7920666f722075732c206974" + "27732074756573646179", + absl::InternalError("Illegal header key: MiXeD-CaSe"), 0}, + {"be", absl::InternalError("Illegal header key: MiXeD-CaSe"), + 0}, + {"400e636f6e74656e742d6c656e6774680135", + absl::InternalError("Illegal header key: MiXeD-CaSe"), + kEndOfHeaders}, + {"be", "content-length: 5\n", 0}, + {"bf", + absl::InternalError("Error parsing 'a.b.c-bin' metadata: " + "illegal base64 encoding"), + 0}, + // Only the first error in each frame is reported, so we should + // still see the same error here... + {"c0", + absl::InternalError("Error parsing 'a.b.c-bin' metadata: " + "illegal base64 encoding"), + kEndOfHeaders}, + // ... but if we look at the next frame we should see the + // stored error + {"c0", absl::InternalError("Illegal header key: MiXeD-CaSe"), + kEndOfHeaders}, + }}, Test{"TeIsTrailers", {}, {}, diff --git a/test/core/transport/chttp2/hpack_sync_fuzzer.cc b/test/core/transport/chttp2/hpack_sync_fuzzer.cc index 81809afcf7175..40de14def3d92 100644 --- a/test/core/transport/chttp2/hpack_sync_fuzzer.cc +++ b/test/core/transport/chttp2/hpack_sync_fuzzer.cc @@ -86,6 +86,10 @@ void FuzzOneInput(const hpack_sync_fuzzer::Msg& msg) { // Not an interesting case to fuzz continue; } + if (msg.check_ab_preservation() && + header.literal_inc_idx().key() == "a") { + continue; + } if (absl::EndsWith(header.literal_inc_idx().value(), "-bin")) { std::ignore = encoder.EmitLitHdrWithBinaryStringKeyIncIdx( Slice::FromCopiedString(header.literal_inc_idx().key()), @@ -97,6 +101,10 @@ void FuzzOneInput(const hpack_sync_fuzzer::Msg& msg) { } break; case hpack_sync_fuzzer::Header::kLiteralNotIdx: + if (msg.check_ab_preservation() && + header.literal_not_idx().key() == "a") { + continue; + } if (absl::EndsWith(header.literal_not_idx().value(), "-bin")) { encoder.EmitLitHdrWithBinaryStringKeyNotIdx( Slice::FromCopiedString(header.literal_not_idx().key()), @@ -115,6 +123,10 @@ void FuzzOneInput(const hpack_sync_fuzzer::Msg& msg) { break; } } + if (msg.check_ab_preservation()) { + std::ignore = encoder.EmitLitHdrWithNonBinaryStringKeyIncIdx( + Slice::FromCopiedString("a"), Slice::FromCopiedString("b")); + } // STAGE 2: Decode the buffer (encode_output) into a list of headers HPackParser parser; @@ -141,6 +153,21 @@ void FuzzOneInput(const hpack_sync_fuzzer::Msg& msg) { } } + if (seen_errors.empty() && msg.check_ab_preservation()) { + std::string backing; + auto a_value = read_metadata.GetStringValue("a", &backing); + if (!a_value.has_value()) { + fprintf(stderr, "Expected 'a' header to be present: %s\n", + read_metadata.DebugString().c_str()); + abort(); + } + if (a_value != "b") { + fprintf(stderr, "Expected 'a' header to be 'b', got '%s'\n", + std::string(*a_value).c_str()); + abort(); + } + } + // STAGE 3: If we reached here we either had a stream error or no error // parsing. // Either way, the hpack tables should be of the same size between client and @@ -169,6 +196,41 @@ void FuzzOneInput(const hpack_sync_fuzzer::Msg& msg) { } abort(); } + + if (msg.check_ab_preservation()) { + SliceBuffer encode_output_2; + hpack_encoder_detail::Encoder encoder_2( + &compressor, msg.use_true_binary_metadata(), encode_output_2); + encoder_2.EmitIndexed(62); + GPR_ASSERT(encode_output_2.Count() == 1); + grpc_metadata_batch read_metadata_2(arena.get()); + parser.BeginFrame( + &read_metadata_2, 1024, 1024, HPackParser::Boundary::EndOfHeaders, + HPackParser::Priority::None, + HPackParser::LogInfo{3, HPackParser::LogInfo::kHeaders, false}); + auto err = parser.Parse(encode_output_2.c_slice_at(0), true, + absl::BitGenRef(proto_bit_src), + /*call_tracer=*/nullptr); + if (!err.ok()) { + fprintf(stderr, "Error parsing preservation encoded data: %s\n", + err.ToString().c_str()); + abort(); + } + std::string backing; + auto a_value = read_metadata_2.GetStringValue("a", &backing); + if (!a_value.has_value()) { + fprintf(stderr, + "Expected 'a' header to be present: %s\nfirst metadata: %s\n", + read_metadata_2.DebugString().c_str(), + read_metadata.DebugString().c_str()); + abort(); + } + if (a_value != "b") { + fprintf(stderr, "Expected 'a' header to be 'b', got '%s'\n", + std::string(*a_value).c_str()); + abort(); + } + } } } // namespace diff --git a/test/core/transport/chttp2/hpack_sync_fuzzer.proto b/test/core/transport/chttp2/hpack_sync_fuzzer.proto index 72792b60d640a..2c075a6abb1a1 100644 --- a/test/core/transport/chttp2/hpack_sync_fuzzer.proto +++ b/test/core/transport/chttp2/hpack_sync_fuzzer.proto @@ -44,4 +44,7 @@ message Msg { repeated Header headers = 2; grpc.testing.FuzzConfigVars config_vars = 3; repeated uint64 random_numbers = 4; + // Ensure that a header "a: b" appended to headers with hpack incremental + // indexing is correctly added to the hpack table. + bool check_ab_preservation = 5; }