From e700604de4e08bc6bdaaf38a7d9752609a7a9ebf Mon Sep 17 00:00:00 2001 From: Rod Hynes Date: Fri, 23 Aug 2024 11:04:37 -0400 Subject: [PATCH] Fix handling of in-proxy clients in getLoadStats - Avoid potential panic when regionStats[client.peerGeoIPData.Country] is nil - Skip proximate established count for None/Unknown client region - Document potential off-by-one proximate count --- psiphon/server/tunnelServer.go | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/psiphon/server/tunnelServer.go b/psiphon/server/tunnelServer.go index f49d27d6d..124faa114 100644 --- a/psiphon/server/tunnelServer.go +++ b/psiphon/server/tunnelServer.go @@ -1298,8 +1298,18 @@ func (sshServer *sshServer) getLoadStats() ( // exact number of other _network connections_, even from the same // client. // - // - For in-proxy tunnel protocols, the same GeoIP caveats - // (see comments above) apply. + // Futhermore, since client.Locks aren't held between the previous + // loop and this one, it's also possible that the client's + // clientGeoIPData was None in the previous loop and is now not + // None. In this case, the regionStats may not be populated at all + // for the client's current region; if so, the client is skipped. + // This scenario can also result in a proximate undercount by one, + // when the regionStats _is_ populated: this client was counted + // under None, not the current client.peerGeoIPData.Country, so + // the -1 subtracts some _other_ client from the populated regionStats. + // + // - For in-proxy protocols, the accepted proximate metric uses the + // peer GeoIP, which represents the proxy, not the client. stats := regionStats[client.peerGeoIPData.Country]["ALL"] @@ -1316,6 +1326,15 @@ func (sshServer *sshServer) getLoadStats() ( } } + // Handle the in-proxy None and None/not-None cases (and any other + // potential scenario where regionStats[client.clientGeoIPData.Country] + // may not be populated). + if client.clientGeoIPData.Country == GEOIP_UNKNOWN_VALUE || + regionStats[client.clientGeoIPData.Country] == nil { + client.Unlock() + continue + } + stats = regionStats[client.clientGeoIPData.Country]["ALL"] n = stats["established_clients"].(int64) - 1