From c874d1296a96bd234c17fb73c0327d69b7f7c5dd Mon Sep 17 00:00:00 2001 From: deadbeef Date: Mon, 13 Feb 2017 15:41:59 -0800 Subject: [PATCH] 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 | 17 +++---- webrtc/base/physicalsocketserver.cc | 44 +++++++++++++++---- webrtc/base/physicalsocketserver_unittest.cc | 39 ++++++++++++++++ webrtc/base/socketserver.h | 3 ++ .../src/jni/androidnetworkmonitor_jni.cc | 23 +++++----- .../src/jni/androidnetworkmonitor_jni.h | 5 ++- 6 files changed, 101 insertions(+), 30 deletions(-) diff --git a/webrtc/base/networkmonitor.h b/webrtc/base/networkmonitor.h index 5459cd63e9..72b07b449c 100644 --- a/webrtc/base/networkmonitor.h +++ b/webrtc/base/networkmonitor.h @@ -19,13 +19,12 @@ namespace rtc { class IPAddress; -// Error values are negative. -enum NetworkBindingResults { - NETWORK_BIND_SUCCESS = 0, // No error - NETWORK_BIND_FAILURE = -1, // Generic error - NETWORK_BIND_NOT_IMPLEMENTED = -2, - NETWORK_BIND_ADDRESS_NOT_FOUND = -3, - NETWORK_BIND_NETWORK_CHANGED = -4 +enum class NetworkBindingResult { + SUCCESS = 0, // No error + FAILURE = -1, // Generic error + NOT_IMPLEMENTED = -2, + ADDRESS_NOT_FOUND = -3, + 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() {} }; diff --git a/webrtc/base/physicalsocketserver.cc b/webrtc/base/physicalsocketserver.cc index 07591e8ad3..c044529c89 100644 --- a/webrtc/base/physicalsocketserver.cc +++ b/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(result); + } else { + LOG(LS_WARNING) << "Binding socket to network address " + << bind_addr.ipaddr().ToString() + << " failed; result: " << static_cast(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(&addr_storage); int err = ::bind(s_, addr, static_cast(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; } diff --git a/webrtc/base/physicalsocketserver_unittest.cc b/webrtc/base/physicalsocketserver_unittest.cc index 37d412dc1f..d0083bdcfc 100644 --- a/webrtc/base/physicalsocketserver_unittest.cc +++ b/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 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 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) { diff --git a/webrtc/base/socketserver.h b/webrtc/base/socketserver.h index 7071f22526..5eada4a406 100644 --- a/webrtc/base/socketserver.h +++ b/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 diff --git a/webrtc/sdk/android/src/jni/androidnetworkmonitor_jni.cc b/webrtc/sdk/android/src/jni/androidnetworkmonitor_jni.cc index b5e5ce948e..4b3144efcb 100644 --- a/webrtc/sdk/android/src/jni/androidnetworkmonitor_jni.cc +++ b/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, - const rtc::IPAddress& address) { +rtc::NetworkBindingResult AndroidNetworkMonitor::BindSocketToNetwork( + 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( @@ -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( @@ -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( diff --git a/webrtc/sdk/android/src/jni/androidnetworkmonitor_jni.h b/webrtc/sdk/android/src/jni/androidnetworkmonitor_jni.h index 8df778e1d3..90e46c3885 100644 --- a/webrtc/sdk/android/src/jni/androidnetworkmonitor_jni.h +++ b/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, - const rtc::IPAddress& address) override; + rtc::NetworkBindingResult BindSocketToNetwork( + 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);