diff --git a/ChangeLog b/ChangeLog index 5e26424d2..0ed5fc3c8 100644 --- a/ChangeLog +++ b/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 diff --git a/src/bt_peer_connection.cpp b/src/bt_peer_connection.cpp index eb5119050..90260be21 100644 --- a/src/bt_peer_connection.cpp +++ b/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); } diff --git a/src/peer_list.cpp b/src/peer_list.cpp index 4c4167107..6e5691b77 100644 --- a/src/peer_list.cpp +++ b/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) - // and whoever has the lower IP,port should be the - // one keeping its outgoing connection. Since outgoing - // ports are selected at random by the OS, we need - // to be careful to only look at the target end of a - // connection for the endpoint. + // for this, we compare our ports and whoever has the lower port + // should be the one keeping its outgoing connection. Since + // outgoing ports are selected at random by the OS, we need to + // be careful to only look at the target end of a connection for + // the endpoint. - int 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 our_port = outgoing1 ? i->connection->local_endpoint().port() : c.local_endpoint().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" - , "\"%d\" < \"%d\"", our_port, other_port); - i->connection->peer_log(peer_log_alert::info, "DUPLICATE_PEER_RESOLUTION" - , "\"%d\" < \"%d\"", our_port, other_port); + c.peer_log(peer_log_alert::info, "DUPLICATE_PEER_RESOLUTION" + , "our: %d other: %d disconnecting: %s" + , our_port, other_port, disconnect1 ? "yes" : "no"); + 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 (!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 + if (disconnect1) { -#ifndef TORRENT_DISABLE_LOGGING - c.peer_log(peer_log_alert::info, "DUPLICATE_PEER_RESOLUTION" - , "\"%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; + 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; } } diff --git a/src/random.cpp b/src/random.cpp index c7b5d0f42..aebadebf0 100644 --- a/src/random.cpp +++ b/src/random.cpp @@ -106,6 +106,7 @@ namespace libtorrent boost::uint32_t randint(int i) { + TORRENT_ASSERT(i > 1); return random() % i; } diff --git a/test/test_peer_list.cpp b/test/test_peer_list.cpp index ba3a36aeb..851645062 100644 --- a/test/test_peer_list.cpp +++ b/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 con_out + = static_cast(peer->connection)->shared_from_this(); + con_out->set_local_ep(ep("10.0.0.1", 3000)); + + // and the incoming connection + boost::shared_ptr con_in + = boost::make_shared(&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) {