Do not allow push until MAX_PUSH_ID is received.
gfe-relnote: n/a; Change to IETF QUIC push which is disabled in production.
PiperOrigin-RevId: 303154113
Change-Id: I4a84c4022f5fc4ba24c50cbb49b1752022df93b4
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc
index cd6a971..cec0ceb 100644
--- a/quic/core/http/end_to_end_test.cc
+++ b/quic/core/http/end_to_end_test.cc
@@ -4175,13 +4175,11 @@
client_->SendSynchronousRequest("/foo");
- EXPECT_EQ(kMaxQuicStreamId,
- static_cast<QuicSpdySession*>(client_->client()->session())
- ->max_allowed_push_id());
+ EXPECT_TRUE(static_cast<QuicSpdySession*>(client_->client()->session())
+ ->CanCreatePushStreamWithId(kMaxQuicStreamId));
- EXPECT_EQ(
- kMaxQuicStreamId,
- static_cast<QuicSpdySession*>(GetServerSession())->max_allowed_push_id());
+ EXPECT_TRUE(static_cast<QuicSpdySession*>(GetServerSession())
+ ->CanCreatePushStreamWithId(kMaxQuicStreamId));
}
TEST_P(EndToEndTest, CustomTransportParameters) {
diff --git a/quic/core/http/quic_spdy_client_session_base.cc b/quic/core/http/quic_spdy_client_session_base.cc
index 415609e..b8dae49 100644
--- a/quic/core/http/quic_spdy_client_session_base.cc
+++ b/quic/core/http/quic_spdy_client_session_base.cc
@@ -85,7 +85,7 @@
}
if (VersionUsesHttp3(transport_version()) &&
- promised_stream_id > max_allowed_push_id()) {
+ !CanCreatePushStreamWithId(promised_stream_id)) {
connection()->CloseConnection(
QUIC_INVALID_STREAM_ID,
"Received push stream id higher than MAX_PUSH_ID.",
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc
index 52bd00e..1a23cdd 100644
--- a/quic/core/http/quic_spdy_session.cc
+++ b/quic/core/http/quic_spdy_session.cc
@@ -398,7 +398,6 @@
spdy_framer_visitor_(new SpdyFramerVisitor(this)),
server_push_enabled_(true),
ietf_server_push_enabled_(false),
- max_push_id_(0),
destruction_indicator_(123456789),
debug_visitor_(nullptr),
http3_goaway_received_(false),
@@ -702,7 +701,7 @@
return;
}
- if (promised_stream_id > max_allowed_push_id()) {
+ if (!max_push_id_.has_value() || promised_stream_id > max_push_id_.value()) {
QUIC_BUG
<< "Server shouldn't send push id higher than client's MAX_PUSH_ID.";
return;
@@ -725,8 +724,9 @@
}
bool QuicSpdySession::server_push_enabled() const {
- return VersionUsesHttp3(transport_version()) ? ietf_server_push_enabled_
- : server_push_enabled_;
+ return VersionUsesHttp3(transport_version())
+ ? ietf_server_push_enabled_ && max_push_id_.has_value()
+ : server_push_enabled_;
}
void QuicSpdySession::SendInitialData() {
@@ -1214,12 +1214,20 @@
void QuicSpdySession::SetMaxPushId(QuicStreamId max_push_id) {
DCHECK(VersionUsesHttp3(transport_version()));
DCHECK_EQ(Perspective::IS_CLIENT, perspective());
- DCHECK_GE(max_push_id, max_push_id_);
+ if (max_push_id_.has_value()) {
+ DCHECK_GE(max_push_id, max_push_id_.value());
+ }
- QuicStreamId old_max_push_id = max_push_id_;
+ ietf_server_push_enabled_ = true;
+
+ if (max_push_id_.has_value()) {
+ QUIC_DVLOG(1) << "Setting max_push_id to: " << max_push_id
+ << " from: " << max_push_id_.value();
+ } else {
+ QUIC_DVLOG(1) << "Setting max_push_id to: " << max_push_id
+ << " from unset";
+ }
max_push_id_ = max_push_id;
- QUIC_DVLOG(1) << "Setting max_push_id to: " << max_push_id_
- << " from: " << old_max_push_id;
if (OneRttKeysAvailable()) {
SendMaxPushId();
@@ -1230,24 +1238,32 @@
DCHECK(VersionUsesHttp3(transport_version()));
DCHECK_EQ(Perspective::IS_SERVER, perspective());
- QuicStreamId old_max_push_id = max_push_id_;
+ if (max_push_id_.has_value()) {
+ QUIC_DVLOG(1) << "Setting max_push_id to: " << max_push_id
+ << " from: " << max_push_id_.value();
+ } else {
+ QUIC_DVLOG(1) << "Setting max_push_id to: " << max_push_id
+ << " from unset";
+ }
+ quiche::QuicheOptional<QuicStreamId> old_max_push_id = max_push_id_;
max_push_id_ = max_push_id;
- QUIC_DVLOG(1) << "Setting max_push_id to: " << max_push_id_
- << " from: " << old_max_push_id;
- if (max_push_id_ > old_max_push_id) {
+ if (!old_max_push_id.has_value() ||
+ max_push_id_.value() > old_max_push_id.value()) {
OnCanCreateNewOutgoingStream(true);
return true;
}
// Equal value is not considered an error.
- return max_push_id >= old_max_push_id;
+ return max_push_id_.value() >= old_max_push_id.value();
}
void QuicSpdySession::SendMaxPushId() {
DCHECK(VersionUsesHttp3(transport_version()));
DCHECK_EQ(Perspective::IS_CLIENT, perspective());
- send_control_stream_->SendMaxPushIdFrame(max_push_id_);
+ // TODO(bnc): Do not send a MAX_PUSH_ID frame if SetMaxPushId() has not been
+ // called yet.
+ send_control_stream_->SendMaxPushIdFrame(max_push_id_.value_or(0));
}
void QuicSpdySession::EnableServerPush() {
@@ -1257,6 +1273,13 @@
ietf_server_push_enabled_ = true;
}
+bool QuicSpdySession::CanCreatePushStreamWithId(QuicStreamId push_id) {
+ DCHECK(VersionUsesHttp3(transport_version()));
+
+ return ietf_server_push_enabled_ && max_push_id_.has_value() &&
+ max_push_id_.value() >= push_id;
+}
+
void QuicSpdySession::CloseConnectionOnDuplicateHttp3UnidirectionalStreams(
quiche::QuicheStringPiece type) {
QUIC_PEER_BUG << quiche::QuicheStrCat("Received a duplicate ", type,
diff --git a/quic/core/http/quic_spdy_session.h b/quic/core/http/quic_spdy_session.h
index 3fa2963..b0ce12a 100644
--- a/quic/core/http/quic_spdy_session.h
+++ b/quic/core/http/quic_spdy_session.h
@@ -24,6 +24,7 @@
#include "net/third_party/quiche/src/quic/core/quic_session.h"
#include "net/third_party/quiche/src/quic/core/quic_versions.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_export.h"
+#include "net/third_party/quiche/src/common/platform/api/quiche_optional.h"
#include "net/third_party/quiche/src/common/platform/api/quiche_string_piece.h"
#include "net/third_party/quiche/src/spdy/core/http2_frame_decoder_adapter.h"
@@ -235,6 +236,14 @@
const QuicHeadersStream* headers_stream() const { return headers_stream_; }
+ // Returns whether server push is enabled.
+ // For a Google QUIC client this always returns false.
+ // For a Google QUIC server this is set by incoming SETTINGS_ENABLE_PUSH.
+ // For an IETF QUIC client this returns true if SetMaxPushId() has ever been
+ // called.
+ // For an IETF QUIC server this returns true if EnableServerPush() has been
+ // called and the server has received at least one MAX_PUSH_ID frame from the
+ // client.
bool server_push_enabled() const;
// Called when a setting is parsed from an incoming SETTINGS frame.
@@ -308,11 +317,6 @@
// |max_push_id_|.
bool OnMaxPushIdFrame(QuicStreamId max_push_id);
- // TODO(b/151451061): Change this API to distinguish between having received
- // no MAX_PUSH_ID frame and one MAX_PUSH_ID frame with push ID 0.
- // TODO(b/136295430): Use sequential PUSH IDs instead of stream IDs.
- QuicStreamId max_allowed_push_id() { return max_push_id_; }
-
// Enables server push.
// Must only be called when using IETF QUIC, for which server push is disabled
// by default. Server push defaults to enabled and cannot be disabled for
@@ -321,6 +325,16 @@
// never calling SetMaxPushId().
void EnableServerPush();
+ // Returns true if push is enabled and a push with |push_id| can be created.
+ // For a server this means that EnableServerPush() has been called, at least
+ // one MAX_PUSH_ID frame has been received, and the largest received
+ // MAX_PUSH_ID value is greater than or equal to |push_id|.
+ // For a client this means that SetMaxPushId() has been called with
+ // |max_push_id| greater than or equal to |push_id|.
+ // Must only be called when using IETF QUIC.
+ // TODO(b/136295430): Use sequential PUSH IDs instead of stream IDs.
+ bool CanCreatePushStreamWithId(QuicStreamId push_id);
+
int32_t destruction_indicator() const { return destruction_indicator_; }
void set_debug_visitor(Http3DebugVisitor* debug_visitor) {
@@ -515,12 +529,18 @@
// Defaults to true.
bool server_push_enabled_;
- // Used in IETF QUIC only, and only for servers. Set locally via
- // EnableServerPush(), not influenced by data received from the client.
- // Defaults to false.
+ // Used in IETF QUIC only. Defaults to false.
+ // Server push is enabled for a server by calling EnableServerPush().
+ // Server push is enabled for a client by calling SetMaxPushId().
bool ietf_server_push_enabled_;
- QuicStreamId max_push_id_;
+ // Used in IETF QUIC only. Unset until a MAX_PUSH_ID frame is received/sent.
+ // For a server, the push ID in the most recently received MAX_PUSH_ID frame.
+ // For a client before 1-RTT keys are available, the push ID to be sent in the
+ // initial MAX_PUSH_ID frame.
+ // For a client after 1-RTT keys are available, the push ID in the most
+ // recently sent MAX_PUSH_ID frame.
+ quiche::QuicheOptional<QuicStreamId> max_push_id_;
// An integer used for live check. The indicator is assigned a value in
// constructor. As long as it is not the assigned value, that would indicate
@@ -535,7 +555,9 @@
// If the endpoint has sent HTTP/3 GOAWAY frame.
bool http3_goaway_sent_;
- // If the endpoint has sent the initial HTTP/3 MAX_PUSH_ID frame.
+ // If SendMaxPushId() has been called from SendInitialData(). Note that a
+ // MAX_PUSH_ID frame is only sent if SetMaxPushId() had been called
+ // beforehand.
bool http3_max_push_id_sent_;
// Priority values received in PRIORITY_UPDATE frames for streams that are not
diff --git a/quic/tools/quic_simple_server_session.cc b/quic/tools/quic_simple_server_session.cc
index bb9a852..5c83b48 100644
--- a/quic/tools/quic_simple_server_session.cc
+++ b/quic/tools/quic_simple_server_session.cc
@@ -74,10 +74,14 @@
for (QuicBackendResponse::ServerPushInfo resource : resources) {
spdy::SpdyHeaderBlock headers = SynthesizePushRequestHeaders(
request_url, resource, original_request_headers);
+ // TODO(b/136295430): Use sequential push IDs for IETF QUIC.
+ // TODO(bnc): If |highest_promised_stream_id_| is too large, it will always
+ // be skipped. Fix it by not incrementing if CanCreatePushStreamWithId()
+ // returns false.
highest_promised_stream_id_ +=
QuicUtils::StreamIdDelta(transport_version());
if (VersionUsesHttp3(transport_version()) &&
- highest_promised_stream_id_ > max_allowed_push_id()) {
+ !CanCreatePushStreamWithId(highest_promised_stream_id_)) {
return;
}
SendPushPromise(original_stream_id, highest_promised_stream_id_,
