From dfdc47378f69237f8aff5743f9cd137784dca3e1 Mon Sep 17 00:00:00 2001 From: Fletcher Dunn Date: Mon, 1 Nov 2021 15:46:03 -0700 Subject: [PATCH] Fix bug if low level datagram send fails. This could leak reliable messages and fail to honor fundamental protocol guarantees about reliable message delivery(!). This can happen but is incredibly rare for ordinary UDP, if, e.g we have filled the send buffer. it is much more likely with an P2P (ICE) connection. P4:6868520 --- .../clientlib/steamnetworkingsockets_connections.h | 1 + .../clientlib/steamnetworkingsockets_snp.cpp | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h index 91cea67..7360f0a 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h @@ -862,6 +862,7 @@ private: uint8 *SNP_SerializeAckBlocks( const SNPPacketSerializeHelper &helper, uint8 *pOut, const uint8 *pOutEnd ); uint8 *SNP_SerializeStopWaitingFrame( SNPPacketSerializeHelper &helper, uint8 *pOut ); + void SNP_QueueReliableSegmentsForRetry( SNPInFlightPacket_t &pkt, int64 nPktNumForDebug, const char *pszDebug ); void SetState( ESteamNetworkingConnectionState eNewState, SteamNetworkingMicroseconds usecNow ); ESteamNetworkingConnectionState m_eConnectionState; diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp index daeae6e..1bc60b9 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp @@ -1408,6 +1408,11 @@ void CSteamNetworkConnectionBase::SNP_SenderProcessPacketNack( int64 nPktNum, SN m_senderState.MaybeCheckReliable(); // Schedule any reliable segments for retry + SNP_QueueReliableSegmentsForRetry( pkt, nPktNum, pszDebug ); +} + +void CSteamNetworkConnectionBase::SNP_QueueReliableSegmentsForRetry( SNPInFlightPacket_t &pkt, int64 nPktNumForDebug, const char *pszDebug ) +{ for ( uint16 hSeg: pkt.m_vecReliableSegments ) { SNPSendReliableSegment_t &relSeg = m_senderState.m_listSentReliableSegments[ hSeg ]; @@ -1444,7 +1449,7 @@ void CSteamNetworkConnectionBase::SNP_SenderProcessPacketNack( int64 nPktNum, SN SpewMsgGroup( m_connectionConfig.m_LogLevel_PacketDecode.Get(), "[%s] pkt %lld %s, queueing retry of reliable range [%lld,%lld)\n", GetDescription(), - nPktNum, + nPktNumForDebug, pszDebug, relSeg.begin(), relSeg.begin() + cbSeg ); @@ -1830,7 +1835,12 @@ bool CSteamNetworkConnectionBase::SNP_SendPacket( CConnectionTransport *pTranspo } } if ( nBytesSent <= 0 ) - return false; // FIXME - We have transfered ownership of some messages to the segments in helper.m_insertInflightPkt. We are gonna leak these? + { + // We have potentially transfered ownership of some reliable messages + // to the segments in helper.m_insertInflightPkt. We must not leak those! + SNP_QueueReliableSegmentsForRetry( helper.m_insertInflightPkt.second, 0, "Send fail" ); + return false; + } // We sent a packet. Track it auto pairInsertResult = m_senderState.m_mapInFlightPacketsByPktNum.insert( helper.m_insertInflightPkt );