gfe-relnote: Use encryption level instead of looking for the string "CHLO" to identify the QUIC ClientHello. Client side change only, not flag protected.
PiperOrigin-RevId: 241995962
Change-Id: I840eb7531ef55f32c7bb06bc08bcdb947459e732
diff --git a/quic/core/quic_dispatcher.cc b/quic/core/quic_dispatcher.cc
index d1200a9..27ff2b8 100644
--- a/quic/core/quic_dispatcher.cc
+++ b/quic/core/quic_dispatcher.cc
@@ -163,7 +163,7 @@
if (!QuicVersionUsesCryptoFrames(framer_->transport_version())) {
if (!creator_.ConsumeData(
QuicUtils::GetCryptoStreamId(framer_->transport_version()),
- reject.length(), offset, offset,
+ reject.length() - offset, offset,
/*fin=*/false,
/*needs_full_padding=*/true, NOT_RETRANSMISSION, &frame)) {
QUIC_BUG << "Unable to consume data into an empty packet.";
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc
index a3542d3..ca53ca4 100644
--- a/quic/core/quic_framer.cc
+++ b/quic/core/quic_framer.cc
@@ -5176,25 +5176,6 @@
return header.long_packet_type == VERSION_NEGOTIATION;
}
-bool QuicFramer::StartsWithChlo(QuicStreamId id,
- QuicStreamOffset offset) const {
- if (data_producer_ == nullptr) {
- QUIC_BUG << "Does not have data producer.";
- return false;
- }
- char buf[sizeof(kCHLO)];
- QuicDataWriter writer(sizeof(kCHLO), buf);
- if (data_producer_->WriteStreamData(id, offset, sizeof(kCHLO), &writer) !=
- WRITE_SUCCESS) {
- QUIC_BUG << "Failed to write data for stream " << id << " with offset "
- << offset << " data_length = " << sizeof(kCHLO);
- return false;
- }
-
- return strncmp(buf, reinterpret_cast<const char*>(&kCHLO), sizeof(kCHLO)) ==
- 0;
-}
-
bool QuicFramer::AppendIetfConnectionCloseFrame(
const QuicConnectionCloseFrame& frame,
QuicDataWriter* writer) {
diff --git a/quic/core/quic_framer.h b/quic/core/quic_framer.h
index d8c135a..b431f14 100644
--- a/quic/core/quic_framer.h
+++ b/quic/core/quic_framer.h
@@ -523,9 +523,6 @@
// Tell framer to infer packet header type from version_.
void InferPacketHeaderTypeFromVersion();
- // Returns true if data with |offset| of stream |id| starts with 'CHLO'.
- bool StartsWithChlo(QuicStreamId id, QuicStreamOffset offset) const;
-
// Returns true if |header| is considered as an stateless reset packet.
bool IsIetfStatelessResetPacket(const QuicPacketHeader& header) const;
diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc
index 9f0e700..e627579 100644
--- a/quic/core/quic_framer_test.cc
+++ b/quic/core/quic_framer_test.cc
@@ -9854,30 +9854,6 @@
framer_.version());
}
-TEST_P(QuicFramerTest, StartsWithChlo) {
- if (QuicVersionUsesCryptoFrames(framer_.transport_version())) {
- // When client hellos are sent in crypto frames, this test doesn't make
- // sense.
- return;
- }
- SimpleDataProducer producer;
- framer_.set_data_producer(&producer);
- QuicStringPiece data("CHLOCHLO");
- struct iovec iovec;
- iovec.iov_base = const_cast<char*>(data.data());
- iovec.iov_len = data.length();
- QuicStreamId stream_id =
- QuicUtils::GetCryptoStreamId(framer_.transport_version());
- producer.SaveStreamData(stream_id, &iovec, 1, 0, data.length());
- for (size_t offset = 0; offset < 5; ++offset) {
- if (offset == 0 || offset == 4) {
- EXPECT_TRUE(framer_.StartsWithChlo(stream_id, offset));
- } else {
- EXPECT_FALSE(framer_.StartsWithChlo(stream_id, offset));
- }
- }
-}
-
TEST_P(QuicFramerTest, IetfBlockedFrame) {
// This test only for version 99.
if (framer_.transport_version() != QUIC_VERSION_99) {
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc
index 951fb81..224aadf 100644
--- a/quic/core/quic_packet_creator.cc
+++ b/quic/core/quic_packet_creator.cc
@@ -174,30 +174,25 @@
}
bool QuicPacketCreator::ConsumeData(QuicStreamId id,
- size_t write_length,
- size_t iov_offset,
+ size_t data_size,
QuicStreamOffset offset,
bool fin,
bool needs_full_padding,
TransmissionType transmission_type,
QuicFrame* frame) {
- // TODO(ianswett): Remove write_length once the multi-packet CHLO check is
- // done higher in the stack.
- DCHECK_GE(write_length, iov_offset);
- const size_t data_size = write_length - iov_offset;
if (!HasRoomForStreamFrame(id, offset, data_size)) {
return false;
}
CreateStreamFrame(id, data_size, offset, fin, frame);
// Explicitly disallow multi-packet CHLOs.
if (FLAGS_quic_enforce_single_packet_chlo &&
- StreamFrameStartsWithChlo(frame->stream_frame) &&
- frame->stream_frame.data_length < write_length) {
+ StreamFrameIsClientHello(frame->stream_frame) &&
+ frame->stream_frame.data_length < data_size) {
const std::string error_details =
"Client hello won't fit in a single packet.";
QUIC_BUG << error_details << " Constructed stream frame length: "
<< frame->stream_frame.data_length
- << " CHLO length: " << write_length;
+ << " CHLO length: " << data_size;
delegate_->OnUnrecoverableError(QUIC_CRYPTO_CHLO_TOO_LARGE, error_details,
ConnectionCloseSource::FROM_SELF);
return false;
@@ -953,15 +948,15 @@
pending_padding_bytes_ += size;
}
-bool QuicPacketCreator::StreamFrameStartsWithChlo(
+bool QuicPacketCreator::StreamFrameIsClientHello(
const QuicStreamFrame& frame) const {
if (framer_->perspective() == Perspective::IS_SERVER ||
frame.stream_id !=
- QuicUtils::GetCryptoStreamId(framer_->transport_version()) ||
- frame.data_length < sizeof(kCHLO)) {
+ QuicUtils::GetCryptoStreamId(framer_->transport_version())) {
return false;
}
- return framer_->StartsWithChlo(frame.stream_id, frame.offset);
+ // The ClientHello is always sent with INITIAL encryption.
+ return packet_.encryption_level == ENCRYPTION_INITIAL;
}
void QuicPacketCreator::SetConnectionIdIncluded(
diff --git a/quic/core/quic_packet_creator.h b/quic/core/quic_packet_creator.h
index 150cb9c..7c8f496 100644
--- a/quic/core/quic_packet_creator.h
+++ b/quic/core/quic_packet_creator.h
@@ -96,8 +96,7 @@
// If current packet is not full, creates a stream frame that fits into the
// open packet and adds it to the packet.
bool ConsumeData(QuicStreamId id,
- size_t write_length,
- size_t iov_offset,
+ size_t data_length,
QuicStreamOffset offset,
bool fin,
bool needs_full_padding,
@@ -352,8 +351,8 @@
// wire. Is non-zero for v99 IETF Initial, 0-RTT or Handshake packets.
QuicVariableLengthIntegerLength GetLengthLength() const;
- // Returns true if |frame| starts with CHLO.
- bool StreamFrameStartsWithChlo(const QuicStreamFrame& frame) const;
+ // Returns true if |frame| is a ClientHello.
+ bool StreamFrameIsClientHello(const QuicStreamFrame& frame) const;
// Returns true if packet under construction has IETF long header.
bool HasIetfLongHeader() const;
diff --git a/quic/core/quic_packet_creator_test.cc b/quic/core/quic_packet_creator_test.cc
index 1cdc3e4..aea0540 100644
--- a/quic/core/quic_packet_creator_test.cc
+++ b/quic/core/quic_packet_creator_test.cc
@@ -97,7 +97,7 @@
if (data_length > 0) {
producer_->SaveStreamData(id, iov, iov_count, iov_offset, data_length);
}
- return QuicPacketCreator::ConsumeData(id, data_length, iov_offset, offset,
+ return QuicPacketCreator::ConsumeData(id, data_length - iov_offset, offset,
fin, needs_full_padding,
transmission_type, frame);
}
@@ -693,6 +693,9 @@
}
TEST_P(QuicPacketCreatorTest, CryptoStreamFramePacketPadding) {
+ // This test serializes crypto payloads slightly larger than a packet, which
+ // Causes the multi-packet ClientHello check to fail.
+ FLAGS_quic_enforce_single_packet_chlo = false;
// Compute the total overhead for a single frame in packet.
size_t overhead =
GetPacketHeaderOverhead(client_framer_.transport_version()) +
diff --git a/quic/core/quic_packet_generator.cc b/quic/core/quic_packet_generator.cc
index 257ca9b..8267ade 100644
--- a/quic/core/quic_packet_generator.cc
+++ b/quic/core/quic_packet_generator.cc
@@ -148,7 +148,7 @@
bool needs_full_padding =
has_handshake && fully_pad_crypto_handshake_packets_;
- if (!packet_creator_.ConsumeData(id, write_length, total_bytes_consumed,
+ if (!packet_creator_.ConsumeData(id, write_length - total_bytes_consumed,
offset + total_bytes_consumed, fin,
needs_full_padding,
next_transmission_type_, &frame)) {
