fix tie-break in duplicate peer connection disconnect logic

ChangeLog

@@ -1,4 +1,5 @@
 
+	* fix tie-break in duplicate peer connection disconnect logic
 	* fix issue with SSL tracker connections left in CLOSE_WAIT state
 	* defer truncating existing files until the first time we write to them
 	* fix issue when receiving a torrent with 0-sized padfiles as magnet link

src/bt_peer_connection.cpp

@@ -3370,7 +3370,7 @@ namespace libtorrent
 					// initiate connections. So, if our peer-id is greater than
 					// the others, we should close the incoming connection,
 					// if not, we should close the outgoing one.
-					if (pid < m_our_peer_id && is_outgoing())
+					if ((pid < m_our_peer_id) == is_outgoing())
 					{
 						p->disconnect(errors::duplicate_peer_id, op_bittorrent);
 					}

src/peer_list.cpp

@@ -640,7 +640,7 @@ namespace libtorrent
 
 			if (i->connection != 0)
 			{
-				bool self_connection =
+				bool const self_connection =
 					i->connection->remote() == c.local_endpoint()
 					|| i->connection->local_endpoint() == c.remote();
 
@@ -671,57 +671,41 @@ namespace libtorrent
 					// disconnect the same one, we need a consistent rule to
 					// select which one.
 
-					bool outgoing1 = c.is_outgoing();
+					bool const outgoing1 = c.is_outgoing();
 
-					// for this, we compare our endpoints (IP and port)
+					// for this, we compare our ports and whoever has the lower port
-					// and whoever has the lower IP,port should be the
+					// should be the one keeping its outgoing connection. Since
-					// one keeping its outgoing connection. Since outgoing
+					// outgoing ports are selected at random by the OS, we need to
-					// ports are selected at random by the OS, we need
+					// be careful to only look at the target end of a connection for
-					// to be careful to only look at the target end of a
+					// the endpoint.
-					// connection for the endpoint.
 
-					int our_port = outgoing1 ? i->connection->local_endpoint().port() : c.local_endpoint().port();
+					int const our_port = outgoing1 ? i->connection->local_endpoint().port() : c.local_endpoint().port();
-					int other_port= outgoing1 ? c.remote().port() : i->connection->remote().port();
+					int const other_port = outgoing1 ? c.remote().port() : i->connection->remote().port();
+
+					// decide which peer connection to disconnect
+					// if the ports are equal, pick on at random
+					bool const disconnect1 = ((our_port < other_port) && !outgoing1)
+						|| ((our_port > other_port) && outgoing1)
+						|| ((our_port == other_port) && randint(2));
 
-					if (our_port < other_port)
-					{
 #ifndef TORRENT_DISABLE_LOGGING
-						c.peer_log(peer_log_alert::info, "DUPLICATE_PEER_RESOLUTION"
+					c.peer_log(peer_log_alert::info, "DUPLICATE_PEER_RESOLUTION"
-							, "\"%d\" < \"%d\"", our_port, other_port);
+						, "our: %d other: %d disconnecting: %s"
-						i->connection->peer_log(peer_log_alert::info, "DUPLICATE_PEER_RESOLUTION"
+						, our_port, other_port, disconnect1 ? "yes" : "no");
-							, "\"%d\" < \"%d\"", our_port, other_port);
+					i->connection->peer_log(peer_log_alert::info, "DUPLICATE_PEER_RESOLUTION"
+						, "our: %d other: %d disconnecting: %s"
+						, our_port, other_port, disconnect1 ? "no" : "yes");
 #endif
 
-						// we should keep our outgoing connection
+					if (disconnect1)
-						if (!outgoing1)
-						{
-							c.disconnect(errors::duplicate_peer_id, op_bittorrent);
-							return false;
-						}
-						TORRENT_ASSERT(m_locked_peer == NULL);
-						m_locked_peer = i;
-						i->connection->disconnect(errors::duplicate_peer_id, op_bittorrent);
-						m_locked_peer = NULL;
-					}
-					else
 					{
-#ifndef TORRENT_DISABLE_LOGGING
+						c.disconnect(errors::duplicate_peer_id, op_bittorrent);
-						c.peer_log(peer_log_alert::info, "DUPLICATE_PEER_RESOLUTION"
+						return false;
-							, "\"%d\" >= \"%d\"", our_port, other_port);
-						i->connection->peer_log(peer_log_alert::info, "DUPLICATE_PEER_RESOLUTION"
-							, "\"%d\" >= \"%d\"", our_port, other_port);
-#endif
-						// they should keep their outgoing connection
-						if (outgoing1)
-						{
-							c.disconnect(errors::duplicate_peer_id, op_bittorrent);
-							return false;
-						}
-						TORRENT_ASSERT(m_locked_peer == NULL);
-						m_locked_peer = i;
-						i->connection->disconnect(errors::duplicate_peer_id, op_bittorrent);
-						m_locked_peer = NULL;
 					}
+					TORRENT_ASSERT(m_locked_peer == NULL);
+					m_locked_peer = i;
+					i->connection->disconnect(errors::duplicate_peer_id, op_bittorrent);
+					m_locked_peer = NULL;
 				}
 			}
 

src/random.cpp

@@ -106,6 +106,7 @@ namespace libtorrent
 
 	boost::uint32_t randint(int i)
 	{
+		TORRENT_ASSERT(i > 1);
 		return random() % i;
 	}
 

test/test_peer_list.cpp

@@ -818,6 +818,45 @@ TORRENT_TEST(double_connection_loose)
 	TEST_EQUAL(con_in->was_disconnected(), false);
 }
 
+// test double connection with identical ports (random)
+TORRENT_TEST(double_connection_random)
+{
+	int in = 0;
+	int out = 0;
+	for (int i = 0; i < 30; ++i)
+	{
+		torrent_state st = init_state(ext_ip);
+		mock_torrent t(&st);
+		st.allow_multiple_connections_per_ip = false;
+		peer_list p(allocator);
+		t.m_p = &p;
+
+		// we are 10.0.0.1 and the other peer is 10.0.0.2
+
+		// our outgoing connection
+		torrent_peer* peer = add_peer(p, st, ep("10.0.0.2", 3000));
+		connect_peer(p, t, st);
+
+		boost::shared_ptr<mock_peer_connection> con_out
+			= static_cast<mock_peer_connection*>(peer->connection)->shared_from_this();
+		con_out->set_local_ep(ep("10.0.0.1", 3000));
+
+		// and the incoming connection
+		boost::shared_ptr<mock_peer_connection> con_in
+			= boost::make_shared<mock_peer_connection>(&t, false, ep("10.0.0.2", 3000));
+		con_in->set_local_ep(ep("10.0.0.1", 3000));
+
+		p.new_connection(*con_in, 0, &st);
+
+		// the rules are documented in peer_list.cpp
+		out += con_out->was_disconnected();
+		in += con_in->was_disconnected();
+	}
+	// we should have gone different ways randomly
+	TEST_CHECK(out > 0);
+	TEST_CHECK(in > 0);
+}
+
 // test double connection (we win)
 TORRENT_TEST(double_connection_win)
 {