Add HpackDecodingError enum, bubble up all third_party/http2/hpack/decoder errors to HpackDecoder. Replace error_detected_ Boolean members with HpackDecodingError; false becomes kOk, true is split to more gradual error codes. gfe-relnote: n/a (no functional change) PiperOrigin-RevId: 293809195 Change-Id: I8ec3264f1dd630045e438b4354b5453a3d9b3ef3
diff --git a/http2/hpack/decoder/hpack_block_decoder.h b/http2/hpack/decoder/hpack_block_decoder.h index ed4b0b0..1e60af4 100644 --- a/http2/hpack/decoder/hpack_block_decoder.h +++ b/http2/hpack/decoder/hpack_block_decoder.h
@@ -14,6 +14,7 @@ #include "net/third_party/quiche/src/http2/decoder/decode_buffer.h" #include "net/third_party/quiche/src/http2/decoder/decode_status.h" +#include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoding_error.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_entry_decoder.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_entry_decoder_listener.h" #include "net/third_party/quiche/src/http2/platform/api/http2_logging.h" @@ -49,6 +50,9 @@ // first byte of a new HPACK entry)? bool before_entry() const { return before_entry_; } + // Return error code after decoding error occurred in HpackEntryDecoder. + HpackDecodingError error() const { return entry_decoder_.error(); } + std::string DebugString() const; private:
diff --git a/http2/hpack/decoder/hpack_decoder.cc b/http2/hpack/decoder/hpack_decoder.cc index b0d5bf8..1013292 100644 --- a/http2/hpack/decoder/hpack_decoder.cc +++ b/http2/hpack/decoder/hpack_decoder.cc
@@ -16,7 +16,7 @@ : decoder_state_(listener), entry_buffer_(&decoder_state_, max_string_size), block_decoder_(&entry_buffer_), - error_detected_(false), + error_(HpackDecodingError::kOk), http2_skip_querying_entry_buffer_error_( GetHttp2ReloadableFlag(http2_skip_querying_entry_buffer_error)) {} @@ -62,8 +62,7 @@ // which finally forwards them to the HpackDecoderListener. DecodeStatus status = block_decoder_.Decode(db); if (status == DecodeStatus::kDecodeError) { - // This has probably already been reported, but just in case... - ReportError("HPACK block malformed."); + ReportError(block_decoder_.error()); HTTP2_CODE_COUNT_N(decompress_failure_3, 4, 23); return false; } else if (DetectError()) { @@ -88,7 +87,7 @@ } if (!block_decoder_.before_entry()) { // The HPACK block ended in the middle of an entry. - ReportError("HPACK block truncated."); + ReportError(HpackDecodingError::kTruncatedBlock); HTTP2_CODE_COUNT_N(decompress_failure_3, 7, 23); return false; } @@ -102,42 +101,45 @@ } bool HpackDecoder::DetectError() { - if (error_detected_) { + if (error_ != HpackDecodingError::kOk) { return true; } - if (decoder_state_.error_detected()) { - HTTP2_DVLOG(2) << "HpackDecoder::error_detected in decoder_state_"; + if (decoder_state_.error() != HpackDecodingError::kOk) { + HTTP2_DVLOG(2) << "Error detected in decoder_state_"; HTTP2_CODE_COUNT_N(decompress_failure_3, 10, 23); HTTP2_CODE_COUNT_N(http2_skip_querying_entry_buffer_error, 1, 3); - error_detected_ = true; + error_ = decoder_state_.error(); } else if (entry_buffer_.error_detected()) { // This should never happen, because if an error had occured in // |entry_buffer_|, it would have notified its listener, |decoder_state_|. if (http2_skip_querying_entry_buffer_error_) { HTTP2_CODE_COUNT_N(http2_skip_querying_entry_buffer_error, 2, 3); } else { - HTTP2_DVLOG(2) << "HpackDecoder::error_detected in entry_buffer_"; + HTTP2_DVLOG(2) << "Error detected in entry_buffer_"; HTTP2_CODE_COUNT_N(decompress_failure_3, 9, 23); HTTP2_CODE_COUNT_N(http2_skip_querying_entry_buffer_error, 3, 3); - error_detected_ = true; + // Since this code path should never be executed, error code does not + // matter as long as it is not HpackDecodingError::kOk. + error_ = HpackDecodingError::kIndexVarintError; } } - return error_detected_; + return error_ != HpackDecodingError::kOk; } size_t HpackDecoder::EstimateMemoryUsage() const { return Http2EstimateMemoryUsage(entry_buffer_); } -void HpackDecoder::ReportError(quiche::QuicheStringPiece error_message) { +void HpackDecoder::ReportError(HpackDecodingError error) { HTTP2_DVLOG(3) << "HpackDecoder::ReportError is new=" - << (!error_detected_ ? "true" : "false") - << ", error_message: " << error_message; - if (!error_detected_) { - error_detected_ = true; - decoder_state_.listener()->OnHeaderErrorDetected(error_message); + << (error_ == HpackDecodingError::kOk ? "true" : "false") + << ", error: " << HpackDecodingErrorToString(error); + if (error_ == HpackDecodingError::kOk) { + error_ = error; + decoder_state_.listener()->OnHeaderErrorDetected( + HpackDecodingErrorToString(error)); } }
diff --git a/http2/hpack/decoder/hpack_decoder.h b/http2/hpack/decoder/hpack_decoder.h index 1b332d9..6e63604 100644 --- a/http2/hpack/decoder/hpack_decoder.h +++ b/http2/hpack/decoder/hpack_decoder.h
@@ -28,9 +28,9 @@ #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoder_listener.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoder_state.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoder_tables.h" +#include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoding_error.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_whole_entry_buffer.h" #include "net/third_party/quiche/src/common/platform/api/quiche_export.h" -#include "net/third_party/quiche/src/common/platform/api/quiche_string_piece.h" namespace http2 { namespace test { @@ -93,9 +93,13 @@ bool EndDecodingBlock(); // If no error has been detected so far, query |decoder_state_| for errors and - // set |error_detected_| if necessary. + // set |error_| if necessary. Returns true if an error has ever been + // detected. bool DetectError(); + // Error code if an error has occurred, HpackDecodingError::kOk otherwise. + HpackDecodingError error() const { return error_; } + // Returns the estimate of dynamically allocated memory in bytes. size_t EstimateMemoryUsage() const; @@ -103,7 +107,7 @@ friend class test::HpackDecoderPeer; // Reports an error to the listener IF this is the first error detected. - void ReportError(quiche::QuicheStringPiece error_message); + void ReportError(HpackDecodingError error); // The decompressor state, as defined by HPACK (i.e. the static and dynamic // tables). @@ -115,8 +119,8 @@ // The decoder of HPACK blocks into entry parts, passed to entry_buffer_. HpackBlockDecoder block_decoder_; - // Has an error been detected? - bool error_detected_; + // Error code if an error has occurred, HpackDecodingError::kOk otherwise. + HpackDecodingError error_; // Latched value of reloadable_flag_http2_skip_querying_entry_buffer_error. const bool http2_skip_querying_entry_buffer_error_;
diff --git a/http2/hpack/decoder/hpack_decoder_state.cc b/http2/hpack/decoder/hpack_decoder_state.cc index 7c10b52..09b64c0 100644 --- a/http2/hpack/decoder/hpack_decoder_state.cc +++ b/http2/hpack/decoder/hpack_decoder_state.cc
@@ -31,7 +31,7 @@ require_dynamic_table_size_update_(false), allow_dynamic_table_size_update_(true), saw_dynamic_table_size_update_(false), - error_detected_(false) {} + error_(HpackDecodingError::kOk) {} HpackDecoderState::~HpackDecoderState() = default; void HpackDecoderState::set_tables_debug_listener( @@ -59,7 +59,8 @@ // This instance can't be reused after an error has been detected, as we must // assume that the encoder and decoder compression states are no longer // synchronized. - DCHECK(!error_detected_); + DCHECK(error_ == HpackDecodingError::kOk) + << HpackDecodingErrorToString(error_); DCHECK_LE(lowest_header_table_size_, final_header_table_size_); allow_dynamic_table_size_update_ = true; saw_dynamic_table_size_update_ = false; @@ -80,11 +81,11 @@ void HpackDecoderState::OnIndexedHeader(size_t index) { HTTP2_DVLOG(2) << "HpackDecoderState::OnIndexedHeader: " << index; - if (error_detected_) { + if (error_ != HpackDecodingError::kOk) { return; } if (require_dynamic_table_size_update_) { - ReportError("Missing dynamic table size update."); + ReportError(HpackDecodingError::kMissingDynamicTableSizeUpdate); return; } allow_dynamic_table_size_update_ = false; @@ -92,7 +93,7 @@ if (entry != nullptr) { listener_->OnHeader(entry->name, entry->value); } else { - ReportError("Invalid index."); + ReportError(HpackDecodingError::kInvalidIndex); } } @@ -103,11 +104,11 @@ HTTP2_DVLOG(2) << "HpackDecoderState::OnNameIndexAndLiteralValue " << entry_type << ", " << name_index << ", " << value_buffer->str(); - if (error_detected_) { + if (error_ != HpackDecodingError::kOk) { return; } if (require_dynamic_table_size_update_) { - ReportError("Missing dynamic table size update."); + ReportError(HpackDecodingError::kMissingDynamicTableSizeUpdate); return; } allow_dynamic_table_size_update_ = false; @@ -119,7 +120,7 @@ decoder_tables_.Insert(entry->name, value); } } else { - ReportError("Invalid name index."); + ReportError(HpackDecodingError::kInvalidNameIndex); } } @@ -129,11 +130,11 @@ HpackDecoderStringBuffer* value_buffer) { HTTP2_DVLOG(2) << "HpackDecoderState::OnLiteralNameAndValue " << entry_type << ", " << name_buffer->str() << ", " << value_buffer->str(); - if (error_detected_) { + if (error_ != HpackDecodingError::kOk) { return; } if (require_dynamic_table_size_update_) { - ReportError("Missing dynamic table size update."); + ReportError(HpackDecodingError::kMissingDynamicTableSizeUpdate); return; } allow_dynamic_table_size_update_ = false; @@ -151,27 +152,29 @@ << (require_dynamic_table_size_update_ ? "true" : "false") << ", allowed=" << (allow_dynamic_table_size_update_ ? "true" : "false"); - if (error_detected_) { + if (error_ != HpackDecodingError::kOk) { return; } DCHECK_LE(lowest_header_table_size_, final_header_table_size_); if (!allow_dynamic_table_size_update_) { // At most two dynamic table size updates allowed at the start, and not // after a header. - ReportError("Dynamic table size update not allowed."); + ReportError(HpackDecodingError::kDynamicTableSizeUpdateNotAllowed); return; } if (require_dynamic_table_size_update_) { // The new size must not be greater than the low water mark. if (size_limit > lowest_header_table_size_) { - ReportError("Initial dynamic table size update is above low water mark."); + ReportError(HpackDecodingError:: + kInitialDynamicTableSizeUpdateIsAboveLowWaterMark); return; } require_dynamic_table_size_update_ = false; } else if (size_limit > final_header_table_size_) { // The new size must not be greater than the final max header table size // that the peer acknowledged. - ReportError("Dynamic table size update is above acknowledged setting."); + ReportError( + HpackDecodingError::kDynamicTableSizeUpdateIsAboveAcknowledgedSetting); return; } decoder_tables_.DynamicTableSizeUpdate(size_limit); @@ -184,35 +187,35 @@ lowest_header_table_size_ = final_header_table_size_; } -void HpackDecoderState::OnHpackDecodeError( - quiche::QuicheStringPiece error_message) { - HTTP2_DVLOG(2) << "HpackDecoderState::OnHpackDecodeError " << error_message; - if (!error_detected_) { - ReportError(error_message); +void HpackDecoderState::OnHpackDecodeError(HpackDecodingError error) { + HTTP2_DVLOG(2) << "HpackDecoderState::OnHpackDecodeError " + << HpackDecodingErrorToString(error); + if (error_ == HpackDecodingError::kOk) { + ReportError(error); } } void HpackDecoderState::OnHeaderBlockEnd() { HTTP2_DVLOG(2) << "HpackDecoderState::OnHeaderBlockEnd"; - if (error_detected_) { + if (error_ != HpackDecodingError::kOk) { return; } if (require_dynamic_table_size_update_) { // Apparently the HPACK block was empty, but we needed it to contain at // least 1 dynamic table size update. - ReportError("Missing dynamic table size update."); + ReportError(HpackDecodingError::kMissingDynamicTableSizeUpdate); } else { listener_->OnHeaderListEnd(); } } -void HpackDecoderState::ReportError(quiche::QuicheStringPiece error_message) { +void HpackDecoderState::ReportError(HpackDecodingError error) { HTTP2_DVLOG(2) << "HpackDecoderState::ReportError is new=" - << (!error_detected_ ? "true" : "false") - << ", error_message: " << error_message; - if (!error_detected_) { - listener_->OnHeaderErrorDetected(error_message); - error_detected_ = true; + << (error_ == HpackDecodingError::kOk ? "true" : "false") + << ", error: " << HpackDecodingErrorToString(error); + if (error_ == HpackDecodingError::kOk) { + listener_->OnHeaderErrorDetected(HpackDecodingErrorToString(error)); + error_ = error; } }
diff --git a/http2/hpack/decoder/hpack_decoder_state.h b/http2/hpack/decoder/hpack_decoder_state.h index 4436ef3..0735f6a 100644 --- a/http2/hpack/decoder/hpack_decoder_state.h +++ b/http2/hpack/decoder/hpack_decoder_state.h
@@ -19,6 +19,7 @@ #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoder_listener.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoder_string_buffer.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoder_tables.h" +#include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoding_error.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_whole_entry_listener.h" #include "net/third_party/quiche/src/http2/hpack/http2_hpack_constants.h" #include "net/third_party/quiche/src/common/platform/api/quiche_export.h" @@ -76,15 +77,15 @@ HpackDecoderStringBuffer* name_buffer, HpackDecoderStringBuffer* value_buffer) override; void OnDynamicTableSizeUpdate(size_t size) override; - void OnHpackDecodeError(quiche::QuicheStringPiece error_message) override; + void OnHpackDecodeError(HpackDecodingError error) override; // OnHeaderBlockEnd notifies this object that an entire HPACK block has been // decoded, which might have extended into CONTINUATION blocks. void OnHeaderBlockEnd(); - // Was an error detected? After an error has been detected and reported, - // no further callbacks will be made to the listener. - bool error_detected() const { return error_detected_; } + // Returns error code after an error has been detected and reported. + // No further callbacks will be made to the listener. + HpackDecodingError error() const { return error_; } const HpackDecoderTables& decoder_tables_for_test() const { return decoder_tables_; @@ -94,7 +95,7 @@ friend class test::HpackDecoderStatePeer; // Reports an error to the listener IF this is the first error detected. - void ReportError(quiche::QuicheStringPiece error_message); + void ReportError(HpackDecodingError error); // The static and dynamic HPACK tables. HpackDecoderTables decoder_tables_; @@ -121,7 +122,7 @@ bool saw_dynamic_table_size_update_; // Has an error already been detected and reported to the listener? - bool error_detected_; + HpackDecodingError error_; }; } // namespace http2
diff --git a/http2/hpack/decoder/hpack_decoder_state_test.cc b/http2/hpack/decoder/hpack_decoder_state_test.cc index 6f54cdb..02abb55 100644 --- a/http2/hpack/decoder/hpack_decoder_state_test.cc +++ b/http2/hpack/decoder/hpack_decoder_state_test.cc
@@ -21,7 +21,6 @@ using ::testing::AssertionResult; using ::testing::AssertionSuccess; using ::testing::Eq; -using ::testing::HasSubstr; using ::testing::Mock; using ::testing::StrictMock; @@ -419,8 +418,8 @@ EXPECT_EQ(0u, header_table_size_limit()); // Three updates aren't allowed. - EXPECT_CALL(listener_, - OnHeaderErrorDetected(HasSubstr("size update not allowed"))); + EXPECT_CALL(listener_, OnHeaderErrorDetected( + Eq("Dynamic table size update not allowed"))); SendSizeUpdate(0); } @@ -442,8 +441,8 @@ // Another HPACK block, but this time missing the required size update. decoder_state_.ApplyHeaderTableSizeSetting(1024); SendStartAndVerifyCallback(); - EXPECT_CALL(listener_, OnHeaderErrorDetected( - HasSubstr("Missing dynamic table size update"))); + EXPECT_CALL(listener_, + OnHeaderErrorDetected(Eq("Missing dynamic table size update"))); decoder_state_.OnIndexedHeader(1); // Further decoded entries are ignored. @@ -456,7 +455,7 @@ decoder_state_.OnLiteralNameAndValue(HpackEntryType::kIndexedLiteralHeader, &name_buffer_, &value_buffer_); decoder_state_.OnHeaderBlockEnd(); - decoder_state_.OnHpackDecodeError("NOT FORWARDED"); + decoder_state_.OnHpackDecodeError(HpackDecodingError::kIndexVarintError); } // Confirm that required size updates are validated. @@ -466,8 +465,10 @@ SendStartAndVerifyCallback(); EXPECT_EQ(Http2SettingsInfo::DefaultHeaderTableSize(), header_table_size_limit()); - EXPECT_CALL(listener_, - OnHeaderErrorDetected(HasSubstr("above low water mark"))); + EXPECT_CALL( + listener_, + OnHeaderErrorDetected( + Eq("Initial dynamic table size update is above low water mark"))); SendSizeUpdate(2048); } @@ -475,8 +476,8 @@ TEST_F(HpackDecoderStateTest, RequiredTableSizeChangeBeforeEnd) { decoder_state_.ApplyHeaderTableSizeSetting(1024); SendStartAndVerifyCallback(); - EXPECT_CALL(listener_, OnHeaderErrorDetected( - HasSubstr("Missing dynamic table size update"))); + EXPECT_CALL(listener_, + OnHeaderErrorDetected(Eq("Missing dynamic table size update"))); decoder_state_.OnHeaderBlockEnd(); } @@ -487,26 +488,32 @@ EXPECT_EQ(Http2SettingsInfo::DefaultHeaderTableSize(), header_table_size_limit()); EXPECT_CALL(listener_, - OnHeaderErrorDetected(HasSubstr("size update is above"))); + OnHeaderErrorDetected(Eq( + "Dynamic table size update is above acknowledged setting"))); SendSizeUpdate(Http2SettingsInfo::DefaultHeaderTableSize() + 1); } TEST_F(HpackDecoderStateTest, InvalidStaticIndex) { SendStartAndVerifyCallback(); - EXPECT_CALL(listener_, OnHeaderErrorDetected(HasSubstr("Invalid index"))); + EXPECT_CALL(listener_, + OnHeaderErrorDetected( + Eq("Invalid index in indexed header field representation"))); decoder_state_.OnIndexedHeader(0); } TEST_F(HpackDecoderStateTest, InvalidDynamicIndex) { SendStartAndVerifyCallback(); - EXPECT_CALL(listener_, OnHeaderErrorDetected(HasSubstr("Invalid index"))); + EXPECT_CALL(listener_, + OnHeaderErrorDetected( + Eq("Invalid index in indexed header field representation"))); decoder_state_.OnIndexedHeader(kFirstDynamicTableIndex); } TEST_F(HpackDecoderStateTest, InvalidNameIndex) { SendStartAndVerifyCallback(); EXPECT_CALL(listener_, - OnHeaderErrorDetected(HasSubstr("Invalid name index"))); + OnHeaderErrorDetected(Eq("Invalid index in literal header field " + "with indexed name representation"))); SetValue("value", UNBUFFERED); decoder_state_.OnNameIndexAndLiteralValue( HpackEntryType::kIndexedLiteralHeader, kFirstDynamicTableIndex, @@ -515,9 +522,9 @@ TEST_F(HpackDecoderStateTest, ErrorsSuppressCallbacks) { SendStartAndVerifyCallback(); - EXPECT_CALL(listener_, OnHeaderErrorDetected(quiche::QuicheStringPiece( - "Huffman decode error."))); - decoder_state_.OnHpackDecodeError("Huffman decode error."); + EXPECT_CALL(listener_, + OnHeaderErrorDetected(Eq("Name Huffman encoding error"))); + decoder_state_.OnHpackDecodeError(HpackDecodingError::kNameHuffmanError); // Further decoded entries are ignored. decoder_state_.OnIndexedHeader(1); @@ -529,7 +536,7 @@ decoder_state_.OnLiteralNameAndValue(HpackEntryType::kIndexedLiteralHeader, &name_buffer_, &value_buffer_); decoder_state_.OnHeaderBlockEnd(); - decoder_state_.OnHpackDecodeError("NOT FORWARDED"); + decoder_state_.OnHpackDecodeError(HpackDecodingError::kIndexVarintError); } } // namespace
diff --git a/http2/hpack/decoder/hpack_decoder_test.cc b/http2/hpack/decoder/hpack_decoder_test.cc index 1b8cdd9..7d6242d 100644 --- a/http2/hpack/decoder/hpack_decoder_test.cc +++ b/http2/hpack/decoder/hpack_decoder_test.cc
@@ -31,7 +31,7 @@ using ::testing::AssertionResult; using ::testing::AssertionSuccess; using ::testing::ElementsAreArray; -using ::testing::HasSubstr; +using ::testing::Eq; namespace http2 { namespace test { @@ -940,8 +940,11 @@ hbb.AppendDynamicTableSizeUpdate(1000); hbb.AppendDynamicTableSizeUpdate(500); EXPECT_FALSE(DecodeBlock(hbb.buffer())); + EXPECT_EQ(HpackDecodingError::kDynamicTableSizeUpdateNotAllowed, + decoder_.error()); EXPECT_EQ(1u, error_messages_.size()); - EXPECT_THAT(error_messages_[0], HasSubstr("size update not allowed")); + EXPECT_THAT(error_messages_[0], + Eq("Dynamic table size update not allowed")); EXPECT_EQ(1000u, header_table_size_limit()); EXPECT_EQ(0u, current_header_table_size()); EXPECT_TRUE(header_entries_.empty()); @@ -997,8 +1000,11 @@ hbb.AppendIndexedHeader(5); // Not decoded. EXPECT_FALSE(DecodeBlock(hbb.buffer())); EXPECT_FALSE(saw_end_); + EXPECT_EQ(HpackDecodingError::kDynamicTableSizeUpdateNotAllowed, + decoder_.error()); EXPECT_EQ(1u, error_messages_.size()); - EXPECT_THAT(error_messages_[0], HasSubstr("size update not allowed")); + EXPECT_THAT(error_messages_[0], + Eq("Dynamic table size update not allowed")); EXPECT_EQ(700u, header_table_size_limit()); EXPECT_EQ(0u, current_header_table_size()); EXPECT_TRUE(header_entries_.empty()); @@ -1020,8 +1026,12 @@ DecodeBuffer db(hbb.buffer()); EXPECT_FALSE(decoder_.DecodeFragment(&db)); EXPECT_FALSE(saw_end_); + EXPECT_EQ( + HpackDecodingError::kInitialDynamicTableSizeUpdateIsAboveLowWaterMark, + decoder_.error()); EXPECT_EQ(1u, error_messages_.size()); - EXPECT_THAT(error_messages_[0], HasSubstr("above low water mark")); + EXPECT_THAT(error_messages_[0], + Eq("Initial dynamic table size update is above low water mark")); EXPECT_EQ(Http2SettingsInfo::DefaultHeaderTableSize(), header_table_size_limit()); } @@ -1030,9 +1040,10 @@ TEST_P(HpackDecoderTest, RequiredTableSizeChangeBeforeEnd) { decoder_.ApplyHeaderTableSizeSetting(1024); EXPECT_FALSE(DecodeBlock("")); + EXPECT_EQ(HpackDecodingError::kMissingDynamicTableSizeUpdate, + decoder_.error()); EXPECT_EQ(1u, error_messages_.size()); - EXPECT_THAT(error_messages_[0], - HasSubstr("Missing dynamic table size update")); + EXPECT_THAT(error_messages_[0], Eq("Missing dynamic table size update")); EXPECT_FALSE(saw_end_); } @@ -1043,9 +1054,10 @@ HpackBlockBuilder hbb; hbb.AppendIndexedHeader(1); EXPECT_FALSE(DecodeBlock(hbb.buffer())); + EXPECT_EQ(HpackDecodingError::kMissingDynamicTableSizeUpdate, + decoder_.error()); EXPECT_EQ(1u, error_messages_.size()); - EXPECT_THAT(error_messages_[0], - HasSubstr("Missing dynamic table size update")); + EXPECT_THAT(error_messages_[0], Eq("Missing dynamic table size update")); EXPECT_FALSE(saw_end_); EXPECT_TRUE(header_entries_.empty()); } @@ -1059,9 +1071,10 @@ hbb.AppendNameIndexAndLiteralValue(HpackEntryType::kIndexedLiteralHeader, 2, false, "PUT"); EXPECT_FALSE(DecodeBlock(hbb.buffer())); + EXPECT_EQ(HpackDecodingError::kMissingDynamicTableSizeUpdate, + decoder_.error()); EXPECT_EQ(1u, error_messages_.size()); - EXPECT_THAT(error_messages_[0], - HasSubstr("Missing dynamic table size update")); + EXPECT_THAT(error_messages_[0], Eq("Missing dynamic table size update")); EXPECT_FALSE(saw_end_); EXPECT_TRUE(header_entries_.empty()); } @@ -1074,9 +1087,10 @@ hbb.AppendLiteralNameAndValue(HpackEntryType::kNeverIndexedLiteralHeader, false, "name", false, "some data."); EXPECT_FALSE(DecodeBlock(hbb.buffer())); + EXPECT_EQ(HpackDecodingError::kMissingDynamicTableSizeUpdate, + decoder_.error()); EXPECT_EQ(1u, error_messages_.size()); - EXPECT_THAT(error_messages_[0], - HasSubstr("Missing dynamic table size update")); + EXPECT_THAT(error_messages_[0], Eq("Missing dynamic table size update")); EXPECT_FALSE(saw_end_); EXPECT_TRUE(header_entries_.empty()); } @@ -1090,8 +1104,10 @@ EXPECT_FALSE(decoder_.DecodeFragment(&db)); EXPECT_TRUE(decoder_.DetectError()); EXPECT_FALSE(saw_end_); + EXPECT_EQ(HpackDecodingError::kIndexVarintError, decoder_.error()); EXPECT_EQ(1u, error_messages_.size()); - EXPECT_THAT(error_messages_[0], HasSubstr("malformed")); + EXPECT_THAT(error_messages_[0], + Eq("Index varint beyond implementation limit")); EXPECT_TRUE(header_entries_.empty()); // Now that an error has been detected, EndDecodingBlock should not succeed. EXPECT_FALSE(decoder_.EndDecodingBlock()); @@ -1105,8 +1121,10 @@ EXPECT_FALSE(decoder_.DecodeFragment(&db)); EXPECT_TRUE(decoder_.DetectError()); EXPECT_FALSE(saw_end_); + EXPECT_EQ(HpackDecodingError::kInvalidIndex, decoder_.error()); EXPECT_EQ(1u, error_messages_.size()); - EXPECT_THAT(error_messages_[0], HasSubstr("Invalid index")); + EXPECT_THAT(error_messages_[0], + Eq("Invalid index in indexed header field representation")); EXPECT_TRUE(header_entries_.empty()); // Now that an error has been detected, EndDecodingBlock should not succeed. EXPECT_FALSE(decoder_.EndDecodingBlock()); @@ -1128,8 +1146,10 @@ // But not if the block is truncated. EXPECT_FALSE(DecodeBlock(hbb.buffer().substr(0, hbb.size() - 1))); EXPECT_FALSE(saw_end_); + EXPECT_EQ(HpackDecodingError::kTruncatedBlock, decoder_.error()); EXPECT_EQ(1u, error_messages_.size()); - EXPECT_THAT(error_messages_[0], HasSubstr("truncated")); + EXPECT_THAT(error_messages_[0], + Eq("Block ends in the middle of an instruction")); // The first update was decoded. EXPECT_EQ(3000u, header_table_size_limit()); EXPECT_EQ(0u, current_header_table_size()); @@ -1156,8 +1176,9 @@ EXPECT_THAT(header_entries_, ElementsAreArray({HpackHeaderEntry{"name", "some data."}})); EXPECT_FALSE(saw_end_); + EXPECT_EQ(HpackDecodingError::kValueTooLong, decoder_.error()); EXPECT_EQ(1u, error_messages_.size()); - EXPECT_THAT(error_messages_[0], HasSubstr("too long")); + EXPECT_THAT(error_messages_[0], Eq("Value length exceeds buffer limit")); } } // namespace
diff --git a/http2/hpack/decoder/hpack_decoding_error.cc b/http2/hpack/decoder/hpack_decoding_error.cc new file mode 100644 index 0000000..56eec6c --- /dev/null +++ b/http2/hpack/decoder/hpack_decoding_error.cc
@@ -0,0 +1,47 @@ +// Copyright 2020 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoding_error.h" + +namespace http2 { + +// static +quiche::QuicheStringPiece HpackDecodingErrorToString(HpackDecodingError error) { + switch (error) { + case HpackDecodingError::kOk: + return "No error detected"; + case HpackDecodingError::kIndexVarintError: + return "Index varint beyond implementation limit"; + case HpackDecodingError::kNameLengthVarintError: + return "Name length varint beyond implementation limit"; + case HpackDecodingError::kValueLengthVarintError: + return "Value length varint beyond implementation limit"; + case HpackDecodingError::kNameTooLong: + return "Name length exceeds buffer limit"; + case HpackDecodingError::kValueTooLong: + return "Value length exceeds buffer limit"; + case HpackDecodingError::kNameHuffmanError: + return "Name Huffman encoding error"; + case HpackDecodingError::kValueHuffmanError: + return "Value Huffman encoding error"; + case HpackDecodingError::kMissingDynamicTableSizeUpdate: + return "Missing dynamic table size update"; + case HpackDecodingError::kInvalidIndex: + return "Invalid index in indexed header field representation"; + case HpackDecodingError::kInvalidNameIndex: + return "Invalid index in literal header field with indexed name " + "representation"; + case HpackDecodingError::kDynamicTableSizeUpdateNotAllowed: + return "Dynamic table size update not allowed"; + case HpackDecodingError::kInitialDynamicTableSizeUpdateIsAboveLowWaterMark: + return "Initial dynamic table size update is above low water mark"; + case HpackDecodingError::kDynamicTableSizeUpdateIsAboveAcknowledgedSetting: + return "Dynamic table size update is above acknowledged setting"; + case HpackDecodingError::kTruncatedBlock: + return "Block ends in the middle of an instruction"; + } + return "invalid HpackDecodingError value"; +} + +} // namespace http2
diff --git a/http2/hpack/decoder/hpack_decoding_error.h b/http2/hpack/decoder/hpack_decoding_error.h new file mode 100644 index 0000000..98df064 --- /dev/null +++ b/http2/hpack/decoder/hpack_decoding_error.h
@@ -0,0 +1,47 @@ +// Copyright 2020 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef QUICHE_HTTP2_HPACK_DECODER_HPACK_DECODING_ERROR_H_ +#define QUICHE_HTTP2_HPACK_DECODER_HPACK_DECODING_ERROR_H_ + +#include "net/third_party/quiche/src/common/platform/api/quiche_export.h" +#include "net/third_party/quiche/src/common/platform/api/quiche_string_piece.h" + +namespace http2 { + +enum class HpackDecodingError { + // No error detected so far. + kOk, + // Varint beyond implementation limit. + kIndexVarintError, + kNameLengthVarintError, + kValueLengthVarintError, + // String literal length exceeds buffer limit. + kNameTooLong, + kValueTooLong, + // Error in Huffman encoding. + kNameHuffmanError, + kValueHuffmanError, + // Next instruction should have been a dynamic table size update. + kMissingDynamicTableSizeUpdate, + // Invalid index in indexed header field representation. + kInvalidIndex, + // Invalid index in literal header field with indexed name representation. + kInvalidNameIndex, + // Dynamic table size update not allowed. + kDynamicTableSizeUpdateNotAllowed, + // Initial dynamic table size update is above low water mark. + kInitialDynamicTableSizeUpdateIsAboveLowWaterMark, + // Dynamic table size update is above acknowledged setting. + kDynamicTableSizeUpdateIsAboveAcknowledgedSetting, + // HPACK block ends in the middle of an instruction. + kTruncatedBlock, +}; + +QUICHE_EXPORT_PRIVATE quiche::QuicheStringPiece HpackDecodingErrorToString( + HpackDecodingError error); + +} // namespace http2 + +#endif // QUICHE_HTTP2_HPACK_DECODER_HPACK_DECODING_ERROR_H_
diff --git a/http2/hpack/decoder/hpack_entry_decoder.cc b/http2/hpack/decoder/hpack_entry_decoder.cc index e177728..4e8b4d0 100644 --- a/http2/hpack/decoder/hpack_entry_decoder.cc +++ b/http2/hpack/decoder/hpack_entry_decoder.cc
@@ -80,6 +80,7 @@ return status; case DecodeStatus::kDecodeError: HTTP2_CODE_COUNT_N(decompress_failure_3, 11, 23); + error_ = HpackDecodingError::kIndexVarintError; // The varint must have been invalid (too long). return status; } @@ -104,6 +105,7 @@ status = entry_type_decoder_.Resume(db); if (status == DecodeStatus::kDecodeError) { HTTP2_CODE_COUNT_N(decompress_failure_3, 12, 23); + error_ = HpackDecodingError::kIndexVarintError; } if (status != DecodeStatus::kDecodeDone) { return status; @@ -136,6 +138,7 @@ state_ = EntryDecoderState::kResumeDecodingName; if (status == DecodeStatus::kDecodeError) { HTTP2_CODE_COUNT_N(decompress_failure_3, 13, 23); + error_ = HpackDecodingError::kNameLengthVarintError; } return status; } @@ -151,6 +154,7 @@ } if (status == DecodeStatus::kDecodeError) { HTTP2_CODE_COUNT_N(decompress_failure_3, 14, 23); + error_ = HpackDecodingError::kValueLengthVarintError; } if (status == DecodeStatus::kDecodeDone) { // Done with decoding the literal value, so we've reached the @@ -180,6 +184,7 @@ state_ = EntryDecoderState::kResumeDecodingName; if (status == DecodeStatus::kDecodeError) { HTTP2_CODE_COUNT_N(decompress_failure_3, 15, 23); + error_ = HpackDecodingError::kNameLengthVarintError; } return status; } @@ -196,6 +201,7 @@ } if (status == DecodeStatus::kDecodeError) { HTTP2_CODE_COUNT_N(decompress_failure_3, 16, 23); + error_ = HpackDecodingError::kValueLengthVarintError; } if (status == DecodeStatus::kDecodeDone) { // Done with decoding the value, therefore the entry as a whole.
diff --git a/http2/hpack/decoder/hpack_entry_decoder.h b/http2/hpack/decoder/hpack_entry_decoder.h index 1da3644..3e77254 100644 --- a/http2/hpack/decoder/hpack_entry_decoder.h +++ b/http2/hpack/decoder/hpack_entry_decoder.h
@@ -14,6 +14,7 @@ #include "net/third_party/quiche/src/http2/decoder/decode_buffer.h" #include "net/third_party/quiche/src/http2/decoder/decode_status.h" +#include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoding_error.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_entry_decoder_listener.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_entry_type_decoder.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_string_decoder.h" @@ -63,6 +64,9 @@ // in decoding the entry type and its varint. DecodeStatus Resume(DecodeBuffer* db, HpackEntryDecoderListener* listener); + // Return error code after decoding error occurred. + HpackDecodingError error() const { return error_; } + std::string DebugString() const; void OutputDebugString(std::ostream& out) const; @@ -73,6 +77,7 @@ HpackEntryTypeDecoder entry_type_decoder_; HpackStringDecoder string_decoder_; EntryDecoderState state_ = EntryDecoderState(); + HpackDecodingError error_ = HpackDecodingError::kOk; }; QUICHE_EXPORT_PRIVATE std::ostream& operator<<(std::ostream& out,
diff --git a/http2/hpack/decoder/hpack_whole_entry_buffer.cc b/http2/hpack/decoder/hpack_whole_entry_buffer.cc index 2173756..da5613e 100644 --- a/http2/hpack/decoder/hpack_whole_entry_buffer.cc +++ b/http2/hpack/decoder/hpack_whole_entry_buffer.cc
@@ -58,7 +58,7 @@ if (len > max_string_size_bytes_) { HTTP2_DVLOG(1) << "Name length (" << len << ") is longer than permitted (" << max_string_size_bytes_ << ")"; - ReportError("HPACK entry name size is too long."); + ReportError(HpackDecodingError::kNameTooLong); HTTP2_CODE_COUNT_N(decompress_failure_3, 18, 23); return; } @@ -72,7 +72,7 @@ << Http2HexDump(quiche::QuicheStringPiece(data, len)); DCHECK_EQ(maybe_name_index_, 0u); if (!error_detected_ && !name_.OnData(data, len)) { - ReportError("Error decoding HPACK entry name."); + ReportError(HpackDecodingError::kNameHuffmanError); HTTP2_CODE_COUNT_N(decompress_failure_3, 19, 23); } } @@ -81,7 +81,7 @@ HTTP2_DVLOG(2) << "HpackWholeEntryBuffer::OnNameEnd"; DCHECK_EQ(maybe_name_index_, 0u); if (!error_detected_ && !name_.OnEnd()) { - ReportError("Error decoding HPACK entry name."); + ReportError(HpackDecodingError::kNameHuffmanError); HTTP2_CODE_COUNT_N(decompress_failure_3, 20, 23); } } @@ -94,7 +94,7 @@ HTTP2_DVLOG(1) << "Value length (" << len << ") is longer than permitted (" << max_string_size_bytes_ << ")"; - ReportError("HPACK entry value size is too long."); + ReportError(HpackDecodingError::kValueTooLong); HTTP2_CODE_COUNT_N(decompress_failure_3, 21, 23); return; } @@ -107,7 +107,7 @@ << " data:\n" << Http2HexDump(quiche::QuicheStringPiece(data, len)); if (!error_detected_ && !value_.OnData(data, len)) { - ReportError("Error decoding HPACK entry value."); + ReportError(HpackDecodingError::kValueHuffmanError); HTTP2_CODE_COUNT_N(decompress_failure_3, 22, 23); } } @@ -118,7 +118,7 @@ return; } if (!value_.OnEnd()) { - ReportError("Error decoding HPACK entry value."); + ReportError(HpackDecodingError::kValueHuffmanError); HTTP2_CODE_COUNT_N(decompress_failure_3, 23, 23); return; } @@ -138,12 +138,12 @@ listener_->OnDynamicTableSizeUpdate(size); } -void HpackWholeEntryBuffer::ReportError( - quiche::QuicheStringPiece error_message) { +void HpackWholeEntryBuffer::ReportError(HpackDecodingError error) { if (!error_detected_) { - HTTP2_DVLOG(1) << "HpackWholeEntryBuffer::ReportError: " << error_message; + HTTP2_DVLOG(1) << "HpackWholeEntryBuffer::ReportError: " + << HpackDecodingErrorToString(error); error_detected_ = true; - listener_->OnHpackDecodeError(error_message); + listener_->OnHpackDecodeError(error); listener_ = HpackWholeEntryNoOpListener::NoOpListener(); } }
diff --git a/http2/hpack/decoder/hpack_whole_entry_buffer.h b/http2/hpack/decoder/hpack_whole_entry_buffer.h index fd445c0..5c0bfe2 100644 --- a/http2/hpack/decoder/hpack_whole_entry_buffer.h +++ b/http2/hpack/decoder/hpack_whole_entry_buffer.h
@@ -13,6 +13,7 @@ #include <stddef.h> #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoder_string_buffer.h" +#include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoding_error.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_entry_decoder_listener.h" #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_whole_entry_listener.h" #include "net/third_party/quiche/src/http2/hpack/http2_hpack_constants.h" @@ -79,7 +80,7 @@ void OnDynamicTableSizeUpdate(size_t size) override; private: - void ReportError(quiche::QuicheStringPiece error_message); + void ReportError(HpackDecodingError error); HpackWholeEntryListener* listener_; HpackDecoderStringBuffer name_, value_;
diff --git a/http2/hpack/decoder/hpack_whole_entry_buffer_test.cc b/http2/hpack/decoder/hpack_whole_entry_buffer_test.cc index 4ac82ea..a48afd7 100644 --- a/http2/hpack/decoder/hpack_whole_entry_buffer_test.cc +++ b/http2/hpack/decoder/hpack_whole_entry_buffer_test.cc
@@ -12,7 +12,6 @@ #include "net/third_party/quiche/src/http2/platform/api/http2_test_helpers.h" using ::testing::AllOf; -using ::testing::HasSubstr; using ::testing::InSequence; using ::testing::Property; using ::testing::StrictMock; @@ -37,8 +36,7 @@ HpackDecoderStringBuffer* name_buffer, HpackDecoderStringBuffer* value_buffer)); MOCK_METHOD1(OnDynamicTableSizeUpdate, void(size_t size)); - MOCK_METHOD1(OnHpackDecodeError, - void(quiche::QuicheStringPiece error_message)); + MOCK_METHOD1(OnHpackDecodeError, void(HpackDecodingError error)); }; class HpackWholeEntryBufferTest : public ::testing::Test { @@ -156,14 +154,14 @@ // Verify that a name longer than the allowed size generates an error. TEST_F(HpackWholeEntryBufferTest, NameTooLong) { entry_buffer_.OnStartLiteralHeader(HpackEntryType::kIndexedLiteralHeader, 0); - EXPECT_CALL(listener_, OnHpackDecodeError(HasSubstr("HPACK entry name"))); + EXPECT_CALL(listener_, OnHpackDecodeError(HpackDecodingError::kNameTooLong)); entry_buffer_.OnNameStart(false, kMaxStringSize + 1); } // Verify that a name longer than the allowed size generates an error. TEST_F(HpackWholeEntryBufferTest, ValueTooLong) { entry_buffer_.OnStartLiteralHeader(HpackEntryType::kIndexedLiteralHeader, 1); - EXPECT_CALL(listener_, OnHpackDecodeError(HasSubstr("HPACK entry value"))); + EXPECT_CALL(listener_, OnHpackDecodeError(HpackDecodingError::kValueTooLong)); entry_buffer_.OnValueStart(false, kMaxStringSize + 1); } @@ -176,7 +174,8 @@ entry_buffer_.OnNameStart(true, 4); entry_buffer_.OnNameData(data, 3); - EXPECT_CALL(listener_, OnHpackDecodeError(HasSubstr("HPACK entry name"))); + EXPECT_CALL(listener_, + OnHpackDecodeError(HpackDecodingError::kNameHuffmanError)); entry_buffer_.OnNameData(data, 1); @@ -187,14 +186,15 @@ // Verify that a Huffman encoded value that isn't properly terminated with // a partial EOS symbol generates an error. -TEST_F(HpackWholeEntryBufferTest, ValueeHuffmanError) { +TEST_F(HpackWholeEntryBufferTest, ValueHuffmanError) { const char data[] = "\x00\x00\x00"; entry_buffer_.OnStartLiteralHeader(HpackEntryType::kNeverIndexedLiteralHeader, 61); entry_buffer_.OnValueStart(true, 3); entry_buffer_.OnValueData(data, 3); - EXPECT_CALL(listener_, OnHpackDecodeError(HasSubstr("HPACK entry value"))); + EXPECT_CALL(listener_, + OnHpackDecodeError(HpackDecodingError::kValueHuffmanError)); entry_buffer_.OnValueEnd();
diff --git a/http2/hpack/decoder/hpack_whole_entry_listener.cc b/http2/hpack/decoder/hpack_whole_entry_listener.cc index 69cf122..5003f11 100644 --- a/http2/hpack/decoder/hpack_whole_entry_listener.cc +++ b/http2/hpack/decoder/hpack_whole_entry_listener.cc
@@ -20,8 +20,8 @@ HpackDecoderStringBuffer* name_buffer, HpackDecoderStringBuffer* value_buffer) {} void HpackWholeEntryNoOpListener::OnDynamicTableSizeUpdate(size_t size) {} -void HpackWholeEntryNoOpListener::OnHpackDecodeError( - quiche::QuicheStringPiece error_message) {} +void HpackWholeEntryNoOpListener::OnHpackDecodeError(HpackDecodingError error) { +} // static HpackWholeEntryNoOpListener* HpackWholeEntryNoOpListener::NoOpListener() {
diff --git a/http2/hpack/decoder/hpack_whole_entry_listener.h b/http2/hpack/decoder/hpack_whole_entry_listener.h index 86e8e7f..ef2f77e 100644 --- a/http2/hpack/decoder/hpack_whole_entry_listener.h +++ b/http2/hpack/decoder/hpack_whole_entry_listener.h
@@ -12,6 +12,7 @@ #include <stddef.h> #include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoder_string_buffer.h" +#include "net/third_party/quiche/src/http2/hpack/decoder/hpack_decoding_error.h" #include "net/third_party/quiche/src/http2/hpack/http2_hpack_constants.h" #include "net/third_party/quiche/src/common/platform/api/quiche_export.h" #include "net/third_party/quiche/src/common/platform/api/quiche_string_piece.h" @@ -50,8 +51,7 @@ virtual void OnDynamicTableSizeUpdate(size_t size) = 0; // OnHpackDecodeError is called if an error is detected while decoding. - // error_message may be used in a GOAWAY frame as the Opaque Data. - virtual void OnHpackDecodeError(quiche::QuicheStringPiece error_message) = 0; + virtual void OnHpackDecodeError(HpackDecodingError error) = 0; }; // A no-op implementation of HpackWholeEntryDecoderListener, useful for ignoring @@ -69,7 +69,7 @@ HpackDecoderStringBuffer* name_buffer, HpackDecoderStringBuffer* value_buffer) override; void OnDynamicTableSizeUpdate(size_t size) override; - void OnHpackDecodeError(quiche::QuicheStringPiece error_message) override; + void OnHpackDecodeError(HpackDecodingError error) override; // Returns a listener that ignores all the calls. static HpackWholeEntryNoOpListener* NoOpListener();