diag: catch a handle handed out while a site still claims it (#101)

A live node reported one close failing with 10038 (WSAENOTSOCK) at the tunnel's pump end. It leaks nothing; what it says is that a site's bookkeeping and its sockets disagree about ownership -- and Windows reissues a handle number only after its last close, so a site whose bookkeeping outlives its socket will eventually close a number that belongs to a live connection. BtfSocketTag now counts a handle arriving while another site still claims it, which names the site to go and read.

2 files changed, 31 insertions, 4 deletions

diff --git a/src/net.cpp b/src/net.cpp
index b56f9f5..3af0189 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -61,8 +61,9 @@ static const char* pszSockSite[SOCK_SITES] = {
 
 static string SockAccountingText()
 {
-    long long nOpened[SOCK_SITES], nClosed[SOCK_SITES], nFailed[SOCK_SITES], nUntagged = 0;
-    BtfSockSnapshot(nOpened, nClosed, nFailed, &nUntagged);
+    long long nOpened[SOCK_SITES], nClosed[SOCK_SITES], nFailed[SOCK_SITES];
+    long long nCollided[SOCK_SITES], nUntagged = 0;
+    BtfSockSnapshot(nOpened, nClosed, nFailed, nCollided, &nUntagged);
 
     long long nUnconn[SOCK_SITES];
     BtfSockLiveUnconnected(nUnconn);
@@ -112,6 +113,19 @@ static string SockAccountingText()
             str += strprintf("  %-24s %s\n", pszSockSite[i], strCodes.c_str());
         }
     }
+
+    // A handle number handed out again while a site still claimed it. Windows
+    // only reuses a number after the last close, so this says that site's
+    // bookkeeping outlived its socket -- and it will close that number again,
+    // on somebody else's connection. Any value above zero is a bug with a name
+    // attached.
+    for (int i = 0; i < SOCK_SITES; i++)
+    {
+        if (nCollided[i] == 0)
+            continue;
+        str += strprintf("  handle reused while %s still claimed it: %lld\n",
+                         pszSockSite[i], nCollided[i]);
+    }
     return str;
 }
 
diff --git a/src/sockcount.h b/src/sockcount.h
index ef99414..96b6efb 100644
--- a/src/sockcount.h
+++ b/src/sockcount.h
@@ -79,10 +79,18 @@ struct BtfSockAccount
     // leak -- see the note at the top of this file -- but it is the one number
     // that would say so if it ever became the leak.
     std::map<int, long long> mapCloseErr[SOCK_SITES];
+    // A handle number arriving from socket()/accept() while some site still
+    // claims it. The operating system only hands a number back out after the
+    // last close of it, so a collision means somebody's bookkeeping outlived
+    // its socket -- and whoever still believes they own that number will
+    // eventually close it, taking down a connection that now belongs to
+    // someone else. Counted against the site that was still holding it, which
+    // is the one to go and read.
+    long long nCollided[SOCK_SITES];
     long long nClosedUntagged;
     BtfSockAccount() : nClosedUntagged(0)
     {
-        for (int i = 0; i < SOCK_SITES; i++) { nOpened[i] = 0; nClosed[i] = 0; nCloseFailed[i] = 0; }
+        for (int i = 0; i < SOCK_SITES; i++) { nOpened[i] = 0; nClosed[i] = 0; nCloseFailed[i] = 0; nCollided[i] = 0; }
     }
 };
 
@@ -102,6 +110,9 @@ inline SOCKET BtfSocketTag(SOCKET hSocket, int nSite)
         return hSocket;
     BtfSockAccount& a = BtfSockAccounting();
     std::lock_guard<std::mutex> lock(a.mtx);
+    std::map<SOCKET, int>::iterator mi = a.mapSite.find(hSocket);
+    if (mi != a.mapSite.end())
+        a.nCollided[mi->second]++;
     a.mapSite[hSocket] = nSite;
     a.nOpened[nSite]++;
     return hSocket;
@@ -151,7 +162,8 @@ inline int BtfCloseSocket(SOCKET hSocket)
 
 // Copy the counters out. Formatting lives in net.cpp, which has strprintf --
 // and which the relay build does not compile.
-inline void BtfSockSnapshot(long long* pOpened, long long* pClosed, long long* pFailed, long long* pUntagged)
+inline void BtfSockSnapshot(long long* pOpened, long long* pClosed, long long* pFailed,
+                            long long* pCollided, long long* pUntagged)
 {
     BtfSockAccount& a = BtfSockAccounting();
     std::lock_guard<std::mutex> lock(a.mtx);
@@ -160,6 +172,7 @@ inline void BtfSockSnapshot(long long* pOpened, long long* pClosed, long long* p
         pOpened[i] = a.nOpened[i];
         pClosed[i] = a.nClosed[i];
         pFailed[i] = a.nCloseFailed[i];
+        pCollided[i] = a.nCollided[i];
     }
     *pUntagged = a.nClosedUntagged;
 }