gfe-relnote: Fix a bug in QuicSpdyClientStreamBase. Protected by --gfe2_reloadable_flag_quic_eliminate_static_stream_map_3
When eliminate_static_stream_map is true, GetSpdyDataStream() will be called with a stream id that may not be a spdy stream. This results in an invalid cast which blows up in some Chrome tests. I've added a DCHECK in GetSpdyDataStream() to make sure we never do this again, and have added and IsStaticStream() method which can be used for checks in this case. Renames gfe2_reloadable_flag_quic_eliminate_static_stream_map_2 to gfe2_reloadable_flag_quic_eliminate_static_stream_map_3.
PiperOrigin-RevId: 248762017
Change-Id: Iab361cc46408cd57e38848f172a7f826f346b28d
diff --git a/quic/core/http/quic_spdy_client_session_base.cc b/quic/core/http/quic_spdy_client_session_base.cc
index 2db5b84..93f6880 100644
--- a/quic/core/http/quic_spdy_client_session_base.cc
+++ b/quic/core/http/quic_spdy_client_session_base.cc
@@ -64,7 +64,7 @@
QuicStreamId promised_stream_id,
size_t frame_len,
const QuicHeaderList& header_list) {
- if (QuicContainsKey(static_streams(), stream_id)) {
+ if (IsStaticStream(stream_id)) {
connection()->CloseConnection(
QUIC_INVALID_HEADERS_STREAM_DATA, "stream is static",
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
@@ -95,12 +95,6 @@
// It's quite possible to receive headers after a stream has been reset.
return;
}
- if (eliminate_static_stream_map() && stream->is_static()) {
- connection()->CloseConnection(
- QUIC_INVALID_HEADERS_STREAM_DATA, "stream is static",
- ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
- return;
- }
stream->OnPromiseHeaderList(promised_stream_id, frame_len, header_list);
}
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc
index 5eec97d..bdaa433 100644
--- a/quic/core/http/quic_spdy_session.cc
+++ b/quic/core/http/quic_spdy_session.cc
@@ -363,7 +363,7 @@
QuicUtils::GetHeadersStreamId(connection()->transport_version()),
headers_stream_.get());
} else {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 7, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 7, 17);
unowned_headers_stream_ = headers_stream_.get();
RegisterStaticStreamNew(std::move(headers_stream_));
}
@@ -416,7 +416,7 @@
bool fin,
size_t frame_len,
const QuicHeaderList& header_list) {
- if (QuicContainsKey(static_streams(), stream_id)) {
+ if (IsStaticStream(stream_id)) {
connection()->CloseConnection(
QUIC_INVALID_HEADERS_STREAM_DATA, "stream is static",
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
@@ -447,13 +447,6 @@
// It's quite possible to receive headers after a stream has been reset.
return;
}
- if (eliminate_static_stream_map() && stream->is_static()) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 8, 17);
- connection()->CloseConnection(
- QUIC_INVALID_HEADERS_STREAM_DATA, "stream is static",
- ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
- return;
- }
stream->OnStreamHeaderList(fin, frame_len, header_list);
}
@@ -542,7 +535,9 @@
QuicSpdyStream* QuicSpdySession::GetSpdyDataStream(
const QuicStreamId stream_id) {
- return static_cast<QuicSpdyStream*>(GetOrCreateDynamicStream(stream_id));
+ QuicStream* stream = GetOrCreateDynamicStream(stream_id);
+ DCHECK(!stream || !stream->is_static());
+ return static_cast<QuicSpdyStream*>(stream);
}
void QuicSpdySession::OnCryptoHandshakeEvent(CryptoHandshakeEvent event) {
@@ -716,7 +711,7 @@
// In the case where session is destructed by calling
// dynamic_streams().clear(), we will have incorrect accounting here.
// TODO(renjietang): Modify destructors and make this a DCHECK.
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 9, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 9, 17);
if (static_cast<size_t>(dynamic_streams().size()) >
num_incoming_static_streams() + num_outgoing_static_streams()) {
return dynamic_streams().size() - num_incoming_static_streams() -
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc
index 8217cd7..0e2fcf8 100644
--- a/quic/core/http/quic_spdy_session_test.cc
+++ b/quic/core/http/quic_spdy_session_test.cc
@@ -926,7 +926,7 @@
EXPECT_CALL(*crypto_stream, OnCanWrite());
}
TestHeadersStream* headers_stream;
- if (!GetQuicReloadableFlag(quic_eliminate_static_stream_map_2) &&
+ if (!GetQuicReloadableFlag(quic_eliminate_static_stream_map_3) &&
!QuicVersionUsesCryptoFrames(connection_->transport_version())) {
QuicSpdySessionPeer::SetHeadersStream(&session_, nullptr);
headers_stream = new TestHeadersStream(&session_);
@@ -1669,7 +1669,7 @@
TEST_P(QuicSpdySessionTestClient, WritePriority) {
TestHeadersStream* headers_stream;
- if (!GetQuicReloadableFlag(quic_eliminate_static_stream_map_2) &&
+ if (!GetQuicReloadableFlag(quic_eliminate_static_stream_map_3) &&
!QuicVersionUsesCryptoFrames(connection_->transport_version())) {
QuicSpdySessionPeer::SetHeadersStream(&session_, nullptr);
headers_stream = new TestHeadersStream(&session_);
diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc
index abc6331..6d526ab 100644
--- a/quic/core/quic_session.cc
+++ b/quic/core/quic_session.cc
@@ -89,7 +89,7 @@
closed_streams_clean_up_alarm_(nullptr),
supported_versions_(supported_versions),
eliminate_static_stream_map_(
- GetQuicReloadableFlag(quic_eliminate_static_stream_map_2) ||
+ GetQuicReloadableFlag(quic_eliminate_static_stream_map_3) ||
QuicVersionUsesCryptoFrames(connection->transport_version())) {
closed_streams_clean_up_alarm_ =
QuicWrapUnique<QuicAlarm>(connection_->alarm_factory()->CreateAlarm(
@@ -113,7 +113,7 @@
QuicUtils::GetCryptoStreamId(connection_->transport_version()),
GetMutableCryptoStream());
} else {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 10, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 10, 17);
QuicStreamId id =
QuicUtils::GetCryptoStreamId(connection_->transport_version());
largest_static_stream_id_ = std::max(id, largest_static_stream_id_);
@@ -216,7 +216,7 @@
}
if (eliminate_static_stream_map_ && frame.fin &&
handler.stream->is_static()) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 1, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 1, 17);
connection()->CloseConnection(
QUIC_INVALID_STREAM_ID, "Attempt to close a static stream",
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
@@ -304,7 +304,7 @@
}
if (eliminate_static_stream_map_ && stream->is_static()) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 2, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 2, 17);
QUIC_DVLOG(1) << ENDPOINT
<< "Received STOP_SENDING for a static stream, id: "
<< stream_id << " Closing connection";
@@ -380,7 +380,7 @@
return; // Errors are handled by GetOrCreateStream.
}
if (eliminate_static_stream_map_ && handler.stream->is_static()) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 3, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 3, 17);
connection()->CloseConnection(
QUIC_INVALID_STREAM_ID, "Attempt to reset a static stream",
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
@@ -439,7 +439,7 @@
}
}
} else {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 4, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 4, 17);
// Copy all non static streams in a new map for the ease of deleting.
QuicSmallMap<QuicStreamId, QuicStream*, 10> non_static_streams;
for (const auto& it : dynamic_stream_map_) {
@@ -770,7 +770,7 @@
DynamicStreamMap::iterator it = dynamic_stream_map_.find(id);
if (it != dynamic_stream_map_.end()) {
if (eliminate_static_stream_map_ && it->second->is_static()) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 5, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 5, 17);
QUIC_DVLOG(1) << ENDPOINT
<< "Try to send rst for a static stream, id: " << id
<< " Closing connection";
@@ -845,7 +845,7 @@
}
QuicStream* stream = it->second.get();
if (eliminate_static_stream_map_ && stream->is_static()) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 6, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 6, 17);
QUIC_DVLOG(1) << ENDPOINT
<< "Try to close a static stream, id: " << stream_id
<< " Closing connection";
@@ -1093,7 +1093,7 @@
}
if (eliminate_static_stream_map_ &&
!QuicVersionUsesCryptoFrames(connection_->transport_version())) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 11, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 11, 17);
GetMutableCryptoStream()->flow_controller()->UpdateReceiveWindowSize(
stream_window);
}
@@ -1144,7 +1144,7 @@
}
if (eliminate_static_stream_map_ &&
!QuicVersionUsesCryptoFrames(connection_->transport_version())) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 12, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 12, 17);
GetMutableCryptoStream()->UpdateSendWindowOffset(new_window);
}
}
@@ -1276,7 +1276,7 @@
if (eliminate_static_stream_map_ &&
QuicUtils::IsCryptoStreamId(connection_->transport_version(),
stream_id)) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 13, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 13, 17);
return StreamHandler(GetMutableCryptoStream());
}
StaticStreamMap::iterator it = static_stream_map_.find(stream_id);
@@ -1423,6 +1423,18 @@
return false;
}
+bool QuicSession::IsStaticStream(QuicStreamId id) const {
+ if (eliminate_static_stream_map()) {
+ auto it = dynamic_stream_map_.find(id);
+ if (it == dynamic_stream_map_.end()) {
+ return false;
+ }
+ return it->second->is_static();
+ }
+
+ return QuicContainsKey(static_streams(), id);
+}
+
size_t QuicSession::GetNumOpenIncomingStreams() const {
return num_dynamic_incoming_streams_ - num_draining_incoming_streams_ +
num_locally_closed_incoming_streams_highest_offset_;
@@ -1511,7 +1523,7 @@
if (eliminate_static_stream_map_ &&
!QuicVersionUsesCryptoFrames(connection_->transport_version()) &&
GetMutableCryptoStream()->flow_controller()->IsBlocked()) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 14, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 14, 17);
return true;
}
return false;
@@ -1572,7 +1584,7 @@
if (eliminate_static_stream_map_ &&
QuicUtils::IsCryptoStreamId(connection_->transport_version(), id)) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 15, 17);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_3, 15, 17);
return const_cast<QuicCryptoStream*>(GetCryptoStream());
}
diff --git a/quic/core/quic_session.h b/quic/core/quic_session.h
index 7da8ec3..d8f40c9 100644
--- a/quic/core/quic_session.h
+++ b/quic/core/quic_session.h
@@ -525,6 +525,9 @@
// Returns true if the stream is still active.
bool IsOpenStream(QuicStreamId id);
+ // Returns true if the stream is a static stream.
+ bool IsStaticStream(QuicStreamId id) const;
+
// Close connection when receive a frame for a locally-created nonexistant
// stream.
// Prerequisite: IsClosedStream(stream_id) == false
diff --git a/quic/core/quic_session_test.cc b/quic/core/quic_session_test.cc
index 5af49fa..5b86bdd 100644
--- a/quic/core/quic_session_test.cc
+++ b/quic/core/quic_session_test.cc
@@ -1268,7 +1268,7 @@
QuicUtils::GetHeadersStreamId(connection_->transport_version());
std::unique_ptr<TestStream> fake_headers_stream = QuicMakeUnique<TestStream>(
headers_stream_id, &session_, /*is_static*/ true, BIDIRECTIONAL);
- if (GetQuicReloadableFlag(quic_eliminate_static_stream_map_2)) {
+ if (GetQuicReloadableFlag(quic_eliminate_static_stream_map_3)) {
QuicSessionPeer::RegisterStaticStreamNew(&session_,
std::move(fake_headers_stream));
} else {
@@ -1289,7 +1289,7 @@
QuicUtils::GetHeadersStreamId(connection_->transport_version());
std::unique_ptr<TestStream> fake_headers_stream = QuicMakeUnique<TestStream>(
headers_stream_id, &session_, /*is_static*/ true, BIDIRECTIONAL);
- if (GetQuicReloadableFlag(quic_eliminate_static_stream_map_2)) {
+ if (GetQuicReloadableFlag(quic_eliminate_static_stream_map_3)) {
QuicSessionPeer::RegisterStaticStreamNew(&session_,
std::move(fake_headers_stream));
} else {
@@ -2428,7 +2428,7 @@
QuicStreamId stream_id = 0;
std::unique_ptr<TestStream> fake_static_stream = QuicMakeUnique<TestStream>(
stream_id, &session_, /*is_static*/ true, BIDIRECTIONAL);
- if (GetQuicReloadableFlag(quic_eliminate_static_stream_map_2)) {
+ if (GetQuicReloadableFlag(quic_eliminate_static_stream_map_3)) {
QuicSessionPeer::RegisterStaticStreamNew(&session_,
std::move(fake_static_stream));
} else {
