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();
