Create a new connection if a candidate reuses an address

If the remote side sends a candidate with the same address and port with an existing candidate,
but with a new ufrag and pwd, the local client will create a new connection from it
and destroy the old connection with the same remote address.

BUG=webrtc:5915

Review-Url: https://codereview.webrtc.org/2018693002
Cr-Commit-Position: refs/heads/master@{#13000}

webrtc/p2p/base/p2ptransportchannel.cc

@@ -855,40 +855,41 @@ bool P2PTransportChannel::CreateConnection(PortInterface* port,
     return false;
   }
   // Look for an existing connection with this remote address.  If one is not
-  // found, then we can create a new connection for this address.
+  // found or it is found but the existing remote candidate has an older
+  // generation, then we can create a new connection for this address.
   Connection* connection = port->GetConnection(remote_candidate.address());
-  if (connection != NULL) {
-    connection->MaybeUpdatePeerReflexiveCandidate(remote_candidate);
-
-    // It is not legal to try to change any of the parameters of an existing
-    // connection; however, the other side can send a duplicate candidate.
-    if (!remote_candidate.IsEquivalent(connection->remote_candidate())) {
-      LOG(INFO) << "Attempt to change a remote candidate."
-                << " Existing remote candidate: "
-                << connection->remote_candidate().ToString()
-                << "New remote candidate: "
-                << remote_candidate.ToString();
+  if (connection == nullptr ||
+      connection->remote_candidate().generation() <
+          remote_candidate.generation()) {
+    // Don't create a connection if this is a candidate we received in a
+    // message and we are not allowed to make outgoing connections.
+    PortInterface::CandidateOrigin origin = GetOrigin(port, origin_port);
+    if (origin == PortInterface::ORIGIN_MESSAGE && incoming_only_) {
       return false;
     }
-  } else {
-    PortInterface::CandidateOrigin origin = GetOrigin(port, origin_port);
-
-    // Don't create connection if this is a candidate we received in a
-    // message and we are not allowed to make outgoing connections.
-    if (origin == PortInterface::ORIGIN_MESSAGE && incoming_only_)
+    Connection* connection = port->CreateConnection(remote_candidate, origin);
+    if (!connection) {
       return false;
-
-    connection = port->CreateConnection(remote_candidate, origin);
-    if (!connection)
-      return false;
-
+    }
     AddConnection(connection);
-
     LOG_J(LS_INFO, this) << "Created connection with origin=" << origin << ", ("
                          << connections_.size() << " total)";
+    return true;
   }
 
-  return true;
+  // No new connection was created.
+  // Check if this is a peer reflexive candidate.
+  connection->MaybeUpdatePeerReflexiveCandidate(remote_candidate);
+
+  // It is not legal to try to change any of the parameters of an existing
+  // connection; however, the other side can send a duplicate candidate.
+  if (!remote_candidate.IsEquivalent(connection->remote_candidate())) {
+    LOG(INFO) << "Attempt to change a remote candidate."
+              << " Existing remote candidate: "
+              << connection->remote_candidate().ToString()
+              << "New remote candidate: " << remote_candidate.ToString();
+  }
+  return false;
 }
 
 bool P2PTransportChannel::FindConnection(Connection* connection) const {

webrtc/p2p/base/p2ptransportchannel_unittest.cc

@@ -1984,6 +1984,25 @@ class P2PTransportChannelPingTest : public testing::Test,
     last_sent_packet_id_ = last_sent_packet_id;
   }
 
+  void ReceivePingOnConnection(cricket::Connection* conn,
+                               const std::string& remote_ufrag,
+                               int priority) {
+    cricket::IceMessage msg;
+    msg.SetType(cricket::STUN_BINDING_REQUEST);
+    msg.AddAttribute(new cricket::StunByteStringAttribute(
+        cricket::STUN_ATTR_USERNAME,
+        conn->local_candidate().username() + ":" + remote_ufrag));
+    msg.AddAttribute(new cricket::StunUInt32Attribute(
+        cricket::STUN_ATTR_PRIORITY, priority));
+    msg.SetTransactionID(
+        rtc::CreateRandomString(cricket::kStunTransactionIdLength));
+    msg.AddMessageIntegrity(conn->local_candidate().password());
+    msg.AddFingerprint();
+    rtc::ByteBufferWriter buf;
+    msg.Write(&buf);
+    conn->OnReadPacket(buf.Data(), buf.Length(), rtc::CreatePacketTime(0));
+  }
+
   void OnReadyToSend(cricket::TransportChannel* channel) {
     channel_ready_to_send_ = true;
   }
@@ -2453,6 +2472,45 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBasedOnMediaReceived) {
   EXPECT_EQ(conn3, ch.best_connection());
 }
 
+// Test that if a new remote candidate has the same address and port with
+// an old one, it will be used to create a new connection.
+TEST_F(P2PTransportChannelPingTest, TestAddRemoteCandidateWithAddressReuse) {
+  cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+  cricket::P2PTransportChannel ch("candidate reuse", 1, &pa);
+  PrepareChannel(&ch);
+  ch.Connect();
+  ch.MaybeStartGathering();
+  const std::string host_address = "1.1.1.1";
+  const int port_num = 1;
+
+  // kIceUfrag[1] is the current generation ufrag.
+  cricket::Candidate candidate =
+      CreateHostCandidate(host_address, port_num, 1, kIceUfrag[1]);
+  ch.AddRemoteCandidate(candidate);
+  cricket::Connection* conn1 = WaitForConnectionTo(&ch, host_address, port_num);
+  ASSERT_TRUE(conn1 != nullptr);
+  EXPECT_EQ(0u, conn1->remote_candidate().generation());
+
+  // Simply adding the same candidate again won't create a new connection.
+  ch.AddRemoteCandidate(candidate);
+  cricket::Connection* conn2 = GetConnectionTo(&ch, host_address, port_num);
+  EXPECT_EQ(conn1, conn2);
+
+  // Update the ufrag of the candidate and add it again.
+  candidate.set_username(kIceUfrag[2]);
+  ch.AddRemoteCandidate(candidate);
+  conn2 = GetConnectionTo(&ch, host_address, port_num);
+  EXPECT_NE(conn1, conn2);
+  EXPECT_EQ(kIceUfrag[2], conn2->remote_candidate().username());
+  EXPECT_EQ(1u, conn2->remote_candidate().generation());
+
+  // Verify that a ping with the new ufrag can be received on the new
+  // connection.
+  EXPECT_EQ(0, conn2->last_ping_received());
+  ReceivePingOnConnection(conn2, kIceUfrag[2], 1 /* priority */);
+  EXPECT_TRUE(conn2->last_ping_received() > 0);
+}
+
 // When the current best connection is strong, lower-priority connections will
 // be pruned. Otherwise, lower-priority connections are kept.
 TEST_F(P2PTransportChannelPingTest, TestDontPruneWhenWeak) {

webrtc/p2p/base/port.cc

@@ -270,8 +270,19 @@ void Port::AddAddress(const rtc::SocketAddress& address,
   }
 }
 
-void Port::AddConnection(Connection* conn) {
-  connections_[conn->remote_candidate().address()] = conn;
+void Port::AddOrReplaceConnection(Connection* conn) {
+  auto ret = connections_.insert(
+      std::make_pair(conn->remote_candidate().address(), conn));
+  // If there is a different connection on the same remote address, replace
+  // it with the new one and destroy the old one.
+  if (ret.second == false && ret.first->second != conn) {
+    LOG_J(LS_WARNING, this)
+        << "A new connection was created on an existing remote address. "
+        << "New remote candidate: " << conn->remote_candidate().ToString();
+    ret.first->second->SignalDestroyed.disconnect(this);
+    ret.first->second->Destroy();
+    ret.first->second = conn;
+  }
   conn->SignalDestroyed.connect(this, &Port::OnConnectionDestroyed);
   SignalConnectionCreated(this, conn);
 }
@@ -688,6 +699,7 @@ void Port::OnConnectionDestroyed(Connection* conn) {
       connections_.find(conn->remote_candidate().address());
   ASSERT(iter != connections_.end());
   connections_.erase(iter);
+  HandleConnectionDestroyed(conn);
 
   // On the controlled side, ports time out after all connections fail.
   // Note: If a new connection is added after this message is posted, but it

webrtc/p2p/base/port.h

@@ -316,8 +316,10 @@ class Port : public PortInterface, public rtc::MessageHandler,
                   uint32_t relay_preference,
                   bool final);
 
-  // Adds the given connection to the list.  (Deleting removes them.)
-  void AddConnection(Connection* conn);
+  // Adds the given connection to the map keyed by the remote candidate address.
+  // If an existing connection has the same address, the existing one will be
+  // replaced and destroyed.
+  void AddOrReplaceConnection(Connection* conn);
 
   // Called when a packet is received from an unknown address that is not
   // currently a connection.  If this is an authenticated STUN binding request,
@@ -346,6 +348,9 @@ class Port : public PortInterface, public rtc::MessageHandler,
     return rtc::DSCP_NO_CHANGE;
   }
 
+  // Extra work to be done in subclasses when a connection is destroyed.
+  virtual void HandleConnectionDestroyed(Connection* conn) {}
+
  private:
   void Construct();
   // Called when one of our connections deletes itself.

webrtc/p2p/base/port_unittest.cc

@@ -169,7 +169,7 @@ class TestPort : public Port {
   virtual Connection* CreateConnection(const Candidate& remote_candidate,
                                        CandidateOrigin origin) {
     Connection* conn = new ProxyConnection(this, 0, remote_candidate);
-    AddConnection(conn);
+    AddOrReplaceConnection(conn);
     // Set use-candidate attribute flag as this will add USE-CANDIDATE attribute
     // in STUN binding requests.
     conn->set_use_candidate_attr(true);
@@ -2665,3 +2665,31 @@ TEST_F(PortTest, TestSetIceParameters) {
   EXPECT_EQ("ufrag2", candidate.username());
   EXPECT_EQ("password2", candidate.password());
 }
+
+TEST_F(PortTest, TestAddConnectionWithSameAddress) {
+  std::unique_ptr<TestPort> port(
+      CreateTestPort(kLocalAddr1, "ufrag1", "password1"));
+  port->PrepareAddress();
+  EXPECT_EQ(1u, port->Candidates().size());
+  rtc::SocketAddress address("1.1.1.1", 5000);
+  cricket::Candidate candidate(1, "udp", address, 0, "", "", "relay", 0, "");
+  cricket::Connection* conn1 =
+      port->CreateConnection(candidate, Port::ORIGIN_MESSAGE);
+  cricket::Connection* conn_in_use = port->GetConnection(address);
+  EXPECT_EQ(conn1, conn_in_use);
+  EXPECT_EQ(0u, conn_in_use->remote_candidate().generation());
+
+  // Creating with a candidate with the same address again will get us a
+  // different connection with the new candidate.
+  candidate.set_generation(2);
+  cricket::Connection* conn2 =
+      port->CreateConnection(candidate, Port::ORIGIN_MESSAGE);
+  EXPECT_NE(conn1, conn2);
+  conn_in_use = port->GetConnection(address);
+  EXPECT_EQ(conn2, conn_in_use);
+  EXPECT_EQ(2u, conn_in_use->remote_candidate().generation());
+
+  // Make sure the new connection was not deleted.
+  rtc::Thread::Current()->ProcessMessages(300);
+  EXPECT_TRUE(port->GetConnection(address) != nullptr);
+}

webrtc/p2p/base/relayport.cc

@@ -302,7 +302,7 @@ Connection* RelayPort::CreateConnection(const Candidate& address,
   }
 
   Connection * conn = new ProxyConnection(this, index, address);
-  AddConnection(conn);
+  AddOrReplaceConnection(conn);
   return conn;
 }
 

webrtc/p2p/base/stunport.cc

@@ -268,7 +268,7 @@ Connection* UDPPort::CreateConnection(const Candidate& address,
   }
 
   Connection* conn = new ProxyConnection(this, 0, address);
-  AddConnection(conn);
+  AddOrReplaceConnection(conn);
   return conn;
 }
 

webrtc/p2p/base/tcpport.cc

@@ -162,7 +162,7 @@ Connection* TCPPort::CreateConnection(const Candidate& address,
   } else {
     conn = new TCPConnection(this, address);
   }
-  AddConnection(conn);
+  AddOrReplaceConnection(conn);
   return conn;
 }
 

webrtc/p2p/base/turnport.cc

@@ -462,8 +462,7 @@ Connection* TurnPort::CreateConnection(const Candidate& address,
   for (size_t index = 0; index < Candidates().size(); ++index) {
     if (Candidates()[index].type() == RELAY_PORT_TYPE) {
       ProxyConnection* conn = new ProxyConnection(this, index, address);
-      conn->SignalDestroyed.connect(this, &TurnPort::OnConnectionDestroyed);
-      AddConnection(conn);
+      AddOrReplaceConnection(conn);
       return conn;
     }
   }
@@ -1013,7 +1012,7 @@ void TurnPort::DestroyEntryIfNotCancelled(TurnEntry* entry, int64_t timestamp) {
   }
 }
 
-void TurnPort::OnConnectionDestroyed(Connection* conn) {
+void TurnPort::HandleConnectionDestroyed(Connection* conn) {
   // Schedule an event to destroy TurnEntry for the connection, which is
   // already destroyed.
   const rtc::SocketAddress& remote_address = conn->remote_candidate().address();

webrtc/p2p/base/turnport.h

@@ -190,6 +190,7 @@ class TurnPort : public Port {
   typedef std::set<rtc::SocketAddress> AttemptedServerSet;
 
   virtual void OnMessage(rtc::Message* pmsg);
+  virtual void HandleConnectionDestroyed(Connection* conn);
 
   bool CreateTurnClientSocket();
 
@@ -243,7 +244,6 @@ class TurnPort : public Port {
   void DestroyEntryIfNotCancelled(TurnEntry* entry, int64_t timestamp);
   void ScheduleEntryDestruction(TurnEntry* entry);
   void CancelEntryDestruction(TurnEntry* entry);
-  void OnConnectionDestroyed(Connection* conn);
 
   // Destroys the connection with remote address |address|. Returns true if
   // a connection is found and destroyed.