Landing Recent QUIC changes until 6:40 PM, Mar 5, 2019 UTC-5
Refactor QuicFramer connection ID read code
This CL refactors how we read connection IDs to allow future changes
that will use different default lengths based on what is configured for
this connection.
Merge internal change: 236935043
https://chromium-review.googlesource.com/c/1504509/
Make a test involving connection IDs more future-proof
Merge internal change: 236924820
https://chromium-review.googlesource.com/c/1504508/
Remove some fixed packet lengths.
This CL moves us yet another step closer to being connection ID length
agnostic in the lower layers such as the QuicFramer. More specifically,
it replaces instances of PACKET_8BYTE_CONNECTION_ID with
connection_id.length(). This is safe to use without flag protection
because we have code in all connection ID entry points to ensure that
their length is 8.
Merge internal change: 236894613
https://chromium-review.googlesource.com/c/1504506/
R=rch@chromium.org
Change-Id: I488160120ad125ff342e5983cba22946dc437183
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504510
Commit-Queue: Victor Vasiliev <vasilvv@chromium.org>
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637998}diff --git a/net/third_party/quic/core/http/end_to_end_test.cc b/net/third_party/quic/core/http/end_to_end_test.cc
index 70491a9..6cc4a659 100644
--- a/net/third_party/quic/core/http/end_to_end_test.cc
+++ b/net/third_party/quic/core/http/end_to_end_test.cc
@@ -3226,8 +3226,7 @@
EXPECT_TRUE(client_->client()->WaitForCryptoHandshakeConfirmed());
QuicConfig* config = client_->client()->session()->config();
EXPECT_TRUE(config->HasReceivedStatelessResetToken());
- // TODO(dschinazi) b/120240679 - convert connection ID to UInt128
- EXPECT_EQ(TestConnectionIdToUInt64(
+ EXPECT_EQ(QuicUtils::GenerateStatelessResetToken(
client_->client()->session()->connection()->connection_id()),
config->ReceivedStatelessResetToken());
client_->Disconnect();
diff --git a/net/third_party/quic/core/quic_crypto_server_handshaker.cc b/net/third_party/quic/core/quic_crypto_server_handshaker.cc
index c0ad057..7fda5fa 100644
--- a/net/third_party/quic/core/quic_crypto_server_handshaker.cc
+++ b/net/third_party/quic/core/quic_crypto_server_handshaker.cc
@@ -430,7 +430,8 @@
server_designated_connection_id, connection->clock(),
connection->random_generator(), compressed_certs_cache_,
crypto_negotiated_params_, signed_config_,
- QuicCryptoStream::CryptoMessageFramingOverhead(transport_version()),
+ QuicCryptoStream::CryptoMessageFramingOverhead(
+ transport_version(), connection->connection_id()),
chlo_packet_size_, std::move(done_cb));
}
diff --git a/net/third_party/quic/core/quic_crypto_stream.cc b/net/third_party/quic/core/quic_crypto_stream.cc
index 802e922..82afbbd 100644
--- a/net/third_party/quic/core/quic_crypto_stream.cc
+++ b/net/third_party/quic/core/quic_crypto_stream.cc
@@ -39,9 +39,12 @@
// static
QuicByteCount QuicCryptoStream::CryptoMessageFramingOverhead(
- QuicTransportVersion version) {
+ QuicTransportVersion version,
+ QuicConnectionId connection_id) {
+ DCHECK(QuicUtils::IsConnectionIdValidForVersion(connection_id, version));
return QuicPacketCreator::StreamFramePacketOverhead(
- version, PACKET_8BYTE_CONNECTION_ID, PACKET_0BYTE_CONNECTION_ID,
+ version, static_cast<QuicConnectionIdLength>(connection_id.length()),
+ PACKET_0BYTE_CONNECTION_ID,
/*include_version=*/true,
/*include_diversification_nonce=*/true,
version > QUIC_VERSION_43 ? PACKET_4BYTE_PACKET_NUMBER
diff --git a/net/third_party/quic/core/quic_crypto_stream.h b/net/third_party/quic/core/quic_crypto_stream.h
index 53653f8..4bb5365 100644
--- a/net/third_party/quic/core/quic_crypto_stream.h
+++ b/net/third_party/quic/core/quic_crypto_stream.h
@@ -42,7 +42,8 @@
// Returns the per-packet framing overhead associated with sending a
// handshake message for |version|.
static QuicByteCount CryptoMessageFramingOverhead(
- QuicTransportVersion version);
+ QuicTransportVersion version,
+ QuicConnectionId connection_id);
// QuicStream implementation
void OnStreamFrame(const QuicStreamFrame& frame) override;
diff --git a/net/third_party/quic/core/quic_framer.cc b/net/third_party/quic/core/quic_framer.cc
index c41a302..d4b93ab 100644
--- a/net/third_party/quic/core/quic_framer.cc
+++ b/net/third_party/quic/core/quic_framer.cc
@@ -782,12 +782,6 @@
frame.token.length();
}
-// static
-size_t QuicFramer::GetVersionNegotiationPacketSize(size_t number_versions) {
- return kPublicFlagsSize + PACKET_8BYTE_CONNECTION_ID +
- number_versions * kQuicVersionSize;
-}
-
// TODO(nharper): Change this method to take a ParsedQuicVersion.
bool QuicFramer::IsSupportedTransportVersion(
const QuicTransportVersion version) const {
@@ -1381,8 +1375,8 @@
}
const QuicData& reset_serialized = reset.GetSerialized();
- size_t len =
- kPublicFlagsSize + PACKET_8BYTE_CONNECTION_ID + reset_serialized.length();
+ size_t len = kPublicFlagsSize + packet.connection_id.length() +
+ reset_serialized.length();
std::unique_ptr<char[]> buffer(new char[len]);
// Endianness is not a concern here, as writer is not going to write integers
// or floating numbers.
@@ -1451,7 +1445,8 @@
return BuildIetfVersionNegotiationPacket(connection_id, versions);
}
DCHECK(!versions.empty());
- size_t len = GetVersionNegotiationPacketSize(versions.size());
+ size_t len = kPublicFlagsSize + connection_id.length() +
+ versions.size() * kQuicVersionSize;
std::unique_ptr<char[]> buffer(new char[len]);
// Endianness is not a concern here, version negotiation packet does not have
// integers or floating numbers.
@@ -1488,7 +1483,7 @@
QUIC_DVLOG(1) << "Building IETF version negotiation packet.";
DCHECK(!versions.empty());
size_t len = kPacketHeaderTypeSize + kConnectionIdLengthSize +
- PACKET_8BYTE_CONNECTION_ID +
+ connection_id.length() +
(versions.size() + 1) * kQuicVersionSize;
std::unique_ptr<char[]> buffer(new char[len]);
QuicDataWriter writer(len, buffer.get());
@@ -1826,12 +1821,11 @@
QuicStringPiece encrypted = encrypted_reader->ReadRemainingPayload();
QuicStringPiece associated_data = GetAssociatedDataFromEncryptedPacket(
version_.transport_version, packet,
- GetIncludedDestinationConnectionIdLength(version_.transport_version,
- *header),
- GetIncludedSourceConnectionIdLength(version_.transport_version, *header),
- header->version_flag, header->nonce != nullptr,
- header->packet_number_length, header->retry_token_length_length,
- header->retry_token.length(), header->length_length);
+ GetIncludedDestinationConnectionIdLength(*header),
+ GetIncludedSourceConnectionIdLength(*header), header->version_flag,
+ header->nonce != nullptr, header->packet_number_length,
+ header->retry_token_length_length, header->retry_token.length(),
+ header->length_length);
size_t decrypted_length = 0;
if (!DecryptPayload(encrypted, associated_data, *header, decrypted_buffer,
@@ -1911,12 +1905,11 @@
QuicStringPiece encrypted = encrypted_reader->ReadRemainingPayload();
QuicStringPiece associated_data = GetAssociatedDataFromEncryptedPacket(
version_.transport_version, packet,
- GetIncludedDestinationConnectionIdLength(version_.transport_version,
- *header),
- GetIncludedSourceConnectionIdLength(version_.transport_version, *header),
- header->version_flag, header->nonce != nullptr,
- header->packet_number_length, header->retry_token_length_length,
- header->retry_token.length(), header->length_length);
+ GetIncludedDestinationConnectionIdLength(*header),
+ GetIncludedSourceConnectionIdLength(*header), header->version_flag,
+ header->nonce != nullptr, header->packet_number_length,
+ header->retry_token_length_length, header->retry_token.length(),
+ header->length_length);
size_t decrypted_length = 0;
if (!DecryptPayload(encrypted, associated_data, *header, decrypted_buffer,
@@ -2158,11 +2151,9 @@
!GetQuicReloadableFlag(quic_use_new_append_connection_id)) {
if (!AppendIetfConnectionId(
header.version_flag, header.destination_connection_id,
- GetIncludedDestinationConnectionIdLength(transport_version(),
- header),
+ GetIncludedDestinationConnectionIdLength(header),
header.source_connection_id,
- GetIncludedSourceConnectionIdLength(transport_version(), header),
- writer)) {
+ GetIncludedSourceConnectionIdLength(header), writer)) {
return false;
}
} else {
@@ -2528,9 +2519,7 @@
header->destination_connection_id_included =
perspective_ == Perspective::IS_CLIENT ? CONNECTION_ID_ABSENT
: CONNECTION_ID_PRESENT;
- if (header->destination_connection_id_included == CONNECTION_ID_ABSENT) {
- header->destination_connection_id = last_serialized_connection_id_;
- }
+ header->source_connection_id_included = CONNECTION_ID_ABSENT;
if (infer_packet_header_type_from_version_ &&
transport_version() > QUIC_VERSION_44 && !(type & FLAGS_FIXED_BIT)) {
set_detailed_error("Fixed bit is 0 in short header.");
@@ -2551,22 +2540,33 @@
if (!ProcessIetfHeaderTypeByte(reader, header)) {
return false;
}
+
+ uint8_t destination_connection_id_length =
+ header->destination_connection_id_included == CONNECTION_ID_PRESENT
+ ? kQuicDefaultConnectionIdLength
+ : 0;
+ uint8_t source_connection_id_length =
+ header->source_connection_id_included == CONNECTION_ID_PRESENT
+ ? kQuicDefaultConnectionIdLength
+ : 0;
if (header->form == IETF_QUIC_LONG_HEADER_PACKET) {
// Read and validate connection ID length.
- uint8_t connection_id_length;
- if (!reader->ReadBytes(&connection_id_length, 1)) {
+ uint8_t connection_id_lengths_byte;
+ if (!reader->ReadBytes(&connection_id_lengths_byte, 1)) {
set_detailed_error("Unable to read ConnectionId length.");
return false;
}
uint8_t dcil =
- (connection_id_length & kDestinationConnectionIdLengthMask) >> 4;
- uint8_t scil = connection_id_length & kSourceConnectionIdLengthMask;
- if ((dcil != 0 &&
- dcil != PACKET_8BYTE_CONNECTION_ID - kConnectionIdLengthAdjustment) ||
- (scil != 0 &&
- scil != PACKET_8BYTE_CONNECTION_ID - kConnectionIdLengthAdjustment) ||
- dcil == scil || (perspective_ == Perspective::IS_CLIENT && scil == 0) ||
- (perspective_ == Perspective::IS_SERVER && dcil == 0)) {
+ (connection_id_lengths_byte & kDestinationConnectionIdLengthMask) >> 4;
+ if (dcil != 0) {
+ dcil += kConnectionIdLengthAdjustment;
+ }
+ uint8_t scil = connection_id_lengths_byte & kSourceConnectionIdLengthMask;
+ if (scil != 0) {
+ scil += kConnectionIdLengthAdjustment;
+ }
+ if (dcil != destination_connection_id_length ||
+ scil != source_connection_id_length) {
// Long header packets received by client must include 8-byte source
// connection ID, and those received by server must include 8-byte
// destination connection ID.
@@ -2575,19 +2575,19 @@
set_detailed_error("Invalid ConnectionId length.");
return false;
}
+ destination_connection_id_length = dcil;
+ source_connection_id_length = scil;
}
// Read connection ID.
- if (header->destination_connection_id_included == CONNECTION_ID_PRESENT &&
- !reader->ReadConnectionId(&header->destination_connection_id,
- kQuicDefaultConnectionIdLength)) {
+ if (!reader->ReadConnectionId(&header->destination_connection_id,
+ destination_connection_id_length)) {
set_detailed_error("Unable to read Destination ConnectionId.");
return false;
}
- if (header->source_connection_id_included == CONNECTION_ID_PRESENT &&
- !reader->ReadConnectionId(&header->source_connection_id,
- kQuicDefaultConnectionIdLength)) {
+ if (!reader->ReadConnectionId(&header->source_connection_id,
+ source_connection_id_length)) {
set_detailed_error("Unable to read Source ConnectionId.");
return false;
}
@@ -2596,6 +2596,9 @@
// Set destination connection ID to source connection ID.
DCHECK_EQ(EmptyQuicConnectionId(), header->destination_connection_id);
header->destination_connection_id = header->source_connection_id;
+ } else if (header->destination_connection_id_included ==
+ CONNECTION_ID_ABSENT) {
+ header->destination_connection_id = last_serialized_connection_id_;
}
return true;
diff --git a/net/third_party/quic/core/quic_framer.h b/net/third_party/quic/core/quic_framer.h
index 51d38c21..8d45a4a0 100644
--- a/net/third_party/quic/core/quic_framer.h
+++ b/net/third_party/quic/core/quic_framer.h
@@ -344,9 +344,6 @@
// Size in bytes for a serialized new token frame
static size_t GetNewTokenFrameSize(const QuicNewTokenFrame& frame);
- // Size in bytes required for a serialized version negotiation packet
- static size_t GetVersionNegotiationPacketSize(size_t number_versions);
-
// Size in bytes required for a serialized stop sending frame.
static size_t GetStopSendingFrameSize(const QuicStopSendingFrame& frame);
diff --git a/net/third_party/quic/core/quic_packet_creator.cc b/net/third_party/quic/core/quic_packet_creator.cc
index 2bd79743..5715123 100644
--- a/net/third_party/quic/core/quic_packet_creator.cc
+++ b/net/third_party/quic/core/quic_packet_creator.cc
@@ -755,14 +755,18 @@
QuicConnectionIdLength QuicPacketCreator::GetDestinationConnectionIdLength()
const {
+ DCHECK(QuicUtils::IsConnectionIdValidForVersion(connection_id_,
+ transport_version()));
return GetDestinationConnectionIdIncluded() == CONNECTION_ID_PRESENT
- ? PACKET_8BYTE_CONNECTION_ID
+ ? static_cast<QuicConnectionIdLength>(connection_id_.length())
: PACKET_0BYTE_CONNECTION_ID;
}
QuicConnectionIdLength QuicPacketCreator::GetSourceConnectionIdLength() const {
+ DCHECK(QuicUtils::IsConnectionIdValidForVersion(connection_id_,
+ transport_version()));
return GetSourceConnectionIdIncluded() == CONNECTION_ID_PRESENT
- ? PACKET_8BYTE_CONNECTION_ID
+ ? static_cast<QuicConnectionIdLength>(connection_id_.length())
: PACKET_0BYTE_CONNECTION_ID;
}
diff --git a/net/third_party/quic/core/quic_packets.cc b/net/third_party/quic/core/quic_packets.cc
index e88ff420..8e10dd2 100644
--- a/net/third_party/quic/core/quic_packets.cc
+++ b/net/third_party/quic/core/quic_packets.cc
@@ -17,41 +17,33 @@
namespace quic {
QuicConnectionIdLength GetIncludedConnectionIdLength(
- QuicTransportVersion version,
QuicConnectionId connection_id,
QuicConnectionIdIncluded connection_id_included) {
DCHECK(connection_id_included == CONNECTION_ID_PRESENT ||
connection_id_included == CONNECTION_ID_ABSENT);
- if (!QuicUtils::VariableLengthConnectionIdAllowedForVersion(version)) {
- return connection_id_included == CONNECTION_ID_PRESENT
- ? PACKET_8BYTE_CONNECTION_ID
- : PACKET_0BYTE_CONNECTION_ID;
- }
return connection_id_included == CONNECTION_ID_PRESENT
? static_cast<QuicConnectionIdLength>(connection_id.length())
: PACKET_0BYTE_CONNECTION_ID;
}
QuicConnectionIdLength GetIncludedDestinationConnectionIdLength(
- QuicTransportVersion version,
const QuicPacketHeader& header) {
return GetIncludedConnectionIdLength(
- version, header.destination_connection_id,
+ header.destination_connection_id,
header.destination_connection_id_included);
}
QuicConnectionIdLength GetIncludedSourceConnectionIdLength(
- QuicTransportVersion version,
const QuicPacketHeader& header) {
- return GetIncludedConnectionIdLength(version, header.source_connection_id,
+ return GetIncludedConnectionIdLength(header.source_connection_id,
header.source_connection_id_included);
}
size_t GetPacketHeaderSize(QuicTransportVersion version,
const QuicPacketHeader& header) {
return GetPacketHeaderSize(
- version, GetIncludedDestinationConnectionIdLength(version, header),
- GetIncludedSourceConnectionIdLength(version, header), header.version_flag,
+ version, GetIncludedDestinationConnectionIdLength(header),
+ GetIncludedSourceConnectionIdLength(header), header.version_flag,
header.nonce != nullptr, header.packet_number_length,
header.retry_token_length_length, header.retry_token.length(),
header.length_length);
@@ -248,8 +240,8 @@
: QuicPacket(buffer,
length,
owns_buffer,
- GetIncludedDestinationConnectionIdLength(version, header),
- GetIncludedSourceConnectionIdLength(version, header),
+ GetIncludedDestinationConnectionIdLength(header),
+ GetIncludedSourceConnectionIdLength(header),
header.version_flag,
header.nonce != nullptr,
header.packet_number_length,
diff --git a/net/third_party/quic/core/quic_packets.h b/net/third_party/quic/core/quic_packets.h
index 06c0539..2ec96aa 100644
--- a/net/third_party/quic/core/quic_packets.h
+++ b/net/third_party/quic/core/quic_packets.h
@@ -35,21 +35,18 @@
// Number of connection ID bytes that are actually included over the wire.
QUIC_EXPORT_PRIVATE QuicConnectionIdLength
-GetIncludedConnectionIdLength(QuicTransportVersion version,
- QuicConnectionId connection_id,
+GetIncludedConnectionIdLength(QuicConnectionId connection_id,
QuicConnectionIdIncluded connection_id_included);
// Number of destination connection ID bytes that are actually included over the
// wire for this particular header.
QUIC_EXPORT_PRIVATE QuicConnectionIdLength
-GetIncludedDestinationConnectionIdLength(QuicTransportVersion version,
- const QuicPacketHeader& header);
+GetIncludedDestinationConnectionIdLength(const QuicPacketHeader& header);
// Number of source connection ID bytes that are actually included over the
// wire for this particular header.
QUIC_EXPORT_PRIVATE QuicConnectionIdLength
-GetIncludedSourceConnectionIdLength(QuicTransportVersion version,
- const QuicPacketHeader& header);
+GetIncludedSourceConnectionIdLength(const QuicPacketHeader& header);
// Size in bytes of the data packet header.
QUIC_EXPORT_PRIVATE size_t GetPacketHeaderSize(QuicTransportVersion version,
diff --git a/net/third_party/quic/core/stateless_rejector.cc b/net/third_party/quic/core/stateless_rejector.cc
index e91ea78..2c8e56d 100644
--- a/net/third_party/quic/core/stateless_rejector.cc
+++ b/net/third_party/quic/core/stateless_rejector.cc
@@ -133,8 +133,8 @@
version_, versions_,
/*use_stateless_rejects=*/true, server_designated_connection_id_, clock_,
random_, compressed_certs_cache_, params_, signed_config_,
- QuicCryptoStream::CryptoMessageFramingOverhead(
- version_.transport_version),
+ QuicCryptoStream::CryptoMessageFramingOverhead(version_.transport_version,
+ connection_id_),
chlo_packet_size_, std::move(cb));
}
diff --git a/net/third_party/quic/test_tools/quic_test_utils.cc b/net/third_party/quic/test_tools/quic_test_utils.cc
index 0cc3c058..0c7151e5 100644
--- a/net/third_party/quic/test_tools/quic_test_utils.cc
+++ b/net/third_party/quic/test_tools/quic_test_utils.cc
@@ -124,10 +124,9 @@
// Re-construct the data packet with data ownership.
return QuicMakeUnique<QuicPacket>(
buffer, length, /* owns_buffer */ true,
- GetIncludedDestinationConnectionIdLength(framer->transport_version(),
- header),
- GetIncludedSourceConnectionIdLength(framer->transport_version(), header),
- header.version_flag, header.nonce != nullptr, header.packet_number_length,
+ GetIncludedDestinationConnectionIdLength(header),
+ GetIncludedSourceConnectionIdLength(header), header.version_flag,
+ header.nonce != nullptr, header.packet_number_length,
header.retry_token_length_length, header.retry_token.length(),
header.length_length);
}
@@ -924,10 +923,9 @@
reinterpret_cast<unsigned char*>(
packet->mutable_data())[GetStartOfEncryptedData(
framer.transport_version(),
- GetIncludedDestinationConnectionIdLength(framer.transport_version(),
- header),
- GetIncludedSourceConnectionIdLength(framer.transport_version(), header),
- version_flag, false /* no diversification nonce */, packet_number_length,
+ GetIncludedDestinationConnectionIdLength(header),
+ GetIncludedSourceConnectionIdLength(header), version_flag,
+ false /* no diversification nonce */, packet_number_length,
VARIABLE_LENGTH_INTEGER_LENGTH_0, 0, VARIABLE_LENGTH_INTEGER_LENGTH_0)] =
0x1F;
