From faf376cd7debfd3d36dda3c91e3fbe1cd43e3610 Mon Sep 17 00:00:00 2001 From: Jonas Oreland Date: Fri, 1 Apr 2022 13:57:05 +0200 Subject: [PATCH] Implement IsAdapterAvailable in AndroidNetworkMonitor This cl/ fixes a race condition with the recent additions to NetworkMonitorAutoDetect (getAllNetworksFromCache). The getAllNetworksFromCache-feature uses the by the Android team preferred way of enumerating networks, i.e to register network listeners. This however introduces a unpleasant race condition like this: 1) network.cc discover rmnet0 2) BasicPortAllocator tries to create UDP port on rmnet0 - This fails as BindSocketToNetwork requires a android handle. 3) NetworkMonitorAutoDetect gets callback with rmnet0 4) BasicPortAllocator tries to create TCP port on rmnet0 - This succeeds. 5) Since rmnet0 has one working port, there will not be any new ports created on that network => We end up with a TCP only connection :( --- By impl. IsAdapterAvailable, we make sure that the network will not be used by BasicPortAllocator (or anyone else!) until we support binding to it. The IsAdapterAvailable was implemented for IOS, and has test coverage using FakeNetworkManager. This cl/ is default enabled with the kill-switch WebRTC-AndroidNetworkMonitor-IsAdapterAvailable. Bug: webrtc:13741 Change-Id: I7c2cb7789660fd2e082c214d00e50c894666b07c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/257400 Reviewed-by: Harald Alvestrand Commit-Queue: Jonas Oreland Cr-Commit-Position: refs/heads/main@{#36406} --- p2p/client/basic_port_allocator.cc | 11 +++++- .../src/jni/android_network_monitor.cc | 36 ++++++++++++++++--- sdk/android/src/jni/android_network_monitor.h | 6 ++++ 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/p2p/client/basic_port_allocator.cc b/p2p/client/basic_port_allocator.cc index 8b4bf57ee7..a62b9bc5d5 100644 --- a/p2p/client/basic_port_allocator.cc +++ b/p2p/client/basic_port_allocator.cc @@ -30,6 +30,7 @@ #include "rtc_base/checks.h" #include "rtc_base/helpers.h" #include "rtc_base/logging.h" +#include "rtc_base/strings/string_builder.h" #include "rtc_base/task_utils/to_queued_task.h" #include "rtc_base/trace_event.h" #include "system_wrappers/include/metrics.h" @@ -141,6 +142,14 @@ bool IsAllowedByCandidateFilter(const Candidate& c, uint32_t filter) { return false; } +std::string NetworksToString(const std::vector& networks) { + rtc::StringBuilder ost; + for (auto n : networks) { + ost << n->name() << " "; + } + return ost.Release(); +} + } // namespace const uint32_t DISABLE_ALL_PHASES = @@ -801,7 +810,7 @@ void BasicPortAllocatorSession::DoAllocate(bool disable_equivalent) { << "Machine has no networks; no ports will be allocated"; done_signal_needed = true; } else { - RTC_LOG(LS_INFO) << "Allocate ports on " << networks.size() << " networks"; + RTC_LOG(LS_INFO) << "Allocate ports on " << NetworksToString(networks); PortConfiguration* config = configs_.empty() ? nullptr : configs_.back().get(); for (uint32_t i = 0; i < networks.size(); ++i) { diff --git a/sdk/android/src/jni/android_network_monitor.cc b/sdk/android/src/jni/android_network_monitor.cc index 754267da83..67206c8505 100644 --- a/sdk/android/src/jni/android_network_monitor.cc +++ b/sdk/android/src/jni/android_network_monitor.cc @@ -250,6 +250,8 @@ void AndroidNetworkMonitor::Start() { "WebRTC-FindNetworkHandleWithoutIpv6TemporaryPart"); bind_using_ifname_ = !webrtc::field_trial::IsDisabled("WebRTC-BindUsingInterfaceName"); + disable_is_adapter_available_ = webrtc::field_trial::IsDisabled( + "WebRTC-AndroidNetworkMonitor-IsAdapterAvailable"); // This pointer is also accessed by the methods called from java threads. // Assigning it here is safe, because the java monitor is in a stopped state, @@ -449,9 +451,9 @@ AndroidNetworkMonitor::FindNetworkHandleFromIfname( RTC_DCHECK_RUN_ON(network_thread_); if (bind_using_ifname_) { for (auto const& iter : network_info_by_handle_) { + // Use substring match so that e.g if_name="v4-wlan0" is matched + // agains iter="wlan0" if (if_name.find(iter.second.interface_name) != absl::string_view::npos) { - // Use partial match so that e.g if_name="v4-wlan0" is matched - // agains iter.first="wlan0" return absl::make_optional(iter.first); } } @@ -506,8 +508,8 @@ rtc::AdapterType AndroidNetworkMonitor::GetAdapterType( if (type == rtc::ADAPTER_TYPE_UNKNOWN && bind_using_ifname_) { for (auto const& iter : adapter_type_by_name_) { - // Use partial match so that e.g if_name="v4-wlan0" is matched - // agains iter.first="wlan0" + // Use substring match so that e.g if_name="v4-wlan0" is matched + // against iter="wlan0" if (if_name.find(iter.first) != absl::string_view::npos) { type = iter.second; break; @@ -566,6 +568,32 @@ rtc::NetworkPreference AndroidNetworkMonitor::GetNetworkPreference( return preference_iter->second; } +// Check if adapter is avaiable, and only return true for the interface +// that has been discovered by NetworkMonitorAutoDetect.java. +bool AndroidNetworkMonitor::IsAdapterAvailable(absl::string_view if_name) { + RTC_DCHECK_RUN_ON(network_thread_); + if (disable_is_adapter_available_) { + return true; + } + if (if_name == "lo") { + // localhost (if_name == lo) is used by unit tests. + return true; + } + bool val = adapter_type_by_name_.find(if_name) != adapter_type_by_name_.end(); + if (!val && bind_using_ifname_) { + for (auto const& iter : network_info_by_handle_) { + // Use substring match so that e.g if_name="v4-wlan0" is matched + // against iter.first="wlan0" + if (if_name.find(iter.second.interface_name) != absl::string_view::npos) { + val = true; + break; + } + } + } + + return val; +} + AndroidNetworkMonitorFactory::AndroidNetworkMonitorFactory() : j_application_context_(nullptr) {} diff --git a/sdk/android/src/jni/android_network_monitor.h b/sdk/android/src/jni/android_network_monitor.h index 60615bb360..a7cb766c0c 100644 --- a/sdk/android/src/jni/android_network_monitor.h +++ b/sdk/android/src/jni/android_network_monitor.h @@ -92,6 +92,7 @@ class AndroidNetworkMonitor : public rtc::NetworkMonitorInterface { absl::string_view if_name) override; rtc::NetworkPreference GetNetworkPreference( absl::string_view if_name) override; + bool IsAdapterAvailable(absl::string_view if_name) override; // Always expected to be called on the network thread. void SetNetworkInfos(const std::vector& network_infos); @@ -152,6 +153,11 @@ class AndroidNetworkMonitor : public rtc::NetworkMonitorInterface { // This applies to adapter_type_by_name_, vpn_underlying_adapter_type_by_name_ // and FindNetworkHandleFromIfname. bool bind_using_ifname_ RTC_GUARDED_BY(network_thread_) = true; + + // NOTE: disable_is_adapter_available_ is a kill switch for the impl. + // of IsAdapterAvailable(). + bool disable_is_adapter_available_ RTC_GUARDED_BY(network_thread_) = false; + rtc::scoped_refptr safety_flag_ RTC_PT_GUARDED_BY(network_thread_) = nullptr; };