tunnel: give both pumps shared ownership of the sockets they close

PumpIn's wrapper closed pumpEnd and the rendezvous socket the moment it
exited, without waiting for PumpOut, which then called shutdown() on both
at its own exit. Which of the two got there first decided whether that
shutdown addressed a live descriptor or a number the kernel had already
handed back out -- in which case it tears down an unrelated connection.
The inbound pump wins that race whenever the rendezvous socket dies before
the app socket, which is the ordinary shape of a relay restart or a far
side hanging up.

The failure path added in 0b566b4 has the same hole. Its catch shuts down
rv to wake an inbound pump it assumes is parked in RvReadN. PumpIn does
not park when the rendezvous connection is already dead: it returns on its
first length read and its wrapper closes both descriptors. A refused
pthread_create and a dead rv are not independent -- both are symptoms of a
loaded process -- so that is the likely case, not the exotic one.

Both descriptors now belong to a shared_ptr the two pumps hold for the
whole of their bodies, so the close happens after the last pump has
stopped touching them and no shutdown can precede it. appEnd is unchanged:
the caller gets it on success, StartTunnel closes it on every failure
path, no pump ever touches it.

0b566b4's message says of that catch that "closing those descriptors from
the failure path instead would race it". True as far as it goes, but it
reads as though what the code did instead was safe, and it was not -- the
shutdown raced the same close. There was no ordering there to prefer, only
a missing owner.

Measured against the real code with the relay in a separate process, every
close() and shutdown() traced and pthread_create refused on demand:

  normal teardown, 25 tunnels    29 shutdowns on a closed fd -> 0
  refused outbound spawn, 25     25 shutdowns on a closed fd -> 0

Close counts are identical before and after (8 and 5 per tunnel, every one
accounted for) and live descriptors return to their starting count, so the
defect was only ever the ordering, never a leak or a double close.

1 files changed, 68 insertions, 17 deletions

diff --git a/src/btftunnel.cpp b/src/btftunnel.cpp
index 2b3f818..4acca1a 100644
--- a/src/btftunnel.cpp
+++ b/src/btftunnel.cpp
@@ -26,6 +26,8 @@ typedef int SOCKET;
 #include "sockcount.h"
 #include <cstring>
 #include <cstdio>
+#include <memory>
+#include <new>
 #include <vector>
 #include <thread>
 #include <system_error>
@@ -121,9 +123,44 @@ static void PumpIn(SOCKET pumpEnd, RvSocket rv, std::vector<unsigned char> key)
     shutdown((SOCKET)rv, SD_BOTH);
 }
 
+// The pump end and the rendezvous socket, owned jointly by whoever is still
+// using them. Closed once, by the last reference to go.
+//
+// Joint ownership rather than "the inbound pump owns the cleanup", because
+// either pump can reach its exit first and BOTH touch BOTH descriptors on the
+// way out -- PumpOut ends in shutdown(rv), shutdown(pumpEnd) and PumpIn ends in
+// the mirror image. A single owner therefore has no exit at which the other is
+// guaranteed to be finished. Holding a reference for the whole body of each
+// pump gives it one: the close cannot happen until the last pump has stopped
+// touching the descriptors, so no shutdown() can land on a number that has
+// already been closed and handed back out by the kernel to an unrelated
+// connection.
+struct TunnelEnds
+{
+    SOCKET   pumpEnd;
+    RvSocket rv;
+    TunnelEnds(SOCKET p, RvSocket r) : pumpEnd(p), rv(r) {}
+    ~TunnelEnds() { BtfCloseSocket(pumpEnd); RvClose(rv); }
+private:
+    TunnelEnds(const TunnelEnds&);
+    TunnelEnds& operator=(const TunnelEnds&);
+};
+
 // Start the two pumps and hand back the app-facing socket. When both pumps
 // finish, close the pump end and the rendezvous socket.
 //
+// That sentence used to be a comment describing something the code did not do.
+// The inbound pump closed both descriptors as soon as it exited, without
+// waiting for the outbound pump, which then called shutdown() on both at its
+// own exit. Whether that shutdown hit a live descriptor or a closed one came
+// down to which pump got there first, and the rendezvous socket dying before
+// the app socket -- a relay restart, the far side hanging up -- is precisely
+// the order that puts the inbound pump ahead. Measured over 25 such teardowns
+// against the real code: 29 shutdowns landed on an already-closed descriptor.
+//
+// appEnd is not part of this. It is handed to the caller on success and closed
+// here on every failure path; no pump ever touches it.
+//
 // Every tunnel is two threads, and a tunnel is one peer connection (a .btf
 // dial or accept, a pool miner, a participant's pool session) -- so the
 // thread count scales with peer count, which is attacker-influenced. When
@@ -139,41 +176,55 @@ static btf_socket_t StartTunnel(RvSocket rv, const unsigned char shared[32])
 
     std::vector<unsigned char> key(shared, shared + 32);
 
-    // The inbound pump starts first because it owns the cleanup: it closes
-    // pumpEnd and rv when it exits. That ownership is what makes the second
-    // spawn's failure path below race-free.
+    // make_shared, not shared_ptr(new ...): it allocates before it constructs,
+    // so it either comes back owning both descriptors or throws having taken
+    // ownership of neither. The two-step form can run the deleter and then
+    // throw, which would close both here and again below.
+    std::shared_ptr<TunnelEnds> ends;
+    try
+    {
+        ends = std::make_shared<TunnelEnds>(pumpEnd, rv);
+    }
+    catch (const std::bad_alloc&)
+    {
+        fprintf(stderr, "btf tunnel: out of memory starting tunnel, tunnel dropped\n");
+        BtfCloseSocket(pumpEnd);
+        BtfCloseSocket(appEnd);
+        RvClose(rv);
+        return INVALID_SOCKET;
+    }
+
     try
     {
-        std::thread([pumpEnd, rv, key]() {
-            PumpIn(pumpEnd, rv, key);
-            BtfCloseSocket(pumpEnd);
-            RvClose(rv);
-        }).detach();
+        std::thread([ends, key]() { PumpIn(ends->pumpEnd, ends->rv, key); }).detach();
     }
     catch (const std::system_error& e)
     {
-        // Nothing is running yet, so closing everything here is safe.
+        // Nothing is running yet: `ends` is the only reference, and returning
+        // drops it, which is what closes pumpEnd and rv.
         fprintf(stderr, "btf tunnel: cannot spawn inbound pump (%s), tunnel dropped\n",
                 e.what());
-        BtfCloseSocket(pumpEnd);
         BtfCloseSocket(appEnd);
-        RvClose(rv);
         return INVALID_SOCKET;
     }
 
     try
     {
-        std::thread(PumpOut, pumpEnd, rv, key).detach();
+        std::thread([ends, key]() { PumpOut(ends->pumpEnd, ends->rv, key); }).detach();
     }
     catch (const std::system_error& e)
     {
-        // The inbound pump is parked in RvReadN(rv). shutdown() wakes it, and
-        // it closes pumpEnd and rv itself -- closing them here instead would
-        // race exactly that, and a reused descriptor is how an unrelated
-        // socket gets closed.
+        // The inbound pump may be parked in RvReadN(rv) or may already have
+        // exited -- when the spawn is refused because the process is loaded,
+        // the rendezvous socket is often dead for the same reason, and then
+        // PumpIn returns on its first length read. Either way this thread
+        // still holds a reference, so both descriptors are still open: the
+        // shutdown wakes the pump if it is parked and is harmless if it is
+        // not. The close belongs to whichever reference goes last.
         fprintf(stderr, "btf tunnel: cannot spawn outbound pump (%s), tunnel dropped\n",
                 e.what());
-        shutdown((SOCKET)rv, SD_BOTH);
+        shutdown((SOCKET)ends->rv, SD_BOTH);
+        shutdown(ends->pumpEnd, SD_BOTH);
         BtfCloseSocket(appEnd);
         return INVALID_SOCKET;
     }