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/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc
index af52992..f6ae7f5 100644
--- a/quic/core/http/quic_spdy_stream_test.cc
+++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -2004,6 +2004,35 @@
session_->qpack_decoder()->OnInsertWithoutNameReference("foo", "bar");
}
+// Regression test for https://crbug.com/1024263 and for
+// https://crbug.com/1025209#c11.
+TEST_P(QuicSpdyStreamTest, BlockedHeaderDecodingUnblockedWithBufferedError) {
+ if (!UsesHttp3()) {
+ return;
+ }
+
+ Initialize(kShouldProcessData);
+ session_->qpack_decoder()->OnSetDynamicTableCapacity(1024);
+
+ // Relative index 2 is invalid because it is larger than or equal to the Base.
+ std::string headers = HeadersFrame(QuicTextUtils::HexDecode("020082"));
+ stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), false, 0, headers));
+
+ // Decoding is blocked.
+ EXPECT_FALSE(stream_->headers_decompressed());
+
+ EXPECT_CALL(
+ *connection_,
+ CloseConnection(QUIC_QPACK_DECOMPRESSION_FAILED,
+ MatchesRegex("Error decoding headers on stream \\d+: "
+ "Invalid relative index."),
+ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET));
+
+ // Deliver one dynamic table entry to decoder
+ // to trigger decoding of header block.
+ session_->qpack_decoder()->OnInsertWithoutNameReference("foo", "bar");
+}
+
TEST_P(QuicSpdyStreamTest, AsyncErrorDecodingTrailers) {
if (!UsesHttp3()) {
return;
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());
diff --git a/quic/core/qpack/qpack_instruction_decoder.cc b/quic/core/qpack/qpack_instruction_decoder.cc
index 187894d..fede8e3 100644
--- a/quic/core/qpack/qpack_instruction_decoder.cc
+++ b/quic/core/qpack/qpack_instruction_decoder.cc
@@ -32,44 +32,48 @@
error_detected_(false),
state_(State::kStartInstruction) {}
-void QpackInstructionDecoder::Decode(QuicStringPiece data) {
+bool QpackInstructionDecoder::Decode(QuicStringPiece data) {
DCHECK(!data.empty());
DCHECK(!error_detected_);
while (true) {
+ bool success = true;
size_t bytes_consumed = 0;
switch (state_) {
case State::kStartInstruction:
- DoStartInstruction(data);
+ success = DoStartInstruction(data);
break;
case State::kStartField:
- DoStartField();
+ success = DoStartField();
break;
case State::kReadBit:
- DoReadBit(data);
+ success = DoReadBit(data);
break;
case State::kVarintStart:
- bytes_consumed = DoVarintStart(data);
+ success = DoVarintStart(data, &bytes_consumed);
break;
case State::kVarintResume:
- bytes_consumed = DoVarintResume(data);
+ success = DoVarintResume(data, &bytes_consumed);
break;
case State::kVarintDone:
- DoVarintDone();
+ success = DoVarintDone();
break;
case State::kReadString:
- bytes_consumed = DoReadString(data);
+ success = DoReadString(data, &bytes_consumed);
break;
case State::kReadStringDone:
- DoReadStringDone();
+ success = DoReadStringDone();
break;
}
- if (error_detected_) {
- return;
+ if (!success) {
+ return false;
}
+ // |success| must be false if an error is detected.
+ DCHECK(!error_detected_);
+
DCHECK_LE(bytes_consumed, data.size());
data = QuicStringPiece(data.data() + bytes_consumed,
@@ -78,35 +82,37 @@
// Stop processing if no more data but next state would require it.
if (data.empty() && (state_ != State::kStartField) &&
(state_ != State::kVarintDone) && (state_ != State::kReadStringDone)) {
- return;
+ return true;
}
}
+
+ return true;
}
bool QpackInstructionDecoder::AtInstructionBoundary() const {
return state_ == State::kStartInstruction;
}
-void QpackInstructionDecoder::DoStartInstruction(QuicStringPiece data) {
+bool QpackInstructionDecoder::DoStartInstruction(QuicStringPiece data) {
DCHECK(!data.empty());
instruction_ = LookupOpcode(data[0]);
field_ = instruction_->fields.begin();
state_ = State::kStartField;
+ return true;
}
-void QpackInstructionDecoder::DoStartField() {
+bool QpackInstructionDecoder::DoStartField() {
if (field_ == instruction_->fields.end()) {
// Completed decoding this instruction.
if (!delegate_->OnInstructionDecoded(instruction_)) {
- error_detected_ = true;
- return;
+ return false;
}
state_ = State::kStartInstruction;
- return;
+ return true;
}
switch (field_->type) {
@@ -114,15 +120,18 @@
case QpackInstructionFieldType::kName:
case QpackInstructionFieldType::kValue:
state_ = State::kReadBit;
- return;
+ return true;
case QpackInstructionFieldType::kVarint:
case QpackInstructionFieldType::kVarint2:
state_ = State::kVarintStart;
- return;
+ return true;
+ default:
+ QUIC_BUG << "Invalid field type.";
+ return false;
}
}
-void QpackInstructionDecoder::DoReadBit(QuicStringPiece data) {
+bool QpackInstructionDecoder::DoReadBit(QuicStringPiece data) {
DCHECK(!data.empty());
switch (field_->type) {
@@ -133,7 +142,7 @@
++field_;
state_ = State::kStartField;
- return;
+ return true;
}
case QpackInstructionFieldType::kName:
case QpackInstructionFieldType::kValue: {
@@ -144,14 +153,16 @@
state_ = State::kVarintStart;
- return;
+ return true;
}
default:
- DCHECK(false);
+ QUIC_BUG << "Invalid field type.";
+ return false;
}
}
-size_t QpackInstructionDecoder::DoVarintStart(QuicStringPiece data) {
+bool QpackInstructionDecoder::DoVarintStart(QuicStringPiece data,
+ size_t* bytes_consumed) {
DCHECK(!data.empty());
DCHECK(field_->type == QpackInstructionFieldType::kVarint ||
field_->type == QpackInstructionFieldType::kVarint2 ||
@@ -162,24 +173,25 @@
http2::DecodeStatus status =
varint_decoder_.Start(data[0], field_->param, &buffer);
- size_t bytes_consumed = 1 + buffer.Offset();
+ *bytes_consumed = 1 + buffer.Offset();
switch (status) {
case http2::DecodeStatus::kDecodeDone:
state_ = State::kVarintDone;
- return bytes_consumed;
+ return true;
case http2::DecodeStatus::kDecodeInProgress:
state_ = State::kVarintResume;
- return bytes_consumed;
+ return true;
case http2::DecodeStatus::kDecodeError:
OnError("Encoded integer too large.");
- return bytes_consumed;
+ return false;
default:
QUIC_BUG << "Unknown decode status " << status;
- return bytes_consumed;
+ return false;
}
}
-size_t QpackInstructionDecoder::DoVarintResume(QuicStringPiece data) {
+bool QpackInstructionDecoder::DoVarintResume(QuicStringPiece data,
+ size_t* bytes_consumed) {
DCHECK(!data.empty());
DCHECK(field_->type == QpackInstructionFieldType::kVarint ||
field_->type == QpackInstructionFieldType::kVarint2 ||
@@ -189,25 +201,25 @@
http2::DecodeBuffer buffer(data);
http2::DecodeStatus status = varint_decoder_.Resume(&buffer);
- size_t bytes_consumed = buffer.Offset();
+ *bytes_consumed = buffer.Offset();
switch (status) {
case http2::DecodeStatus::kDecodeDone:
state_ = State::kVarintDone;
- return bytes_consumed;
+ return true;
case http2::DecodeStatus::kDecodeInProgress:
- DCHECK_EQ(bytes_consumed, data.size());
+ DCHECK_EQ(*bytes_consumed, data.size());
DCHECK(buffer.Empty());
- return bytes_consumed;
+ return true;
case http2::DecodeStatus::kDecodeError:
OnError("Encoded integer too large.");
- return bytes_consumed;
+ return false;
default:
QUIC_BUG << "Unknown decode status " << status;
- return bytes_consumed;
+ return false;
}
}
-void QpackInstructionDecoder::DoVarintDone() {
+bool QpackInstructionDecoder::DoVarintDone() {
DCHECK(field_->type == QpackInstructionFieldType::kVarint ||
field_->type == QpackInstructionFieldType::kVarint2 ||
field_->type == QpackInstructionFieldType::kName ||
@@ -218,7 +230,7 @@
++field_;
state_ = State::kStartField;
- return;
+ return true;
}
if (field_->type == QpackInstructionFieldType::kVarint2) {
@@ -226,13 +238,13 @@
++field_;
state_ = State::kStartField;
- return;
+ return true;
}
string_length_ = varint_decoder_.value();
if (string_length_ > kStringLiteralLengthLimit) {
OnError("String literal too long.");
- return;
+ return false;
}
std::string* const string =
@@ -242,15 +254,17 @@
if (string_length_ == 0) {
++field_;
state_ = State::kStartField;
- return;
+ return true;
}
string->reserve(string_length_);
state_ = State::kReadString;
+ return true;
}
-size_t QpackInstructionDecoder::DoReadString(QuicStringPiece data) {
+bool QpackInstructionDecoder::DoReadString(QuicStringPiece data,
+ size_t* bytes_consumed) {
DCHECK(!data.empty());
DCHECK(field_->type == QpackInstructionFieldType::kName ||
field_->type == QpackInstructionFieldType::kValue);
@@ -259,18 +273,17 @@
(field_->type == QpackInstructionFieldType::kName) ? &name_ : &value_;
DCHECK_LT(string->size(), string_length_);
- size_t bytes_consumed =
- std::min(string_length_ - string->size(), data.size());
- string->append(data.data(), bytes_consumed);
+ *bytes_consumed = std::min(string_length_ - string->size(), data.size());
+ string->append(data.data(), *bytes_consumed);
DCHECK_LE(string->size(), string_length_);
if (string->size() == string_length_) {
state_ = State::kReadStringDone;
}
- return bytes_consumed;
+ return true;
}
-void QpackInstructionDecoder::DoReadStringDone() {
+bool QpackInstructionDecoder::DoReadStringDone() {
DCHECK(field_->type == QpackInstructionFieldType::kName ||
field_->type == QpackInstructionFieldType::kValue);
@@ -285,13 +298,14 @@
huffman_decoder_.Decode(*string, &decoded_value);
if (!huffman_decoder_.InputProperlyTerminated()) {
OnError("Error in Huffman-encoded string.");
- return;
+ return false;
}
*string = std::move(decoded_value);
}
++field_;
state_ = State::kStartField;
+ return true;
}
const QpackInstruction* QpackInstructionDecoder::LookupOpcode(
diff --git a/quic/core/qpack/qpack_instruction_decoder.h b/quic/core/qpack/qpack_instruction_decoder.h
index f478c24..becb048 100644
--- a/quic/core/qpack/qpack_instruction_decoder.h
+++ b/quic/core/qpack/qpack_instruction_decoder.h
@@ -33,11 +33,15 @@
// Returns true if decoded fields are valid.
// Returns false otherwise, in which case QpackInstructionDecoder stops
// decoding: Delegate methods will not be called, and Decode() must not be
- // called.
+ // called. Implementations are allowed to destroy the
+ // QpackInstructionDecoder instance synchronously if OnInstructionDecoded()
+ // returns false.
virtual bool OnInstructionDecoded(const QpackInstruction* instruction) = 0;
// Called by QpackInstructionDecoder if an error has occurred.
// No more data is processed afterwards.
+ // Implementations are allowed to destroy the QpackInstructionDecoder
+ // instance synchronously.
virtual void OnError(QuicStringPiece error_message) = 0;
};
@@ -48,8 +52,9 @@
QpackInstructionDecoder& operator=(const QpackInstructionDecoder&) = delete;
// Provide a data fragment to decode. Must not be called after an error has
- // occurred. Must not be called with empty |data|.
- void Decode(QuicStringPiece data);
+ // occurred. Must not be called with empty |data|. Return true on success,
+ // false on error (in which case Delegate::OnError() is called synchronously).
+ bool Decode(QuicStringPiece data);
// Returns true if no decoding has taken place yet or if the last instruction
// has been entirely parsed.
@@ -84,18 +89,19 @@
kReadStringDone
};
- // One method for each state. Some take input data and return the number of
- // octets processed. Some take input data but do have void return type
- // because they not consume any bytes. Some do not take any arguments because
- // they only change internal state.
- void DoStartInstruction(QuicStringPiece data);
- void DoStartField();
- void DoReadBit(QuicStringPiece data);
- size_t DoVarintStart(QuicStringPiece data);
- size_t DoVarintResume(QuicStringPiece data);
- void DoVarintDone();
- size_t DoReadString(QuicStringPiece data);
- void DoReadStringDone();
+ // One method for each state. They each return true on success, false on
+ // error (in which case |this| might already be destroyed). Some take input
+ // data and set |*bytes_consumed| to the number of octets processed. Some
+ // take input data but do not consume any bytes. Some do not take any
+ // arguments because they only change internal state.
+ bool DoStartInstruction(QuicStringPiece data);
+ bool DoStartField();
+ bool DoReadBit(QuicStringPiece data);
+ bool DoVarintStart(QuicStringPiece data, size_t* bytes_consumed);
+ bool DoVarintResume(QuicStringPiece data, size_t* bytes_consumed);
+ bool DoVarintDone();
+ bool DoReadString(QuicStringPiece data, size_t* bytes_consumed);
+ bool DoReadStringDone();
// Identify instruction based on opcode encoded in |byte|.
// Returns a pointer to an element of |*language_|.
@@ -127,8 +133,8 @@
// Decoder instance for decoding Huffman encoded strings.
http2::HpackHuffmanDecoder huffman_decoder_;
- // True if a decoding error has been detected either by
- // QpackInstructionDecoder or by Delegate.
+ // True if a decoding error has been detected by QpackInstructionDecoder.
+ // Only used in DCHECKs.
bool error_detected_;
// Decoding state.
diff --git a/quic/core/qpack/qpack_instruction_decoder_test.cc b/quic/core/qpack/qpack_instruction_decoder_test.cc
index bdd85cf..54fa416 100644
--- a/quic/core/qpack/qpack_instruction_decoder_test.cc
+++ b/quic/core/qpack/qpack_instruction_decoder_test.cc
@@ -15,6 +15,7 @@
using ::testing::_;
using ::testing::Eq;
using ::testing::Expectation;
+using ::testing::Invoke;
using ::testing::Return;
using ::testing::StrictMock;
using ::testing::Values;
@@ -64,36 +65,52 @@
};
class QpackInstructionDecoderTest : public QuicTestWithParam<FragmentMode> {
- public:
+ protected:
QpackInstructionDecoderTest()
- : decoder_(TestLanguage(), &delegate_), fragment_mode_(GetParam()) {}
+ : decoder_(std::make_unique<QpackInstructionDecoder>(TestLanguage(),
+ &delegate_)),
+ fragment_mode_(GetParam()) {}
~QpackInstructionDecoderTest() override = default;
- protected:
+ void SetUp() override {
+ // Destroy QpackInstructionDecoder on error to test that it does not crash.
+ // See https://crbug.com/1025209.
+ ON_CALL(delegate_, OnError(_))
+ .WillByDefault(Invoke(
+ [this](QuicStringPiece /* error_message */) { decoder_.reset(); }));
+ }
+
// Decode one full instruction with fragment sizes dictated by
// |fragment_mode_|.
- // Verifies that AtInstructionBoundary() returns true before and after the
+ // Assumes that |data| is a single complete instruction, and accordingly
+ // verifies that AtInstructionBoundary() returns true before and after the
// instruction, and returns false while decoding is in progress.
+ // Assumes that delegate methods destroy |decoder_| if they return false.
void DecodeInstruction(QuicStringPiece data) {
- EXPECT_TRUE(decoder_.AtInstructionBoundary());
+ EXPECT_TRUE(decoder_->AtInstructionBoundary());
FragmentSizeGenerator fragment_size_generator =
FragmentModeToFragmentSizeGenerator(fragment_mode_);
while (!data.empty()) {
size_t fragment_size = std::min(fragment_size_generator(), data.size());
- decoder_.Decode(data.substr(0, fragment_size));
+ bool success = decoder_->Decode(data.substr(0, fragment_size));
+ if (!decoder_) {
+ EXPECT_FALSE(success);
+ return;
+ }
+ EXPECT_TRUE(success);
data = data.substr(fragment_size);
if (!data.empty()) {
- EXPECT_FALSE(decoder_.AtInstructionBoundary());
+ EXPECT_FALSE(decoder_->AtInstructionBoundary());
}
}
- EXPECT_TRUE(decoder_.AtInstructionBoundary());
+ EXPECT_TRUE(decoder_->AtInstructionBoundary());
}
StrictMock<MockDelegate> delegate_;
- QpackInstructionDecoder decoder_;
+ std::unique_ptr<QpackInstructionDecoder> decoder_;
private:
const FragmentMode fragment_mode_;
@@ -108,60 +125,82 @@
EXPECT_CALL(delegate_, OnInstructionDecoded(TestInstruction1()));
DecodeInstruction(QuicTextUtils::HexDecode("7f01ff65"));
- EXPECT_TRUE(decoder_.s_bit());
- EXPECT_EQ(64u, decoder_.varint());
- EXPECT_EQ(356u, decoder_.varint2());
+ EXPECT_TRUE(decoder_->s_bit());
+ EXPECT_EQ(64u, decoder_->varint());
+ EXPECT_EQ(356u, decoder_->varint2());
EXPECT_CALL(delegate_, OnInstructionDecoded(TestInstruction1()));
DecodeInstruction(QuicTextUtils::HexDecode("05c8"));
- EXPECT_FALSE(decoder_.s_bit());
- EXPECT_EQ(5u, decoder_.varint());
- EXPECT_EQ(200u, decoder_.varint2());
+ EXPECT_FALSE(decoder_->s_bit());
+ EXPECT_EQ(5u, decoder_->varint());
+ EXPECT_EQ(200u, decoder_->varint2());
}
TEST_P(QpackInstructionDecoderTest, NameAndValue) {
EXPECT_CALL(delegate_, OnInstructionDecoded(TestInstruction2()));
DecodeInstruction(QuicTextUtils::HexDecode("83666f6f03626172"));
- EXPECT_EQ("foo", decoder_.name());
- EXPECT_EQ("bar", decoder_.value());
+ EXPECT_EQ("foo", decoder_->name());
+ EXPECT_EQ("bar", decoder_->value());
EXPECT_CALL(delegate_, OnInstructionDecoded(TestInstruction2()));
DecodeInstruction(QuicTextUtils::HexDecode("8000"));
- EXPECT_EQ("", decoder_.name());
- EXPECT_EQ("", decoder_.value());
+ EXPECT_EQ("", decoder_->name());
+ EXPECT_EQ("", decoder_->value());
EXPECT_CALL(delegate_, OnInstructionDecoded(TestInstruction2()));
DecodeInstruction(QuicTextUtils::HexDecode("c294e7838c767f"));
- EXPECT_EQ("foo", decoder_.name());
- EXPECT_EQ("bar", decoder_.value());
+ EXPECT_EQ("foo", decoder_->name());
+ EXPECT_EQ("bar", decoder_->value());
}
TEST_P(QpackInstructionDecoderTest, InvalidHuffmanEncoding) {
EXPECT_CALL(delegate_, OnError(Eq("Error in Huffman-encoded string.")));
- decoder_.Decode(QuicTextUtils::HexDecode("c1ff"));
+ DecodeInstruction(QuicTextUtils::HexDecode("c1ff"));
}
TEST_P(QpackInstructionDecoderTest, InvalidVarintEncoding) {
EXPECT_CALL(delegate_, OnError(Eq("Encoded integer too large.")));
- decoder_.Decode(QuicTextUtils::HexDecode("ffffffffffffffffffffff"));
+ DecodeInstruction(QuicTextUtils::HexDecode("ffffffffffffffffffffff"));
}
TEST_P(QpackInstructionDecoderTest, DelegateSignalsError) {
// First instruction is valid.
Expectation first_call =
EXPECT_CALL(delegate_, OnInstructionDecoded(TestInstruction1()))
- .WillOnce(Return(true));
+ .WillOnce(Invoke(
+ [this](const QpackInstruction * /* instruction */) -> bool {
+ EXPECT_EQ(1u, decoder_->varint());
+ return true;
+ }));
+
// Second instruction is invalid. Decoding must halt.
EXPECT_CALL(delegate_, OnInstructionDecoded(TestInstruction1()))
.After(first_call)
- .WillOnce(Return(false));
- decoder_.Decode(QuicTextUtils::HexDecode("01000200030004000500"));
+ .WillOnce(
+ Invoke([this](const QpackInstruction * /* instruction */) -> bool {
+ EXPECT_EQ(2u, decoder_->varint());
+ return false;
+ }));
- EXPECT_EQ(2u, decoder_.varint());
+ EXPECT_FALSE(
+ decoder_->Decode(QuicTextUtils::HexDecode("01000200030004000500")));
+}
+
+// QpackInstructionDecoder must not crash if it is destroyed from a
+// Delegate::OnInstructionDecoded() call as long as it returns false.
+TEST_P(QpackInstructionDecoderTest, DelegateSignalsErrorAndDestroysDecoder) {
+ EXPECT_CALL(delegate_, OnInstructionDecoded(TestInstruction1()))
+ .WillOnce(
+ Invoke([this](const QpackInstruction * /* instruction */) -> bool {
+ EXPECT_EQ(1u, decoder_->varint());
+ decoder_.reset();
+ return false;
+ }));
+ DecodeInstruction(QuicTextUtils::HexDecode("0100"));
}
} // namespace
diff --git a/quic/core/qpack/qpack_progressive_decoder.cc b/quic/core/qpack/qpack_progressive_decoder.cc
index 63b2669..8c8a77e 100644
--- a/quic/core/qpack/qpack_progressive_decoder.cc
+++ b/quic/core/qpack/qpack_progressive_decoder.cc
@@ -57,11 +57,13 @@
while (!prefix_decoded_) {
DCHECK(!blocked_);
- prefix_decoder_->Decode(data.substr(0, 1));
- if (error_detected_) {
+ if (!prefix_decoder_->Decode(data.substr(0, 1))) {
return;
}
+ // |prefix_decoder_->Decode()| must return false if an error is detected.
+ DCHECK(!error_detected_);
+
data = data.substr(1);
if (data.empty()) {
return;
@@ -115,20 +117,28 @@
DCHECK(!error_detected_);
error_detected_ = true;
+ // Might destroy |this|.
handler_->OnDecodingErrorDetected(error_message);
}
void QpackProgressiveDecoder::OnInsertCountReachedThreshold() {
DCHECK(blocked_);
- if (!buffer_.empty()) {
- instruction_decoder_.Decode(buffer_);
- buffer_.clear();
- }
-
+ // Clear |blocked_| before calling instruction_decoder_.Decode() below,
+ // because that might destroy |this| and ~QpackProgressiveDecoder() needs to
+ // know not to call UnregisterObserver().
blocked_ = false;
enforcer_->OnStreamUnblocked(stream_id_);
+ if (!buffer_.empty()) {
+ std::string buffer(std::move(buffer_));
+ buffer_.clear();
+ if (!instruction_decoder_.Decode(buffer)) {
+ // |this| might be destroyed.
+ return;
+ }
+ }
+
if (!decoding_) {
FinishDecoding();
}
diff --git a/quic/core/qpack/qpack_progressive_decoder.h b/quic/core/qpack/qpack_progressive_decoder.h
index 2f306c8..6599c1a 100644
--- a/quic/core/qpack/qpack_progressive_decoder.h
+++ b/quic/core/qpack/qpack_progressive_decoder.h
@@ -44,7 +44,8 @@
virtual void OnDecodingCompleted() = 0;
// Called when a decoding error has occurred. No other methods will be
- // called afterwards.
+ // called afterwards. Implementations are allowed to destroy
+ // the QpackProgressiveDecoder instance synchronously.
virtual void OnDecodingErrorDetected(QuicStringPiece error_message) = 0;
};
