Surface hpack decoder detailed error for header value too long, and put detailed error to quic connection close error details. only change connection close error detail, not protected. PiperOrigin-RevId: 323008871 Change-Id: I6f41118464ae686fbfeb009c8ac13d83b8fff3f0
diff --git a/http2/hpack/decoder/hpack_decoder.cc b/http2/hpack/decoder/hpack_decoder.cc index b879511..6cfd1e0 100644 --- a/http2/hpack/decoder/hpack_decoder.cc +++ b/http2/hpack/decoder/hpack_decoder.cc
@@ -60,7 +60,7 @@ // which finally forwards them to the HpackDecoderListener. DecodeStatus status = block_decoder_.Decode(db); if (status == DecodeStatus::kDecodeError) { - ReportError(block_decoder_.error()); + ReportError(block_decoder_.error(), ""); HTTP2_CODE_COUNT_N(decompress_failure_3, 4, 23); return false; } else if (DetectError()) { @@ -85,7 +85,7 @@ } if (!block_decoder_.before_entry()) { // The HPACK block ended in the middle of an entry. - ReportError(HpackDecodingError::kTruncatedBlock); + ReportError(HpackDecodingError::kTruncatedBlock, ""); HTTP2_CODE_COUNT_N(decompress_failure_3, 7, 23); return false; } @@ -107,6 +107,7 @@ HTTP2_DVLOG(2) << "Error detected in decoder_state_"; HTTP2_CODE_COUNT_N(decompress_failure_3, 10, 23); error_ = decoder_state_.error(); + detailed_error_ = decoder_state_.detailed_error(); } return error_ != HpackDecodingError::kOk; @@ -116,12 +117,14 @@ return Http2EstimateMemoryUsage(entry_buffer_); } -void HpackDecoder::ReportError(HpackDecodingError error) { +void HpackDecoder::ReportError(HpackDecodingError error, + std::string detailed_error) { HTTP2_DVLOG(3) << "HpackDecoder::ReportError is new=" << (error_ == HpackDecodingError::kOk ? "true" : "false") << ", error: " << HpackDecodingErrorToString(error); if (error_ == HpackDecodingError::kOk) { error_ = error; + detailed_error_ = detailed_error; decoder_state_.listener()->OnHeaderErrorDetected( HpackDecodingErrorToString(error)); }
diff --git a/http2/hpack/decoder/hpack_decoder.h b/http2/hpack/decoder/hpack_decoder.h index efe334d..0666c38 100644 --- a/http2/hpack/decoder/hpack_decoder.h +++ b/http2/hpack/decoder/hpack_decoder.h
@@ -103,11 +103,13 @@ // Returns the estimate of dynamically allocated memory in bytes. size_t EstimateMemoryUsage() const; + std::string detailed_error() const { return detailed_error_; } + private: friend class test::HpackDecoderPeer; // Reports an error to the listener IF this is the first error detected. - void ReportError(HpackDecodingError error); + void ReportError(HpackDecodingError error, std::string detailed_error); // The decompressor state, as defined by HPACK (i.e. the static and dynamic // tables). @@ -121,6 +123,7 @@ // Error code if an error has occurred, HpackDecodingError::kOk otherwise. HpackDecodingError error_; + std::string detailed_error_; }; } // namespace http2
diff --git a/http2/hpack/decoder/hpack_decoder_state.cc b/http2/hpack/decoder/hpack_decoder_state.cc index 09b64c0..eba3e66 100644 --- a/http2/hpack/decoder/hpack_decoder_state.cc +++ b/http2/hpack/decoder/hpack_decoder_state.cc
@@ -85,7 +85,7 @@ return; } if (require_dynamic_table_size_update_) { - ReportError(HpackDecodingError::kMissingDynamicTableSizeUpdate); + ReportError(HpackDecodingError::kMissingDynamicTableSizeUpdate, ""); return; } allow_dynamic_table_size_update_ = false; @@ -93,7 +93,7 @@ if (entry != nullptr) { listener_->OnHeader(entry->name, entry->value); } else { - ReportError(HpackDecodingError::kInvalidIndex); + ReportError(HpackDecodingError::kInvalidIndex, ""); } } @@ -108,7 +108,7 @@ return; } if (require_dynamic_table_size_update_) { - ReportError(HpackDecodingError::kMissingDynamicTableSizeUpdate); + ReportError(HpackDecodingError::kMissingDynamicTableSizeUpdate, ""); return; } allow_dynamic_table_size_update_ = false; @@ -120,7 +120,7 @@ decoder_tables_.Insert(entry->name, value); } } else { - ReportError(HpackDecodingError::kInvalidNameIndex); + ReportError(HpackDecodingError::kInvalidNameIndex, ""); } } @@ -134,7 +134,7 @@ return; } if (require_dynamic_table_size_update_) { - ReportError(HpackDecodingError::kMissingDynamicTableSizeUpdate); + ReportError(HpackDecodingError::kMissingDynamicTableSizeUpdate, ""); return; } allow_dynamic_table_size_update_ = false; @@ -159,14 +159,15 @@ if (!allow_dynamic_table_size_update_) { // At most two dynamic table size updates allowed at the start, and not // after a header. - ReportError(HpackDecodingError::kDynamicTableSizeUpdateNotAllowed); + 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(HpackDecodingError:: - kInitialDynamicTableSizeUpdateIsAboveLowWaterMark); + ReportError( + HpackDecodingError::kInitialDynamicTableSizeUpdateIsAboveLowWaterMark, + ""); return; } require_dynamic_table_size_update_ = false; @@ -174,7 +175,8 @@ // The new size must not be greater than the final max header table size // that the peer acknowledged. ReportError( - HpackDecodingError::kDynamicTableSizeUpdateIsAboveAcknowledgedSetting); + HpackDecodingError::kDynamicTableSizeUpdateIsAboveAcknowledgedSetting, + ""); return; } decoder_tables_.DynamicTableSizeUpdate(size_limit); @@ -187,11 +189,12 @@ lowest_header_table_size_ = final_header_table_size_; } -void HpackDecoderState::OnHpackDecodeError(HpackDecodingError error) { +void HpackDecoderState::OnHpackDecodeError(HpackDecodingError error, + std::string detailed_error) { HTTP2_DVLOG(2) << "HpackDecoderState::OnHpackDecodeError " << HpackDecodingErrorToString(error); if (error_ == HpackDecodingError::kOk) { - ReportError(error); + ReportError(error, detailed_error); } } @@ -203,19 +206,21 @@ 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(HpackDecodingError::kMissingDynamicTableSizeUpdate); + ReportError(HpackDecodingError::kMissingDynamicTableSizeUpdate, ""); } else { listener_->OnHeaderListEnd(); } } -void HpackDecoderState::ReportError(HpackDecodingError error) { +void HpackDecoderState::ReportError(HpackDecodingError error, + std::string detailed_error) { HTTP2_DVLOG(2) << "HpackDecoderState::ReportError is new=" << (error_ == HpackDecodingError::kOk ? "true" : "false") << ", error: " << HpackDecodingErrorToString(error); if (error_ == HpackDecodingError::kOk) { listener_->OnHeaderErrorDetected(HpackDecodingErrorToString(error)); error_ = error; + detailed_error_ = detailed_error; } }
diff --git a/http2/hpack/decoder/hpack_decoder_state.h b/http2/hpack/decoder/hpack_decoder_state.h index 0735f6a..3854a6c 100644 --- a/http2/hpack/decoder/hpack_decoder_state.h +++ b/http2/hpack/decoder/hpack_decoder_state.h
@@ -77,7 +77,8 @@ HpackDecoderStringBuffer* name_buffer, HpackDecoderStringBuffer* value_buffer) override; void OnDynamicTableSizeUpdate(size_t size) override; - void OnHpackDecodeError(HpackDecodingError error) override; + void OnHpackDecodeError(HpackDecodingError error, + std::string detailed_error) override; // OnHeaderBlockEnd notifies this object that an entire HPACK block has been // decoded, which might have extended into CONTINUATION blocks. @@ -91,11 +92,13 @@ return decoder_tables_; } + std::string detailed_error() const { return detailed_error_; } + private: friend class test::HpackDecoderStatePeer; // Reports an error to the listener IF this is the first error detected. - void ReportError(HpackDecodingError error); + void ReportError(HpackDecodingError error, std::string detailed_error); // The static and dynamic HPACK tables. HpackDecoderTables decoder_tables_; @@ -123,6 +126,7 @@ // Has an error already been detected and reported to the listener? HpackDecodingError error_; + std::string detailed_error_; }; } // namespace http2
diff --git a/http2/hpack/decoder/hpack_decoder_state_test.cc b/http2/hpack/decoder/hpack_decoder_state_test.cc index 02abb55..41a8453 100644 --- a/http2/hpack/decoder/hpack_decoder_state_test.cc +++ b/http2/hpack/decoder/hpack_decoder_state_test.cc
@@ -455,7 +455,7 @@ decoder_state_.OnLiteralNameAndValue(HpackEntryType::kIndexedLiteralHeader, &name_buffer_, &value_buffer_); decoder_state_.OnHeaderBlockEnd(); - decoder_state_.OnHpackDecodeError(HpackDecodingError::kIndexVarintError); + decoder_state_.OnHpackDecodeError(HpackDecodingError::kIndexVarintError, ""); } // Confirm that required size updates are validated. @@ -524,7 +524,7 @@ SendStartAndVerifyCallback(); EXPECT_CALL(listener_, OnHeaderErrorDetected(Eq("Name Huffman encoding error"))); - decoder_state_.OnHpackDecodeError(HpackDecodingError::kNameHuffmanError); + decoder_state_.OnHpackDecodeError(HpackDecodingError::kNameHuffmanError, ""); // Further decoded entries are ignored. decoder_state_.OnIndexedHeader(1); @@ -536,7 +536,7 @@ decoder_state_.OnLiteralNameAndValue(HpackEntryType::kIndexedLiteralHeader, &name_buffer_, &value_buffer_); decoder_state_.OnHeaderBlockEnd(); - decoder_state_.OnHpackDecodeError(HpackDecodingError::kIndexVarintError); + decoder_state_.OnHpackDecodeError(HpackDecodingError::kIndexVarintError, ""); } } // namespace
diff --git a/http2/hpack/decoder/hpack_whole_entry_buffer.cc b/http2/hpack/decoder/hpack_whole_entry_buffer.cc index da5613e..dc93d1e 100644 --- a/http2/hpack/decoder/hpack_whole_entry_buffer.cc +++ b/http2/hpack/decoder/hpack_whole_entry_buffer.cc
@@ -9,6 +9,7 @@ #include "net/third_party/quiche/src/http2/platform/api/http2_logging.h" #include "net/third_party/quiche/src/http2/platform/api/http2_macros.h" #include "net/third_party/quiche/src/http2/platform/api/http2_string_utils.h" +#include "net/third_party/quiche/src/common/platform/api/quiche_str_cat.h" namespace http2 { @@ -58,7 +59,7 @@ if (len > max_string_size_bytes_) { HTTP2_DVLOG(1) << "Name length (" << len << ") is longer than permitted (" << max_string_size_bytes_ << ")"; - ReportError(HpackDecodingError::kNameTooLong); + ReportError(HpackDecodingError::kNameTooLong, ""); HTTP2_CODE_COUNT_N(decompress_failure_3, 18, 23); return; } @@ -72,7 +73,7 @@ << Http2HexDump(quiche::QuicheStringPiece(data, len)); DCHECK_EQ(maybe_name_index_, 0u); if (!error_detected_ && !name_.OnData(data, len)) { - ReportError(HpackDecodingError::kNameHuffmanError); + ReportError(HpackDecodingError::kNameHuffmanError, ""); HTTP2_CODE_COUNT_N(decompress_failure_3, 19, 23); } } @@ -81,7 +82,7 @@ HTTP2_DVLOG(2) << "HpackWholeEntryBuffer::OnNameEnd"; DCHECK_EQ(maybe_name_index_, 0u); if (!error_detected_ && !name_.OnEnd()) { - ReportError(HpackDecodingError::kNameHuffmanError); + ReportError(HpackDecodingError::kNameHuffmanError, ""); HTTP2_CODE_COUNT_N(decompress_failure_3, 20, 23); } } @@ -91,10 +92,11 @@ << (huffman_encoded ? "true" : "false") << ", len=" << len; if (!error_detected_) { if (len > max_string_size_bytes_) { - HTTP2_DVLOG(1) << "Value length (" << len - << ") is longer than permitted (" << max_string_size_bytes_ - << ")"; - ReportError(HpackDecodingError::kValueTooLong); + std::string detailed_error = quiche::QuicheStrCat( + "Value length (", len, ") of [", name_.str(), + "] is longer than permitted (", max_string_size_bytes_, ")"); + HTTP2_DVLOG(1) << detailed_error; + ReportError(HpackDecodingError::kValueTooLong, detailed_error); HTTP2_CODE_COUNT_N(decompress_failure_3, 21, 23); return; } @@ -107,7 +109,7 @@ << " data:\n" << Http2HexDump(quiche::QuicheStringPiece(data, len)); if (!error_detected_ && !value_.OnData(data, len)) { - ReportError(HpackDecodingError::kValueHuffmanError); + ReportError(HpackDecodingError::kValueHuffmanError, ""); HTTP2_CODE_COUNT_N(decompress_failure_3, 22, 23); } } @@ -118,7 +120,7 @@ return; } if (!value_.OnEnd()) { - ReportError(HpackDecodingError::kValueHuffmanError); + ReportError(HpackDecodingError::kValueHuffmanError, ""); HTTP2_CODE_COUNT_N(decompress_failure_3, 23, 23); return; } @@ -138,12 +140,13 @@ listener_->OnDynamicTableSizeUpdate(size); } -void HpackWholeEntryBuffer::ReportError(HpackDecodingError error) { +void HpackWholeEntryBuffer::ReportError(HpackDecodingError error, + std::string detailed_error) { if (!error_detected_) { HTTP2_DVLOG(1) << "HpackWholeEntryBuffer::ReportError: " << HpackDecodingErrorToString(error); error_detected_ = true; - listener_->OnHpackDecodeError(error); + listener_->OnHpackDecodeError(error, detailed_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 5c0bfe2..afca3b8 100644 --- a/http2/hpack/decoder/hpack_whole_entry_buffer.h +++ b/http2/hpack/decoder/hpack_whole_entry_buffer.h
@@ -80,7 +80,7 @@ void OnDynamicTableSizeUpdate(size_t size) override; private: - void ReportError(HpackDecodingError error); + void ReportError(HpackDecodingError error, std::string detailed_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 a48afd7..2396bd6 100644 --- a/http2/hpack/decoder/hpack_whole_entry_buffer_test.cc +++ b/http2/hpack/decoder/hpack_whole_entry_buffer_test.cc
@@ -11,6 +11,7 @@ #include "testing/gtest/include/gtest/gtest.h" #include "net/third_party/quiche/src/http2/platform/api/http2_test_helpers.h" +using ::testing::_; using ::testing::AllOf; using ::testing::InSequence; using ::testing::Property; @@ -26,17 +27,24 @@ public: ~MockHpackWholeEntryListener() override = default; - MOCK_METHOD1(OnIndexedHeader, void(size_t index)); - MOCK_METHOD3(OnNameIndexAndLiteralValue, - void(HpackEntryType entry_type, - size_t name_index, - HpackDecoderStringBuffer* value_buffer)); - MOCK_METHOD3(OnLiteralNameAndValue, - void(HpackEntryType entry_type, - HpackDecoderStringBuffer* name_buffer, - HpackDecoderStringBuffer* value_buffer)); - MOCK_METHOD1(OnDynamicTableSizeUpdate, void(size_t size)); - MOCK_METHOD1(OnHpackDecodeError, void(HpackDecodingError error)); + MOCK_METHOD(void, OnIndexedHeader, (size_t index), (override)); + MOCK_METHOD(void, + OnNameIndexAndLiteralValue, + (HpackEntryType entry_type, + size_t name_index, + HpackDecoderStringBuffer* value_buffer), + (override)); + MOCK_METHOD(void, + OnLiteralNameAndValue, + (HpackEntryType entry_type, + HpackDecoderStringBuffer* name_buffer, + HpackDecoderStringBuffer* value_buffer), + (override)); + MOCK_METHOD(void, OnDynamicTableSizeUpdate, (size_t size), (override)); + MOCK_METHOD(void, + OnHpackDecodeError, + (HpackDecodingError error, std::string detailed_error), + (override)); }; class HpackWholeEntryBufferTest : public ::testing::Test { @@ -154,14 +162,21 @@ // 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(HpackDecodingError::kNameTooLong)); + EXPECT_CALL(listener_, + OnHpackDecodeError(HpackDecodingError::kNameTooLong, _)); entry_buffer_.OnNameStart(false, kMaxStringSize + 1); } -// Verify that a name longer than the allowed size generates an error. +// Verify that a value longer than the allowed size generates an error. TEST_F(HpackWholeEntryBufferTest, ValueTooLong) { - entry_buffer_.OnStartLiteralHeader(HpackEntryType::kIndexedLiteralHeader, 1); - EXPECT_CALL(listener_, OnHpackDecodeError(HpackDecodingError::kValueTooLong)); + entry_buffer_.OnStartLiteralHeader(HpackEntryType::kIndexedLiteralHeader, 0); + EXPECT_CALL(listener_, + OnHpackDecodeError( + HpackDecodingError::kValueTooLong, + "Value length (21) of [path] is longer than permitted (20)")); + entry_buffer_.OnNameStart(false, 4); + entry_buffer_.OnNameData("path", 4); + entry_buffer_.OnNameEnd(); entry_buffer_.OnValueStart(false, kMaxStringSize + 1); } @@ -175,7 +190,7 @@ entry_buffer_.OnNameData(data, 3); EXPECT_CALL(listener_, - OnHpackDecodeError(HpackDecodingError::kNameHuffmanError)); + OnHpackDecodeError(HpackDecodingError::kNameHuffmanError, _)); entry_buffer_.OnNameData(data, 1); @@ -194,7 +209,7 @@ entry_buffer_.OnValueData(data, 3); EXPECT_CALL(listener_, - OnHpackDecodeError(HpackDecodingError::kValueHuffmanError)); + 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 02f3a7a..5900b6d 100644 --- a/http2/hpack/decoder/hpack_whole_entry_listener.cc +++ b/http2/hpack/decoder/hpack_whole_entry_listener.cc
@@ -21,7 +21,8 @@ HpackDecoderStringBuffer* /*value_buffer*/) {} void HpackWholeEntryNoOpListener::OnDynamicTableSizeUpdate(size_t /*size*/) {} void HpackWholeEntryNoOpListener::OnHpackDecodeError( - HpackDecodingError /*error*/) {} + HpackDecodingError /*error*/, + std::string /*detailed_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 ef2f77e..ec2ae52 100644 --- a/http2/hpack/decoder/hpack_whole_entry_listener.h +++ b/http2/hpack/decoder/hpack_whole_entry_listener.h
@@ -51,7 +51,8 @@ virtual void OnDynamicTableSizeUpdate(size_t size) = 0; // OnHpackDecodeError is called if an error is detected while decoding. - virtual void OnHpackDecodeError(HpackDecodingError error) = 0; + virtual void OnHpackDecodeError(HpackDecodingError error, + std::string detailed_error) = 0; }; // A no-op implementation of HpackWholeEntryDecoderListener, useful for ignoring @@ -69,7 +70,8 @@ HpackDecoderStringBuffer* name_buffer, HpackDecoderStringBuffer* value_buffer) override; void OnDynamicTableSizeUpdate(size_t size) override; - void OnHpackDecodeError(HpackDecodingError error) override; + void OnHpackDecodeError(HpackDecodingError error, + std::string detailed_error) override; // Returns a listener that ignores all the calls. static HpackWholeEntryNoOpListener* NoOpListener();