Fix use-after-free in QpackProgressiveDecoder and QpackInstructionDecoder.
I locally verified that this fixes both https://crbug.com/1025158 and
https://crbug.com/1025209.
QpackDecodedHeadersAccumulator and QuicSpdyStream had a secret conference at
cl/280327626 where they agreed that QpackDecodedHeadersAccumulator will support
being destroyed by the handler and QuicSpdyStream can get much cleaner by
relying on it. However, they forgot to tell QpackProgressiveDecoder and
QpackInstructionDecoder, who are now quite grumpy at https://crbug.com/1025209.
It is clear that these four do not talk, otherwise how could it happen that
QuicSpdyStream is the Visitor of QpackDecodedHeadersAccumulator,
QpackDecodedHeadersAccumulator is the Handler of QpackProgressiveDecoder,
and QpackProgressiveDecoder is the Delegate of QpackInstructionDecoder,
three different design pattern names for almost identical relationships.
gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99.
PiperOrigin-RevId: 281188599
Change-Id: I63b5612c142e7f1fbe0bf05eb32ddf68457b4bc3
diff --git a/quic/core/qpack/qpack_decoder_test.cc b/quic/core/qpack/qpack_decoder_test.cc
index 323a893..1fc5802 100644
--- a/quic/core/qpack/qpack_decoder_test.cc
+++ b/quic/core/qpack/qpack_decoder_test.cc
@@ -13,7 +13,9 @@
#include "net/third_party/quiche/src/quic/test_tools/qpack/qpack_test_utils.h"
#include "net/third_party/quiche/src/spdy/core/spdy_header_block.h"
+using ::testing::_;
using ::testing::Eq;
+using ::testing::Invoke;
using ::testing::Mock;
using ::testing::Sequence;
using ::testing::StrictMock;
@@ -42,6 +44,15 @@
~QpackDecoderTest() override = default;
+ void SetUp() override {
+ // Destroy QpackProgressiveDecoder on error to test that it does not crash.
+ // See https://crbug.com/1025209.
+ ON_CALL(handler_, OnDecodingErrorDetected(_))
+ .WillByDefault(Invoke([this](QuicStringPiece /* error_message */) {
+ progressive_decoder_.reset();
+ }));
+ }
+
void DecodeEncoderStreamData(QuicStringPiece data) {
qpack_decoder_.encoder_stream_receiver()->Decode(data);
}
@@ -61,7 +72,7 @@
void DecodeData(QuicStringPiece data) {
auto fragment_size_generator =
FragmentModeToFragmentSizeGenerator(fragment_mode_);
- while (!data.empty()) {
+ while (progressive_decoder_ && !data.empty()) {
size_t fragment_size = std::min(fragment_size_generator(), data.size());
progressive_decoder_->Decode(data.substr(0, fragment_size));
data = data.substr(fragment_size);
@@ -70,9 +81,11 @@
// Signal end of header block to QpackProgressiveDecoder.
void EndDecoding() {
- progressive_decoder_->EndHeaderBlock();
- // |progressive_decoder_| is kept alive so that it can
- // handle callbacks later in case of blocked decoding.
+ if (progressive_decoder_) {
+ progressive_decoder_->EndHeaderBlock();
+ }
+ // If no error was detected, |*progressive_decoder_| is kept alive so that
+ // it can handle callbacks later in case of blocked decoding.
}
// Decode an entire header block.
@@ -105,6 +118,18 @@
DecodeHeaderBlock(QuicTextUtils::HexDecode("00"));
}
+// Regression test for https://1025209: QpackProgressiveDecoder must not crash
+// in Decode() if it is destroyed by handler_.OnDecodingErrorDetected().
+TEST_P(QpackDecoderTest, InvalidPrefix) {
+ StartDecoding();
+
+ EXPECT_CALL(handler_,
+ OnDecodingErrorDetected(Eq("Encoded integer too large.")));
+
+ // Encoded Required Insert Count in Header Data Prefix is too large.
+ DecodeData(QuicTextUtils::HexDecode("ffffffffffffffffffffffffffff"));
+}
+
TEST_P(QpackDecoderTest, EmptyHeaderBlock) {
EXPECT_CALL(handler_, OnDecodingCompleted());
