From 1e819e3896f882ca0e948680d22b67d344e09521 Mon Sep 17 00:00:00 2001 From: kwy404 Date: Fri, 25 Sep 2026 03:04:32 -0300 Subject: [PATCH] ICE: Dedupe servers and track the server index on STUN and TURN requests When the same STUN or TURN server address was listed more than once, the failover path found the current server with index_of() on its address, so it kept landing on the first copy and retried the same dead server forever. Skip duplicate addresses when the STUN and TURN server lists are built (for TURN the first entry keeps its credentials, since they are looked up by address), and store the server index on each request so binding, allocate and keepalive failover always advance to the next entry. Fixes #437 --- .../steamnetworkingsockets_ice_client.cpp | 48 +++++++++++-------- .../steamnetworkingsockets_ice_client.h | 6 ++- tests/test_p2p.py | 23 +++++++++ 3 files changed, 57 insertions(+), 20 deletions(-) diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.cpp b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.cpp index bd9648a6..722e3e5e 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.cpp +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.cpp @@ -1081,7 +1081,7 @@ bool ICESessionInterface::QueueTURNRequest( uint32 nMsgType, int nEncoding, cons return true; } -void ICESessionInterface::QueueAllocateRequest( const netadr_t &addrTURNServer, RecvSTUNPacketCallback_t cb, int nEncoding ) +void ICESessionInterface::QueueAllocateRequest( int nTURNServerIdx, RecvSTUNPacketCallback_t cb, int nEncoding ) { // REQUESTED-TRANSPORT: UDP (IANA protocol 17), protocol byte + 3 RFFU bytes uint32 uTransport = htonl( 17u << 24 ); @@ -1090,7 +1090,8 @@ void ICESessionInterface::QueueAllocateRequest( const netadr_t &addrTURNServer, reqTransport.m_nLength = 4; reqTransport.m_pData = &uTransport; - QueueTURNRequest( k_nTURN_AllocateRequest, nEncoding, addrTURNServer, cb, &reqTransport, 1 ); + if ( QueueTURNRequest( k_nTURN_AllocateRequest, nEncoding, m_session.m_vecTURNServers[ nTURNServerIdx ], cb, &reqTransport, 1 ) ) + m_pPendingSTUNRequest->m_nServerIdx = nTURNServerIdx; } void ICESessionInterface::QueueRefreshRequest( RecvSTUNPacketCallback_t cb, int nEncoding ) @@ -1211,7 +1212,10 @@ CSteamNetworkingICESession::CSteamNetworkingICESession( const ICESessionConfig& { netadr_t adr; SteamNetworkingIPAddrToNetAdr( adr, ip ); - m_vecSTUNServers.push_back( adr ); + + // Skip duplicates, so a dead server listed twice is only tried once. + if ( index_of( m_vecSTUNServers, adr ) < 0 ) + m_vecSTUNServers.push_back( adr ); } } } @@ -1235,6 +1239,11 @@ CSteamNetworkingICESession::CSteamNetworkingICESession( const ICESessionConfig& { netadr_t adr; SteamNetworkingIPAddrToNetAdr( adr, ip ); + + // Skip duplicates, keeping the first entry's credentials. Credentials + // are looked up by address, so a later duplicate could never use its own. + if ( index_of( m_vecTURNServers, adr ) >= 0 ) + continue; m_vecTURNServers.push_back( adr ); TURNCredentials cred; cred.m_strUsername = pszUsername; @@ -1957,12 +1966,13 @@ void CSteamNetworkingICESession::Think_DiscoverServerReflexiveCandidates() continue; // Find the first STUN server matching this interface's address family. - for ( const netadr_t &srv : m_vecSTUNServers ) + for ( int idx = 0; idx < len( m_vecSTUNServers ); ++idx ) { - if ( srv.GetType() == pIntf->m_boundAddr.GetType() ) + if ( m_vecSTUNServers[idx].GetType() == pIntf->m_boundAddr.GetType() ) { ++TEST_ICE_ctr_srflx_send; - pIntf->QueueBindRequest( srv, &CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveCandidate, m_nEncoding | kSTUNPacketEncodingFlags_MappedAddress ); + pIntf->QueueBindRequest( m_vecSTUNServers[idx], &CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveCandidate, m_nEncoding | kSTUNPacketEncodingFlags_MappedAddress ); + pIntf->m_pPendingSTUNRequest->m_nServerIdx = idx; break; } } @@ -1985,11 +1995,11 @@ void CSteamNetworkingICESession::Think_DiscoverRelayCandidate() continue; // Find the first TURN server matching this interface's address family. - for ( const netadr_t &srv : m_vecTURNServers ) + for ( int idx = 0; idx < len( m_vecTURNServers ); ++idx ) { - if ( srv.GetType() == pIntf->m_boundAddr.GetType() ) + if ( m_vecTURNServers[idx].GetType() == pIntf->m_boundAddr.GetType() ) { - pIntf->QueueAllocateRequest( srv, &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding ); + pIntf->QueueAllocateRequest( idx, &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding ); break; } } @@ -2067,7 +2077,7 @@ void CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay( const RecvST CUtlNetAdrRender( info.m_pRequest->m_remoteAddr ).String(), cred.m_strUsername.c_str() ); // Re-queue the allocate with credentials. - pIntf->QueueAllocateRequest( info.m_pRequest->m_remoteAddr, &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding ); + pIntf->QueueAllocateRequest( info.m_pRequest->m_nServerIdx, &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding ); return; } } @@ -2081,9 +2091,8 @@ void CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay( const RecvST } // Timed out -- try the next TURN server if available. - const int nTURNServerIdx = index_of( m_vecTURNServers, info.m_pRequest->m_remoteAddr ); - const int nNextTURNServerIdx = nTURNServerIdx + 1; - if ( nTURNServerIdx < 0 || nNextTURNServerIdx >= len( m_vecTURNServers ) ) + const int nNextTURNServerIdx = info.m_pRequest->m_nServerIdx + 1; + if ( nNextTURNServerIdx >= len( m_vecTURNServers ) ) { // Exhausted all TURN servers. Mark failed. pIntf->m_addrTURNServer = info.m_pRequest->m_remoteAddr; @@ -2092,7 +2101,7 @@ void CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay( const RecvST } // Try the next TURN server. - pIntf->QueueAllocateRequest( m_vecTURNServers[nNextTURNServerIdx], &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding ); + pIntf->QueueAllocateRequest( nNextTURNServerIdx, &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding ); } void CSteamNetworkingICESession::STUNRequestCallback_RefreshAllocation( const RecvSTUNPktInfo_t &info ) @@ -2294,9 +2303,8 @@ void CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveCandidate( c } // Timed out to this STUN server -- try the next one if available. - const int nSTUNServerIdx = index_of( m_vecSTUNServers, info.m_pRequest->m_remoteAddr ); - const int nNextSTUNServerIdx = nSTUNServerIdx + 1; - if ( nSTUNServerIdx < 0 || nNextSTUNServerIdx >= len( m_vecSTUNServers ) ) + const int nNextSTUNServerIdx = info.m_pRequest->m_nServerIdx + 1; + if ( nNextSTUNServerIdx >= len( m_vecSTUNServers ) ) { // Exhausted all STUN servers. Mark failed so Think_DiscoverServerReflexiveCandidates // does not retry this interface indefinitely. @@ -2307,6 +2315,7 @@ void CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveCandidate( c // Try the next server pIntf->QueueBindRequest( m_vecSTUNServers[nNextSTUNServerIdx], &CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveCandidate, m_nEncoding ); + pIntf->m_pPendingSTUNRequest->m_nServerIdx = nNextSTUNServerIdx; } void CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveKeepAlive( const RecvSTUNPktInfo_t &info ) @@ -2329,9 +2338,9 @@ void CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveKeepAlive( c if ( m_vecSTUNServers.empty() ) return; - const int nSTUNServerIdx = std::max( 0, index_of( m_vecSTUNServers, info.m_pRequest->m_remoteAddr ) ); - const int nNextSTUNServerIdx = ( nSTUNServerIdx + 1 ) % len( m_vecSTUNServers ); + const int nNextSTUNServerIdx = ( info.m_pRequest->m_nServerIdx + 1 ) % len( m_vecSTUNServers ); pIntf->QueueBindRequest( m_vecSTUNServers[ nNextSTUNServerIdx ], &CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveKeepAlive, m_nEncoding ); + pIntf->m_pPendingSTUNRequest->m_nServerIdx = nNextSTUNServerIdx; } void CSteamNetworkingICESession::UpdateKeepalive( ICESessionInterface *pIntf ) @@ -2342,6 +2351,7 @@ void CSteamNetworkingICESession::UpdateKeepalive( ICESessionInterface *pIntf ) return; pIntf->QueueBindRequest( pIntf->m_addrSTUNServer, &CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveKeepAlive, m_nEncoding ); + pIntf->m_pPendingSTUNRequest->m_nServerIdx = std::max( 0, index_of( m_vecSTUNServers, pIntf->m_addrSTUNServer ) ); } void CSteamNetworkingICESession::Think_KeepAliveOnCandidates( SteamNetworkingMicroseconds usecNow ) diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.h b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.h index b36a53b7..d9dfe2f3 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.h +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.h @@ -130,7 +130,7 @@ namespace SteamNetworkingSocketsLib { void QueueBindRequest( const netadr_t &addrSTUNServer, RecvSTUNPacketCallback_t cb, int nEncoding ); // Send a TURN Allocate request - void QueueAllocateRequest( const netadr_t &addrTURNServer, RecvSTUNPacketCallback_t cb, int nEncoding ); + void QueueAllocateRequest( int nTURNServerIdx, RecvSTUNPacketCallback_t cb, int nEncoding ); // Send a TURN Refresh request to keep the allocation alive void QueueRefreshRequest( RecvSTUNPacketCallback_t cb, int nEncoding ); @@ -250,6 +250,10 @@ namespace SteamNetworkingSocketsLib { // Not used for other request types int m_nTURNPermissionRevision; + // For STUN binding and TURN Allocate requests, the index of the server in the + // session's STUN or TURN server list. Used to fail over to the next entry. + int m_nServerIdx = 0; + // Serialize the packet and start the retry loop. void Queue( uint32 nMessageType, int nEncoding, netadr_t remoteAddr, RecvSTUNPacketCallback_t cb, STUNAttribute *pExtraAttrs = nullptr, int nExtraAttrs = 0 ); diff --git a/tests/test_p2p.py b/tests/test_p2p.py index b9671c8c..2c5122a2 100755 --- a/tests/test_p2p.py +++ b/tests/test_p2p.py @@ -446,6 +446,17 @@ def ClientServerExpectedFailureTest( server_extra_args=[], client_extra_args=[], }, expected_candidates=( {'host': 1}, {'host': 1} ) ) ), + # Same as above, but the dead STUN server is listed twice (like several hostnames + # resolving to one IP). The duplicate is skipped, so the server is tried once and + # then discovery gives up, instead of failing over to the same entry forever. The + # longer connection timeout leaves room for more attempts, which must not happen. + ( 'STUN unavailable, duplicate server entries', + _nat( _SRV_INT, _SRV_GW, 'full-cone' ) + [ '--timeout-ms', '16000' ], + _nat( _CLI_INT, _CLI_GW, 'full-cone' ) + [ '--timeout-ms', '16000' ], + dict( stun='%s,%s' % ( _DEAD_SERVER, _DEAD_SERVER ), turn=None, + expected_counters={ 'binding_req_retx': (4, 4) }, + expected_candidates=( {'host': 1}, {'host': 1} ) ) ), + # TURN not configured: symmetric NAT requires relay; without it the connection must fail. # Connectivity checks to srflx candidates retransmit 4 times before giving up. ( 'TURN not configured (symmetric NAT)', @@ -475,6 +486,18 @@ def ClientServerExpectedFailureTest( server_extra_args=[], client_extra_args=[], }, expected_candidates=( _CAND_NAT_NO_TURN, _CAND_NAT_NO_TURN ) ) ), + # Same as above, but the dead TURN server is listed twice. The duplicate is + # skipped, so there is one allocate attempt and then relay discovery gives up. + ( 'TURN unreachable, duplicate server entries (symmetric NAT)', + _nat( _SRV_INT, _SRV_GW, 'symmetric' ) + [ '--timeout-ms', '16000' ], + _nat( _CLI_INT, _CLI_GW, 'symmetric' ) + [ '--timeout-ms', '16000' ], + dict( turn='%s,%s' % ( _DEAD_SERVER, _DEAD_SERVER ), + expected_counters={ + 'allocate_send': (1, 1), + 'allocate_retx': (4, 4), + }, + expected_candidates=( _CAND_NAT_NO_TURN, _CAND_NAT_NO_TURN ) ) ), + # TURN wrong password: the server sends a 401 challenge; the client retries with a # bad HMAC (wrong password) and gets a second 401, marking relay as failed. # allocate_send=2: initial (no auth) + one retry (wrong credentials).