Fixing logic for using android_setsocknetwork() with bind().

If android_setsocknetwork() is available, and it fails, then bind()
should *not* be called, and an error should be returned.

If it succeeds, then bind should be called, but with an "any" address.

This is to prevent cases where sockets are sent with a source address
that doesn't match the network interface they're sent on. See bug below.

This CL also changes "NetworkBinderResults" to an enum class, and
renames it to "NetworkBinderResult".

BUG=webrtc:7026

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

webrtc/base/networkmonitor.h

@@ -19,13 +19,12 @@ namespace rtc {
 
 class IPAddress;
 
-// Error values are negative.
+enum class NetworkBindingResult {
-enum NetworkBindingResults {
+  SUCCESS = 0,   // No error
-  NETWORK_BIND_SUCCESS = 0,   // No error
+  FAILURE = -1,  // Generic error
-  NETWORK_BIND_FAILURE = -1,  // Generic error
+  NOT_IMPLEMENTED = -2,
-  NETWORK_BIND_NOT_IMPLEMENTED = -2,
+  ADDRESS_NOT_FOUND = -3,
-  NETWORK_BIND_ADDRESS_NOT_FOUND = -3,
+  NETWORK_CHANGED = -4
-  NETWORK_BIND_NETWORK_CHANGED = -4
 };
 
 enum AdapterType {
@@ -44,7 +43,9 @@ class NetworkBinderInterface {
   // packets on the socket |socket_fd| will be sent via that network.
   // This is needed because some operating systems (like Android) require a
   // special bind call to put packets on a non-default network interface.
-  virtual int BindSocketToNetwork(int socket_fd, const IPAddress& address) = 0;
+  virtual NetworkBindingResult BindSocketToNetwork(
+      int socket_fd,
+      const IPAddress& address) = 0;
   virtual ~NetworkBinderInterface() {}
 };
 

webrtc/base/physicalsocketserver.cc

@@ -190,8 +190,42 @@ SocketAddress PhysicalSocket::GetRemoteAddress() const {
 }
 
 int PhysicalSocket::Bind(const SocketAddress& bind_addr) {
+  SocketAddress copied_bind_addr = bind_addr;
+  // If a network binder is available, use it to bind a socket to an interface
+  // instead of bind(), since this is more reliable on an OS with a weak host
+  // model.
+  if (ss_->network_binder()) {
+    NetworkBindingResult result =
+        ss_->network_binder()->BindSocketToNetwork(s_, bind_addr.ipaddr());
+    if (result == NetworkBindingResult::SUCCESS) {
+      // Since the network binder handled binding the socket to the desired
+      // network interface, we don't need to (and shouldn't) include an IP in
+      // the bind() call; bind() just needs to assign a port.
+      copied_bind_addr.SetIP(GetAnyIP(copied_bind_addr.ipaddr().family()));
+    } else if (result == NetworkBindingResult::NOT_IMPLEMENTED) {
+      LOG(LS_INFO) << "Can't bind socket to network because "
+                      "network binding is not implemented for this OS.";
+    } else {
+      if (bind_addr.IsLoopbackIP()) {
+        // If we couldn't bind to a loopback IP (which should only happen in
+        // test scenarios), continue on. This may be expected behavior.
+        LOG(LS_VERBOSE) << "Binding socket to loopback address "
+                        << bind_addr.ipaddr().ToString()
+                        << " failed; result: " << static_cast<int>(result);
+      } else {
+        LOG(LS_WARNING) << "Binding socket to network address "
+                        << bind_addr.ipaddr().ToString()
+                        << " failed; result: " << static_cast<int>(result);
+        // If a network binding was attempted and failed, we should stop here
+        // and not try to use the socket. Otherwise, we may end up sending
+        // packets with an invalid source address.
+        // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=7026
+        return -1;
+      }
+    }
+  }
   sockaddr_storage addr_storage;
-  size_t len = bind_addr.ToSockAddrStorage(&addr_storage);
+  size_t len = copied_bind_addr.ToSockAddrStorage(&addr_storage);
   sockaddr* addr = reinterpret_cast<sockaddr*>(&addr_storage);
   int err = ::bind(s_, addr, static_cast<int>(len));
   UpdateLastError();
@@ -201,14 +235,6 @@ int PhysicalSocket::Bind(const SocketAddress& bind_addr) {
     dbg_addr_.append(GetLocalAddress().ToString());
   }
 #endif
-  if (ss_->network_binder()) {
-    int result =
-        ss_->network_binder()->BindSocketToNetwork(s_, bind_addr.ipaddr());
-    if (result < 0) {
-      LOG(LS_INFO) << "Binding socket to network address "
-                   << bind_addr.ipaddr().ToString() << " result " << result;
-    }
-  }
   return err;
 }
 

webrtc/base/physicalsocketserver_unittest.cc

@@ -14,6 +14,7 @@
 
 #include "webrtc/base/gunit.h"
 #include "webrtc/base/logging.h"
+#include "webrtc/base/networkmonitor.h"
 #include "webrtc/base/physicalsocketserver.h"
 #include "webrtc/base/socket_unittest.h"
 #include "webrtc/base/testutils.h"
@@ -85,6 +86,18 @@ class FakePhysicalSocketServer : public PhysicalSocketServer {
   PhysicalSocketTest* test_;
 };
 
+class FakeNetworkBinder : public NetworkBinderInterface {
+ public:
+  NetworkBindingResult BindSocketToNetwork(int, const IPAddress&) override {
+    return result_;
+  }
+
+  void set_result(NetworkBindingResult result) { result_ = result; }
+
+ private:
+  NetworkBindingResult result_ = NetworkBindingResult::SUCCESS;
+};
+
 class PhysicalSocketTest : public SocketTest {
  public:
   // Set flag to simluate failures when calling "::accept" on a AsyncSocket.
@@ -415,6 +428,32 @@ TEST_F(PhysicalSocketTest, TestSocketRecvTimestampIPv6) {
 }
 #endif
 
+// Verify that if the socket was unable to be bound to a real network interface
+// (not loopback), Bind will return an error.
+TEST_F(PhysicalSocketTest,
+       BindFailsIfNetworkBinderFailsForNonLoopbackInterface) {
+  FakeNetworkBinder fake_network_binder;
+  server_->set_network_binder(&fake_network_binder);
+  std::unique_ptr<AsyncSocket> socket(
+      server_->CreateAsyncSocket(AF_INET, SOCK_DGRAM));
+  fake_network_binder.set_result(NetworkBindingResult::FAILURE);
+  EXPECT_EQ(-1, socket->Bind(SocketAddress("192.168.0.1", 0)));
+  server_->set_network_binder(nullptr);
+}
+
+// For a loopback interface, failures to bind to the interface should be
+// tolerated.
+TEST_F(PhysicalSocketTest,
+       BindSucceedsIfNetworkBinderFailsForLoopbackInterface) {
+  FakeNetworkBinder fake_network_binder;
+  server_->set_network_binder(&fake_network_binder);
+  std::unique_ptr<AsyncSocket> socket(
+      server_->CreateAsyncSocket(AF_INET, SOCK_DGRAM));
+  fake_network_binder.set_result(NetworkBindingResult::FAILURE);
+  EXPECT_EQ(0, socket->Bind(SocketAddress(kIPv4Loopback, 0)));
+  server_->set_network_binder(nullptr);
+}
+
 class PosixSignalDeliveryTest : public testing::Test {
  public:
   static void RecordSignal(int signum) {

webrtc/base/socketserver.h

@@ -17,6 +17,9 @@
 namespace rtc {
 
 class MessageQueue;
+// Needs to be forward declared because there's a circular dependency between
+// NetworkMonitor and Thread.
+// TODO(deadbeef): Fix this.
 class NetworkBinderInterface;
 
 // Provides the ability to wait for activity on a set of sockets.  The Thread

webrtc/sdk/android/src/jni/androidnetworkmonitor_jni.cc

@@ -236,8 +236,9 @@ void AndroidNetworkMonitor::Stop() {
 
 // The implementation is largely taken from UDPSocketPosix::BindToNetwork in
 // https://cs.chromium.org/chromium/src/net/udp/udp_socket_posix.cc
-int AndroidNetworkMonitor::BindSocketToNetwork(int socket_fd,
+rtc::NetworkBindingResult AndroidNetworkMonitor::BindSocketToNetwork(
-                                               const rtc::IPAddress& address) {
+    int socket_fd,
+    const rtc::IPAddress& address) {
   RTC_CHECK(thread_checker_.CalledOnValidThread());
   // Android prior to Lollipop didn't have support for binding sockets to
   // networks. In that case it should not have reached here because
@@ -246,12 +247,12 @@ int AndroidNetworkMonitor::BindSocketToNetwork(int socket_fd,
   if (android_sdk_int_ < SDK_VERSION_LOLLIPOP) {
     LOG(LS_ERROR) << "BindSocketToNetwork is not supported in Android SDK "
                   << android_sdk_int_;
-    return rtc::NETWORK_BIND_NOT_IMPLEMENTED;
+    return rtc::NetworkBindingResult::NOT_IMPLEMENTED;
   }
 
   auto iter = network_handle_by_address_.find(address);
   if (iter == network_handle_by_address_.end()) {
-    return rtc::NETWORK_BIND_ADDRESS_NOT_FOUND;
+    return rtc::NetworkBindingResult::ADDRESS_NOT_FOUND;
   }
   NetworkHandle network_handle = iter->second;
 
@@ -271,7 +272,7 @@ int AndroidNetworkMonitor::BindSocketToNetwork(int socket_fd,
       void* lib = dlopen(android_native_lib_path.c_str(), RTLD_NOW);
       if (lib == nullptr) {
         LOG(LS_ERROR) << "Library " << android_native_lib_path << " not found!";
-        return rtc::NETWORK_BIND_NOT_IMPLEMENTED;
+        return rtc::NetworkBindingResult::NOT_IMPLEMENTED;
       }
       marshmallowSetNetworkForSocket =
           reinterpret_cast<MarshmallowSetNetworkForSocket>(
@@ -279,7 +280,7 @@ int AndroidNetworkMonitor::BindSocketToNetwork(int socket_fd,
     }
     if (!marshmallowSetNetworkForSocket) {
       LOG(LS_ERROR) << "Symbol marshmallowSetNetworkForSocket is not found";
-      return rtc::NETWORK_BIND_NOT_IMPLEMENTED;
+      return rtc::NetworkBindingResult::NOT_IMPLEMENTED;
     }
     rv = marshmallowSetNetworkForSocket(network_handle, socket_fd);
   } else {
@@ -300,7 +301,7 @@ int AndroidNetworkMonitor::BindSocketToNetwork(int socket_fd,
       void* lib = dlopen(net_library_path.c_str(), RTLD_NOW | RTLD_NOLOAD);
       if (lib == nullptr) {
         LOG(LS_ERROR) << "Library " << net_library_path << " not found!";
-        return rtc::NETWORK_BIND_NOT_IMPLEMENTED;
+        return rtc::NetworkBindingResult::NOT_IMPLEMENTED;
       }
       lollipopSetNetworkForSocket =
           reinterpret_cast<LollipopSetNetworkForSocket>(
@@ -308,7 +309,7 @@ int AndroidNetworkMonitor::BindSocketToNetwork(int socket_fd,
     }
     if (!lollipopSetNetworkForSocket) {
       LOG(LS_ERROR) << "Symbol lollipopSetNetworkForSocket is not found ";
-      return rtc::NETWORK_BIND_NOT_IMPLEMENTED;
+      return rtc::NetworkBindingResult::NOT_IMPLEMENTED;
     }
     rv = lollipopSetNetworkForSocket(network_handle, socket_fd);
   }
@@ -317,12 +318,12 @@ int AndroidNetworkMonitor::BindSocketToNetwork(int socket_fd,
   // ERR_NETWORK_CHANGED, rather than MapSystemError(ENONET) which gives back
   // the less descriptive ERR_FAILED.
   if (rv == 0) {
-    return rtc::NETWORK_BIND_SUCCESS;
+    return rtc::NetworkBindingResult::SUCCESS;
   }
   if (rv == ENONET) {
-    return rtc::NETWORK_BIND_NETWORK_CHANGED;
+    return rtc::NetworkBindingResult::NETWORK_CHANGED;
   }
-  return rtc::NETWORK_BIND_FAILURE;
+  return rtc::NetworkBindingResult::FAILURE;
 }
 
 void AndroidNetworkMonitor::OnNetworkConnected(

webrtc/sdk/android/src/jni/androidnetworkmonitor_jni.h

@@ -58,8 +58,9 @@ class AndroidNetworkMonitor : public rtc::NetworkMonitorBase,
   void Start() override;
   void Stop() override;
 
-  int BindSocketToNetwork(int socket_fd,
+  rtc::NetworkBindingResult BindSocketToNetwork(
-                          const rtc::IPAddress& address) override;
+      int socket_fd,
+      const rtc::IPAddress& address) override;
   rtc::AdapterType GetAdapterType(const std::string& if_name) override;
   void OnNetworkConnected(const NetworkInformation& network_info);
   void OnNetworkDisconnected(NetworkHandle network_handle);