diff --git a/quic/QuicConstants.h b/quic/QuicConstants.h index f9cb8efb9..e1b69698d 100644 --- a/quic/QuicConstants.h +++ b/quic/QuicConstants.h @@ -30,9 +30,8 @@ constexpr uint16_t kDefaultUDPSendPacketLen = (kDefaultV4UDPSendPacketLen < kDefaultV6UDPSendPacketLen ? kDefaultV4UDPSendPacketLen : kDefaultV6UDPSendPacketLen); -// This is the default if the transport parameter for max packet size is missing -// or zero. -constexpr uint16_t kDefaultMaxUDPPayload = 4096; +// The max we will tolerate a peer's max_packet_size to be. +constexpr uint16_t kDefaultMaxUDPPayload = 1452; // This is the minimum the max_packet_size transport parameter is allowed to be, // per the spec. Note this actually refers to the max UDP payload size, not the diff --git a/quic/client/state/ClientStateMachine.cpp b/quic/client/state/ClientStateMachine.cpp index cc1368814..fdde024d7 100644 --- a/quic/client/state/ClientStateMachine.cpp +++ b/quic/client/state/ClientStateMachine.cpp @@ -92,7 +92,7 @@ void processServerInitialParams( // TODO Validate active_connection_id_limit if (!packetSize || *packetSize == 0) { - packetSize = kDefaultMaxUDPPayload; + packetSize = kDefaultUDPSendPacketLen; } if (*packetSize < kMinMaxUDPPayload) { throw QuicTransportException( @@ -134,8 +134,10 @@ void processServerInitialParams( ackDelayExponent.value_or(kDefaultAckDelayExponent); // TODO: udpSendPacketLen should also be limited by PMTU if (conn.transportSettings.canIgnorePathMTU) { - conn.udpSendPacketLen = - std::min(*packetSize, kDefaultMaxUDPPayload); + if (*packetSize > kDefaultMaxUDPPayload) { + *packetSize = kDefaultUDPSendPacketLen; + } + conn.udpSendPacketLen = *packetSize; } // Currently no-op for a client; it doesn't issue connection ids diff --git a/quic/client/test/QuicClientTransportTest.cpp b/quic/client/test/QuicClientTransportTest.cpp index e994b8f24..ec038077f 100644 --- a/quic/client/test/QuicClientTransportTest.cpp +++ b/quic/client/test/QuicClientTransportTest.cpp @@ -1228,7 +1228,7 @@ class FakeOneRttHandshakeLayer : public FizzClientHandshake { bool connected_{false}; QuicVersion negotiatedVersion{QuicVersion::MVFST}; - uint64_t maxRecvPacketSize{2 * 1024}; + uint64_t maxRecvPacketSize{kDefaultMaxUDPPayload}; uint64_t maxInitialStreamData{kDefaultStreamWindowSize}; uint64_t connWindowSize{kDefaultConnectionWindowSize}; uint64_t maxInitialStreamsBidi{std::numeric_limits::max()}; diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index 520235681..8a1b6f633 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -125,7 +125,7 @@ void processClientInitialParams( // TODO Validate active_connection_id_limit if (!packetSize || *packetSize == 0) { - packetSize = kDefaultMaxUDPPayload; + packetSize = kDefaultUDPSendPacketLen; } if (*packetSize < kMinMaxUDPPayload) { throw QuicTransportException( @@ -165,8 +165,10 @@ void processClientInitialParams( ackDelayExponent.value_or(kDefaultAckDelayExponent); // TODO: udpSendPacketLen should also be limited by PMTU if (conn.transportSettings.canIgnorePathMTU) { - conn.udpSendPacketLen = - std::min(*packetSize, kDefaultMaxUDPPayload); + if (*packetSize > kDefaultMaxUDPPayload) { + *packetSize = kDefaultUDPSendPacketLen; + } + conn.udpSendPacketLen = *packetSize; } conn.peerActiveConnectionIdLimit = diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index 299552764..37a8023c8 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -181,7 +181,7 @@ class FakeServerHandshake : public ServerHandshake { QuicServerConnectionState& conn_; bool chloSync_{false}; bool cfinSync_{false}; - uint64_t maxRecvPacketSize{2 * 1024}; + uint64_t maxRecvPacketSize{kDefaultMaxUDPPayload}; bool allowZeroRttKeys_{false}; std::vector sourceAddrs_; folly::Optional clientActiveConnectionIdLimit_; @@ -3645,6 +3645,7 @@ TEST_F( server->getConn().transportSettings.limitedCwndInMss * originalUdpSize + server->getConn().transportSettings.limitedCwndInMss * server->getConn().udpSendPacketLen; + EXPECT_NE(originalUdpSize, server->getConn().udpSendPacketLen); EXPECT_EQ(*server->getNonConstConn().writableBytesLimit, expectedLen); std::vector indices = getQLogEventIndices(QLogEventType::TransportStateUpdate, qLogger); @@ -3658,6 +3659,16 @@ TEST_F( } } +TEST_F(QuicUnencryptedServerTransportTest, MaxReceivePacketSizeTooLarge) { + getFakeHandshakeLayer()->allowZeroRttKeys(); + auto originalUdpSize = server->getConn().udpSendPacketLen; + fakeHandshake->maxRecvPacketSize = 4096; + setupClientReadCodec(); + recvClientHello(); + EXPECT_NE(originalUdpSize, server->getConn().udpSendPacketLen); + EXPECT_EQ(server->getConn().udpSendPacketLen, kDefaultUDPSendPacketLen); +} + TEST_F(QuicUnencryptedServerTransportTest, TestGarbageData) { auto qLogger = std::make_shared(VantagePoint::Server); server->getNonConstConn().qLogger = qLogger;