From f1e3f2ca54d4bfeb5b756bb5ed01447ca8e39d10 Mon Sep 17 00:00:00 2001
From: Sarah Jamie Lewis <sarah@openprivacy.ca>
Date: Tue, 21 Sep 2021 15:46:45 -0700
Subject: [PATCH] Correctly resolve multiple inbound connections to
 complete-closure

Connect() now prevents Open if there are *any connections* under the assumption
that one of them will be resolved valid. (Before connect allowed Open on any
error, this was a bug that would occasionally be tripped by multiple Inbounds)
---
 networks/tor/BaseOnionService.go | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/networks/tor/BaseOnionService.go b/networks/tor/BaseOnionService.go
index 9361cb8..c3c6383 100644
--- a/networks/tor/BaseOnionService.go
+++ b/networks/tor/BaseOnionService.go
@@ -73,27 +73,44 @@ func (s *BaseOnionService) WaitForCapabilityOrClose(cid string, name tapir.Capab
 
 		// If We have 2 connections for the same hostname...
 		if err != nil {
-			log.Debugf("found duplicate connections for %v <-> %v", s.id.Hostname(), cid)
+			log.Debugf("found duplicate connections for %v <-> %v %v", s.id.Hostname(), cid, err)
 			// By convention the lowest lexicographical hostname purges all their outbounds to the higher
 			// hostname
 			// Which should only leave a single connection remaining (as we dedupe on connect too)
 			// This does allow people to attempt to guarantee being the outbound connection but due to the bidirectional
 			// authentication this should never result in an advantage in the protocol.
+			inboundCount := 0
 			if s.id.Hostname() < cid {
-				// Close all output connections to connection
+				// Close all outbound connections to connection
 				s.connections.Range(func(key, value interface{}) bool {
 					connection := value.(tapir.Connection)
 					if connection.Hostname() == cid {
 						if !connection.IsClosed() && connection.IsOutbound() {
 							connection.Close()
+						} else if !connection.IsClosed() && !connection.IsOutbound() {
+							inboundCount++
 						}
 					}
 					return true
 				})
+
+				// This shouldn't happen honestly, but if it does then it can cause an infinite check here
+				if inboundCount > 1 {
+					s.connections.Range(func(key, value interface{}) bool {
+						connection := value.(tapir.Connection)
+						if connection.Hostname() == cid {
+							if !connection.IsClosed() {
+								connection.Close()
+							}
+						}
+						return true
+					})
+					return nil, errors.New("multiple inbound connections found and closed; the only resolution to this is to close them all and try connecting again")
+				}
 			}
 		}
 
-		// We had a connection but it didn't have a cpability...try again
+		// We had a connection but it didn't have a capability...try again
 		conn, err = s.GetConnection(cid)
 	}
 	// There are no active connections with that hostname
@@ -126,8 +143,8 @@ func (s *BaseOnionService) GetConnection(hostname string) (tapir.Connection, err
 
 // Connect initializes a new outbound connection to the given peer, using the defined Application
 func (s *BaseOnionService) Connect(hostname string, app tapir.Application) (bool, error) {
-	_, err := s.GetConnection(hostname)
-	if err == nil {
+	currconn, _ := s.GetConnection(hostname)
+	if currconn != nil {
 		// Note: This check is not 100% reliable. And we may end up with two connections between peers
 		// This can happen when a client connects to a server as the server is connecting to the client
 		// Because at the start of the connection the server cannot derive the true hostname of the client until it
-- 
2.25.1