Skip to content

[v1.60] [chttp2] Fix a bug in hpack error handling #37361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
veblush merged 2 commits into from
Aug 1, 2024
Merged

[v1.60] [chttp2] Fix a bug in hpack error handling #37361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5718be2
[chttp2] Fix a bug in hpack error handling
ctiller Jul 29, 2024
49cc2bc
fix
ctiller Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 36 additions & 27 deletions src/core/ext/transport/chttp2/transport/hpack_parser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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_);
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
? ""
Expand Down Expand Up @@ -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<Slice>(&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(
Expand All @@ -965,7 +974,7 @@ class HPackParser::Parser {
} else {
const auto* memento = absl::get<const HPackTable::Memento*>(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);
}
}
Expand All @@ -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) {
Expand Down Expand Up @@ -1163,13 +1172,13 @@ grpc_error_handle HPackParser::Parse(
std::vector<uint8_t> 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(
Expand Down
2 changes: 2 additions & 0 deletions src/core/ext/transport/chttp2/transport/hpack_parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
89 changes: 76 additions & 13 deletions test/core/transport/chttp2/hpack_parser_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
{},
{},
Expand Down
62 changes: 62 additions & 0 deletions test/core/transport/chttp2/hpack_sync_fuzzer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,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()),
Expand All @@ -96,6 +100,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()),
Expand All @@ -114,6 +122,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;
Expand All @@ -140,6 +152,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
Expand Down Expand Up @@ -168,6 +195,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
Expand Down
3 changes: 3 additions & 0 deletions test/core/transport/chttp2/hpack_sync_fuzzer.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}