WebRTC Bug 4865

Bug 4865: even without STUN/TURN, as long as the peer is on the open internet, the connectivity should work. This is actually a regression even for hangouts.

We need to issue the 0.0.0.0 candidate into Port::candidates_ and filter it out later. The reason is that when we create connection, we need a local candidate to match the remote candidate.

The same connection later will be updated with the prflx local candidate once the STUN ping response is received.

BUG=webrtc:4865
R=juberti@webrtc.org

Review URL: https://codereview.webrtc.org/1274013002 .

Cr-Commit-Position: refs/heads/master@{#9708}

webrtc/base/virtualsocket_unittest.cc

@@ -149,6 +149,41 @@ class VirtualSocketServerTest : public testing::Test {
     }
   }
 
+  // Test a client can bind to the any address, and all sent packets will have
+  // the default route as the source address. Also, it can receive packets sent
+  // to the default route.
+  void TestDefaultRoute(const IPAddress& default_route) {
+    ss_->SetDefaultRoute(default_route);
+
+    // Create client1 bound to the any address.
+    AsyncSocket* socket =
+        ss_->CreateAsyncSocket(default_route.family(), SOCK_DGRAM);
+    socket->Bind(EmptySocketAddressWithFamily(default_route.family()));
+    SocketAddress client1_any_addr = socket->GetLocalAddress();
+    EXPECT_TRUE(client1_any_addr.IsAnyIP());
+    TestClient* client1 = new TestClient(new AsyncUDPSocket(socket));
+
+    // Create client2 bound to the default route.
+    AsyncSocket* socket2 =
+        ss_->CreateAsyncSocket(default_route.family(), SOCK_DGRAM);
+    socket2->Bind(SocketAddress(default_route, 0));
+    SocketAddress client2_addr = socket2->GetLocalAddress();
+    EXPECT_FALSE(client2_addr.IsAnyIP());
+    TestClient* client2 = new TestClient(new AsyncUDPSocket(socket2));
+
+    // Client1 sends to client2, client2 should see the default route as
+    // client1's address.
+    SocketAddress client1_addr;
+    EXPECT_EQ(6, client1->SendTo("bizbaz", 6, client2_addr));
+    EXPECT_TRUE(client2->CheckNextPacket("bizbaz", 6, &client1_addr));
+    EXPECT_EQ(client1_addr,
+              SocketAddress(default_route, client1_any_addr.port()));
+
+    // Client2 can send back to client1's default route address.
+    EXPECT_EQ(3, client2->SendTo("foo", 3, client1_addr));
+    EXPECT_TRUE(client1->CheckNextPacket("foo", 3, &client2_addr));
+  }
+
   void BasicTest(const SocketAddress& initial_addr) {
     AsyncSocket* socket = ss_->CreateAsyncSocket(initial_addr.family(),
                                                  SOCK_DGRAM);
@@ -791,6 +826,18 @@ TEST_F(VirtualSocketServerTest, basic_v6) {
   BasicTest(ipv6_test_addr);
 }
 
+TEST_F(VirtualSocketServerTest, TestDefaultRoute_v4) {
+  IPAddress ipv4_default_addr(0x01020304);
+  TestDefaultRoute(ipv4_default_addr);
+}
+
+TEST_F(VirtualSocketServerTest, TestDefaultRoute_v6) {
+  IPAddress ipv6_default_addr;
+  EXPECT_TRUE(
+      IPFromString("2401:fa00:4:1000:be30:5bff:fee5:c3", &ipv6_default_addr));
+  TestDefaultRoute(ipv6_default_addr);
+}
+
 TEST_F(VirtualSocketServerTest, connect_v4) {
   ConnectTest(kIPv4AnyAddress);
 }

webrtc/base/virtualsocketserver.cc

@@ -17,6 +17,7 @@
 #include <map>
 #include <vector>
 
+#include "webrtc/base/checks.h"
 #include "webrtc/base/common.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/physicalsocketserver.h"
@@ -661,7 +662,22 @@ VirtualSocket* VirtualSocketServer::LookupBinding(const SocketAddress& addr) {
   SocketAddress normalized(addr.ipaddr().Normalized(),
                            addr.port());
   AddressMap::iterator it = bindings_->find(normalized);
-  return (bindings_->end() != it) ? it->second : NULL;
+  if (it != bindings_->end()) {
+    return it->second;
+  }
+
+  IPAddress default_ip = GetDefaultRoute(addr.ipaddr().family());
+  if (!IPIsUnspec(default_ip) && addr.ipaddr() == default_ip) {
+    // If we can't find a binding for the packet which is sent to the interface
+    // corresponding to the default route, it should match a binding with the
+    // correct port to the any address.
+    SocketAddress sock_addr =
+        EmptySocketAddressWithFamily(addr.ipaddr().family());
+    sock_addr.SetPort(addr.port());
+    return LookupBinding(sock_addr);
+  }
+
+  return nullptr;
 }
 
 int VirtualSocketServer::Unbind(const SocketAddress& addr,
@@ -866,8 +882,18 @@ void VirtualSocketServer::AddPacketToNetwork(VirtualSocket* sender,
   // Find the delay for crossing the many virtual hops of the network.
   uint32 transit_delay = GetRandomTransitDelay();
 
+  // When the incoming packet is from a binding of the any address, translate it
+  // to the default route here such that the recipient will see the default
+  // route.
+  SocketAddress sender_addr = sender->local_addr_;
+  IPAddress default_ip = GetDefaultRoute(sender_addr.ipaddr().family());
+  if (sender_addr.IsAnyIP() && !IPIsUnspec(default_ip)) {
+    sender_addr.SetIP(default_ip);
+  }
+
   // Post the packet as a message to be delivered (on our own thread)
-  Packet* p = new Packet(data, data_size, sender->local_addr_);
+  Packet* p = new Packet(data, data_size, sender_addr);
+
   uint32 ts = TimeAfter(send_delay + transit_delay);
   if (ordered) {
     // Ensure that new packets arrive after previous ones
@@ -1080,4 +1106,22 @@ bool VirtualSocketServer::CanInteractWith(VirtualSocket* local,
   return false;
 }
 
+IPAddress VirtualSocketServer::GetDefaultRoute(int family) {
+  if (family == AF_INET) {
+    return default_route_v4_;
+  }
+  if (family == AF_INET6) {
+    return default_route_v6_;
+  }
+  return IPAddress();
+}
+void VirtualSocketServer::SetDefaultRoute(const IPAddress& from_addr) {
+  DCHECK(!IPIsAny(from_addr));
+  if (from_addr.family() == AF_INET) {
+    default_route_v4_ = from_addr;
+  } else if (from_addr.family() == AF_INET6) {
+    default_route_v6_ = from_addr;
+  }
+}
+
 }  // namespace rtc

webrtc/base/virtualsocketserver.h

@@ -38,6 +38,11 @@ class VirtualSocketServer : public SocketServer, public sigslot::has_slots<> {
 
   SocketServer* socketserver() { return server_; }
 
+  // The default route indicates which local address to use when a socket is
+  // bound to the 'any' address, e.g. 0.0.0.0.
+  IPAddress GetDefaultRoute(int family);
+  void SetDefaultRoute(const IPAddress& from_addr);
+
   // Limits the network bandwidth (maximum bytes per second).  Zero means that
   // all sends occur instantly.  Defaults to 0.
   uint32 bandwidth() const { return bandwidth_; }
@@ -224,6 +229,9 @@ class VirtualSocketServer : public SocketServer, public sigslot::has_slots<> {
   AddressMap* bindings_;
   ConnectionMap* connections_;
 
+  IPAddress default_route_v4_;
+  IPAddress default_route_v6_;
+
   uint32 bandwidth_;
   uint32 network_capacity_;
   uint32 send_buffer_capacity_;
@@ -248,9 +256,6 @@ class VirtualSocket : public AsyncSocket, public MessageHandler {
   SocketAddress GetLocalAddress() const override;
   SocketAddress GetRemoteAddress() const override;
 
-  // Used by server sockets to set the local address without binding.
-  void SetLocalAddress(const SocketAddress& addr);
-
   // Used by TurnPortTest to mimic a case where proxy returns local host address
   // instead of the original one TurnPort was bound against. Please see WebRTC
   // issue 3927 for more detail.
@@ -297,6 +302,9 @@ class VirtualSocket : public AsyncSocket, public MessageHandler {
   int SendUdp(const void* pv, size_t cb, const SocketAddress& addr);
   int SendTcp(const void* pv, size_t cb);
 
+  // Used by server sockets to set the local address without binding.
+  void SetLocalAddress(const SocketAddress& addr);
+
   VirtualSocketServer* server_;
   int family_;
   int type_;

webrtc/p2p/client/basicportallocator.cc

@@ -21,6 +21,7 @@
 #include "webrtc/p2p/base/tcpport.h"
 #include "webrtc/p2p/base/turnport.h"
 #include "webrtc/p2p/base/udpport.h"
+#include "webrtc/base/checks.h"
 #include "webrtc/base/common.h"
 #include "webrtc/base/helpers.h"
 #include "webrtc/base/logging.h"
@@ -440,26 +441,32 @@ void BasicPortAllocatorSession::OnCandidateReady(
   if (data->complete())
     return;
 
-  // Send candidates whose protocol is enabled.
-  std::vector<Candidate> candidates;
   ProtocolType pvalue;
-  bool candidate_allowed_to_send = CheckCandidateFilter(c);
+  bool candidate_signalable = CheckCandidateFilter(c);
-  if (StringToProto(c.protocol().c_str(), &pvalue) &&
+  bool candidate_pairable =
-      data->sequence()->ProtocolEnabled(pvalue) &&
+      candidate_signalable ||
-      candidate_allowed_to_send) {
+      (c.address().IsAnyIP() &&
-    candidates.push_back(c);
+       (port->SharedSocket() || c.protocol() == TCP_PROTOCOL_NAME));
-  }
+  bool candidate_protocol_enabled =
+      StringToProto(c.protocol().c_str(), &pvalue) &&
+      data->sequence()->ProtocolEnabled(pvalue);
 
-  if (!candidates.empty()) {
+  if (candidate_signalable && candidate_protocol_enabled) {
+    std::vector<Candidate> candidates;
+    candidates.push_back(c);
     SignalCandidatesReady(this, candidates);
   }
 
-  // Moving to READY state as we have atleast one candidate from the port.
+  // Port has been made ready. Nothing to do here.
-  // Since this port has atleast one candidate we should forward this port
+  if (data->ready()) {
-  // to listners, to allow connections from this port.
+    return;
-  // Also we should make sure that candidate gathered from this port is allowed
+  }
-  // to send outside.
+
-  if (!data->ready() && candidate_allowed_to_send) {
+  // Move the port to the READY state, either because we have a usable candidate
+  // from the port, or simply because the port is bound to the any address and
+  // therefore has no host candidate. This will trigger the port to start
+  // creating candidate pairs (connections) and issue connectivity checks.
+  if (candidate_pairable) {
     data->set_ready();
     SignalPortReady(this, port);
   }
@@ -508,9 +515,10 @@ void BasicPortAllocatorSession::OnProtocolEnabled(AllocationSequence* seq,
       if (!CheckCandidateFilter(potentials[i]))
         continue;
       ProtocolType pvalue;
-      if (!StringToProto(potentials[i].protocol().c_str(), &pvalue))
+      bool candidate_protocol_enabled =
-        continue;
+          StringToProto(potentials[i].protocol().c_str(), &pvalue) &&
-      if (pvalue == proto) {
+          pvalue == proto;
+      if (candidate_protocol_enabled) {
         candidates.push_back(potentials[i]);
       }
     }

webrtc/p2p/client/portallocator_unittest.cc

@@ -88,7 +88,7 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> {
         fss_(new rtc::FirewallSocketServer(vss_.get())),
         ss_scope_(fss_.get()),
         nat_factory_(vss_.get(), kNatUdpAddr, kNatTcpAddr),
-        nat_socket_factory_(&nat_factory_),
+        nat_socket_factory_(new rtc::BasicPacketSocketFactory(&nat_factory_)),
         stun_server_(cricket::TestStunServer::Create(Thread::Current(),
                                                      kStunAddr)),
         relay_server_(Thread::Current(), kRelayUdpIntAddr, kRelayUdpExtAddr,
@@ -110,9 +110,20 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> {
   void AddInterface(const SocketAddress& addr) {
     network_manager_.AddInterface(addr);
   }
+  void AddInterfaceAsDefaultRoute(const SocketAddress& addr) {
+    AddInterface(addr);
+    // When a binding comes from the any address, the |addr| will be used as the
+    // srflx address.
+    vss_->SetDefaultRoute(addr.ipaddr());
+  }
   bool SetPortRange(int min_port, int max_port) {
     return allocator_->SetPortRange(min_port, max_port);
   }
+  // No STUN/TURN configuration.
+  void ResetWithNoServers() {
+    allocator_.reset(new cricket::BasicPortAllocator(&network_manager_));
+    allocator_->set_step_delay(cricket::kMinimumStepDelay);
+  }
   void ResetWithNatServer(const rtc::SocketAddress& stun_server) {
     nat_server_.reset(new rtc::NATServer(
         rtc::NAT_OPEN_CONE, vss_.get(), kNatUdpAddr, kNatTcpAddr, vss_.get(),
@@ -123,7 +134,19 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> {
       stun_servers.insert(stun_server);
     }
     allocator_.reset(new cricket::BasicPortAllocator(
-        &network_manager_, &nat_socket_factory_, stun_servers));
+        &network_manager_, nat_socket_factory_.get(), stun_servers));
+    allocator().set_step_delay(cricket::kMinimumStepDelay);
+  }
+
+  void ResetWithStunServer(const rtc::SocketAddress& stun_server) {
+    nat_socket_factory_.reset(new rtc::BasicPacketSocketFactory());
+
+    ServerAddresses stun_servers;
+    if (!stun_server.IsNil()) {
+      stun_servers.insert(stun_server);
+    }
+    allocator_.reset(new cricket::BasicPortAllocator(
+        &network_manager_, nat_socket_factory_.get(), stun_servers));
     allocator().set_step_delay(cricket::kMinimumStepDelay);
   }
 
@@ -232,27 +255,51 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> {
     }
   }
 
-  void CheckDisableAdapterEnumeration() {
+  // This function starts the port/address gathering and check the existence of
+  // candidates as specified. When |expect_stun_candidate| is true,
+  // |stun_candidate_addr| carries the expected reflective address, which is
+  // also the related address for TURN candidate if it is expected. Otherwise,
+  // it should be ignore.
+  void CheckDisableAdapterEnumeration(
+      uint32 total_ports,
+      const rtc::IPAddress& stun_candidate_addr,
+      const rtc::IPAddress& relay_candidate_udp_transport_addr,
+      const rtc::IPAddress& relay_candidate_tcp_transport_addr) {
     EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
-    session_->set_flags(cricket::PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION);
+    session_->set_flags(cricket::PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
+                        cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET);
+    allocator().set_allow_tcp_listen(false);
     session_->StartGettingPorts();
     EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout);
 
-    // Only 2 candidates as local UDP/TCP are all 0s and get trimmed out.
+    uint32 total_candidates = 0;
-    EXPECT_EQ(2U, candidates_.size());
+    if (!IPIsUnspec(stun_candidate_addr)) {
-    EXPECT_EQ(2U, ports_.size());  // One stunport and one turnport.
+      ++total_candidates;
-
       EXPECT_PRED5(CheckCandidate, candidates_[0],
                    cricket::ICE_CANDIDATE_COMPONENT_RTP, "stun", "udp",
-                 rtc::SocketAddress(kNatUdpAddr.ipaddr(), 0));
+                   rtc::SocketAddress(stun_candidate_addr, 0));
       EXPECT_EQ(
           rtc::EmptySocketAddressWithFamily(candidates_[0].address().family()),
           candidates_[0].related_address());
-
+    }
+    if (!IPIsUnspec(relay_candidate_udp_transport_addr)) {
+      ++total_candidates;
       EXPECT_PRED5(CheckCandidate, candidates_[1],
                    cricket::ICE_CANDIDATE_COMPONENT_RTP, "relay", "udp",
-                 rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0));
+                   rtc::SocketAddress(relay_candidate_udp_transport_addr, 0));
-    EXPECT_EQ(kNatUdpAddr.ipaddr(), candidates_[1].related_address().ipaddr());
+      EXPECT_EQ(stun_candidate_addr, candidates_[1].related_address().ipaddr());
+    }
+    if (!IPIsUnspec(relay_candidate_tcp_transport_addr)) {
+      ++total_candidates;
+      EXPECT_PRED5(CheckCandidate, candidates_[2],
+                   cricket::ICE_CANDIDATE_COMPONENT_RTP, "relay", "udp",
+                   rtc::SocketAddress(relay_candidate_tcp_transport_addr, 0));
+      EXPECT_EQ(stun_candidate_addr, candidates_[2].related_address().ipaddr());
+    }
+
+    EXPECT_EQ(total_candidates, candidates_.size());
+    EXPECT_EQ(total_ports, ports_.size());
   }
 
  protected:
@@ -293,7 +340,7 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> {
   rtc::SocketServerScope ss_scope_;
   rtc::scoped_ptr<rtc::NATServer> nat_server_;
   rtc::NATSocketFactory nat_factory_;
-  rtc::BasicPacketSocketFactory nat_socket_factory_;
+  rtc::scoped_ptr<rtc::BasicPacketSocketFactory> nat_socket_factory_;
   rtc::scoped_ptr<cricket::TestStunServer> stun_server_;
   cricket::TestRelayServer relay_server_;
   cricket::TestTurnServer turn_server_;
@@ -455,24 +502,68 @@ TEST_F(PortAllocatorTest, TestGetAllPortsNoAdapters) {
 
 // Test that we should only get STUN and TURN candidates when adapter
 // enumeration is disabled.
-TEST_F(PortAllocatorTest, TestDisableAdapterEnumeration) {
+TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationBehindNat) {
   AddInterface(kClientAddr);
   // GTURN is not configured here.
   ResetWithNatServer(kStunAddr);
   AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress());
-
+  // Expect to see 3 ports: STUN, TURN/UDP and TCP ports, and both STUN and
-  CheckDisableAdapterEnumeration();
+  // TURN/UDP candidates.
+  CheckDisableAdapterEnumeration(3U, kNatUdpAddr.ipaddr(),
+                                 kTurnUdpExtAddr.ipaddr(), rtc::IPAddress());
 }
 
-// Test that even with multiple interfaces, the result should be only 1 Stun
+// Test that even with multiple interfaces, the result should still be one STUN
-// candidate since we bind to any address (i.e. all 0s).
+// and one TURN candidate since we bind to any address (i.e. all 0s).
-TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationMultipleInterfaces) {
+TEST_F(PortAllocatorTest,
+       TestDisableAdapterEnumerationBehindNatMultipleInterfaces) {
   AddInterface(kPrivateAddr);
   AddInterface(kPrivateAddr2);
   ResetWithNatServer(kStunAddr);
   AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress());
+  // Expect to see 3 ports: STUN, TURN/UDP and TCP ports, and both STUN and
+  // TURN/UDP candidates.
+  CheckDisableAdapterEnumeration(3U, kNatUdpAddr.ipaddr(),
+                                 kTurnUdpExtAddr.ipaddr(), rtc::IPAddress());
+}
 
-  CheckDisableAdapterEnumeration();
+// Test that we should get STUN, TURN/UDP and TURN/TCP candidates when a
+// TURN/TCP server is specified.
+TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationBehindNatWithTcp) {
+  turn_server_.AddInternalSocket(kTurnTcpIntAddr, cricket::PROTO_TCP);
+  AddInterface(kClientAddr);
+  // GTURN is not configured here.
+  ResetWithNatServer(kStunAddr);
+  AddTurnServers(kTurnUdpIntAddr, kTurnTcpIntAddr);
+  // Expect to see 4 ports - STUN, TURN/UDP, TURN/TCP and TCP port. STUN,
+  // TURN/UDP, and TURN/TCP candidates.
+  CheckDisableAdapterEnumeration(4U, kNatUdpAddr.ipaddr(),
+                                 kTurnUdpExtAddr.ipaddr(),
+                                 kTurnUdpExtAddr.ipaddr());
+}
+
+// Test that we should only get STUN and TURN candidates when adapter
+// enumeration is disabled. Since the endpoint is not behind NAT, the srflx
+// address should be the public client interface.
+TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationWithoutNat) {
+  AddInterfaceAsDefaultRoute(kClientAddr);
+  ResetWithStunServer(kStunAddr);
+  AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress());
+  // Expect to see 3 ports: STUN, TURN/UDP and TCP ports, but only both STUN and
+  // TURN candidates. The STUN candidate should have kClientAddr as srflx
+  // address, and TURN candidate with kClientAddr as the related address.
+  CheckDisableAdapterEnumeration(3U, kClientAddr.ipaddr(),
+                                 kTurnUdpExtAddr.ipaddr(), rtc::IPAddress());
+}
+
+// Test that when adapter enumeration is disabled, for endpoints without
+// STUN/TURN specified, no candidate is generated.
+TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationWithoutNatOrServers) {
+  AddInterfaceAsDefaultRoute(kClientAddr);
+  ResetWithNoServers();
+  // Expect to see 2 ports: STUN and TCP ports, but no candidate.
+  CheckDisableAdapterEnumeration(2U, rtc::IPAddress(), rtc::IPAddress(),
+                                 rtc::IPAddress());
 }
 
 // Disable for asan, see