STUN and TURN failovers and keepalive should only look for the same address family

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_ice_client.cpp

@@ -67,6 +67,30 @@ static bool IsRemoteAddressOnLocalSubnet( const netadr_t &localAddr, int nPrefix
     return false;
 }
 
+// Interface sockets are bound to a single address family.  A server of a
+// different family can't be reached from them.  Returns the index of the first
+// server after idxCurrent with the same family as localAddr, or -1.
+// bWrapAround allows for keepalive rotation as it wraps around to the
+// start of the list (ex: 0, 1, 2, 3, with idxCurrent as 2 will check 3, 0, 1, then 2).
+// Otherwise it stops at the end (ex: 0, 1, 2, 3, with idxCurrent as 3, will return -1).
+static int FindNextServerForInterface( const std_vector<netadr_t> &vecServers, int idxCurrent, const netadr_t &localAddr, bool bWrapAround )
+{
+    if ( idxCurrent < 0 )
+        return -1;
+
+    const int nServers = len( vecServers );
+    const int nToCheck = bWrapAround ? nServers : nServers - idxCurrent - 1;
+
+    for ( int i = 0; i < nToCheck; ++i )
+    {
+        const int idx = ( idxCurrent + 1 + i ) % nServers;
+        if ( vecServers[idx].GetType() == localAddr.GetType() )
+            return idx;
+    }
+
+    return -1;
+}
+
 
 
 /* After the STUN header are zero or more attributes.  Each attribute
@@ -2090,8 +2114,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 = FindNextServerForInterface( m_vecTURNServers, nTURNServerIdx, pIntf->m_boundAddr, false );
+    if ( nNextTURNServerIdx < 0 )
     {
         // Exhausted all TURN servers. Mark failed.
         pIntf->m_addrTURNServer = info.m_pRequest->m_remoteAddr;
@@ -2303,8 +2327,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 = FindNextServerForInterface( m_vecSTUNServers, nSTUNServerIdx, pIntf->m_boundAddr, false );
+    if ( nNextSTUNServerIdx < 0 )
     {
         // Exhausted all STUN servers.  Mark failed so Think_DiscoverServerReflexiveCandidates
         // does not retry this interface indefinitely.
@@ -2314,6 +2338,7 @@ void CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveCandidate( c
     }
 
     // Try the next server
+    ++TEST_ICE_ctr_srflx_send;
     pIntf->QueueBindRequest( m_vecSTUNServers[nNextSTUNServerIdx], &CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveCandidate, m_nEncoding );
 }
 
@@ -2338,7 +2363,9 @@ void CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveKeepAlive( c
         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 = FindNextServerForInterface( m_vecSTUNServers, nSTUNServerIdx, pIntf->m_boundAddr, true );
+    if ( nNextSTUNServerIdx < 0 )
+        return;
     pIntf->QueueBindRequest( m_vecSTUNServers[ nNextSTUNServerIdx ], &CSteamNetworkingICESession::STUNRequestCallback_ServerReflexiveKeepAlive, m_nEncoding );
 }
 

tests/test_p2p.py

@@ -457,6 +457,19 @@ def ClientServerExpectedFailureTest( server_extra_args=[], client_extra_args=[],
             expected_counters={ 'binding_req_retx': (4, 4) },
             expected_candidates=( {'host': 1}, {'host': 1} ) ) ),
 
+    # The dead STUN server is followed by an IPv6 one, but the adapters are IPv4 only.
+    # Failover must skip servers of the other address family, so there is one srflx
+    # attempt and then discovery gives up.
+    ( 'STUN unavailable, other server is a different address family',
+      _nat( _SRV_INT, _SRV_GW, 'full-cone' ),
+      _nat( _CLI_INT, _CLI_GW, 'full-cone' ),
+      dict( stun='%s,[%s]:%d' % ( _DEAD_SERVER, g_stun_ipv6, g_stun_port ), turn=None,
+            expected_counters={
+                'srflx_send':         (1, 1),
+                '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)',
@@ -498,6 +511,19 @@ def ClientServerExpectedFailureTest( server_extra_args=[], client_extra_args=[],
             },
             expected_candidates=( _CAND_NAT_NO_TURN, _CAND_NAT_NO_TURN ) ) ),
 
+    # The dead TURN server is followed by an IPv6 one, but the adapters are IPv4 only.
+    # Failover must skip servers of the other address family, so there is one allocate
+    # attempt and then relay discovery gives up.
+    ( 'TURN unreachable, other server is a different address family (symmetric NAT)',
+      _nat( _SRV_INT, _SRV_GW, 'symmetric' ),
+      _nat( _CLI_INT, _CLI_GW, 'symmetric' ),
+      dict( turn='%s,[%s]:%d' % ( _DEAD_SERVER, g_stun_ipv6, g_stun_port ),
+            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).