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)) {