mobile: Fix bugs in the Apple PAC proxy resolver

Signed-off-by: Ali Beyad <abeyad@google.com>

mobile/library/common/BUILD

@@ -31,6 +31,7 @@ envoy_cc_library(
         "//library/common/http:header_utility_lib",
         "//library/common/logger:logger_delegate_lib",
         "//library/common/network:connectivity_manager_lib",
+        "//library/common/network:proxy_api_lib",
         "//library/common/stats:utility_lib",
         "//library/common/types:c_types_lib",
         "@envoy//envoy/server:lifecycle_notifier_interface",

mobile/library/common/extensions/filters/http/network_configuration/filter.cc

@@ -113,9 +113,9 @@ NetworkConfigurationFilter::resolveProxy(Http::RequestHeaderMap& request_headers
             Thread::MainThread::isMainOrTestThread(),
             "NetworkConfigurationProxy PAC proxy resolver callback not running on main thread.");
         // This is the callback invoked from the Apple APIs resolving the PAC file URL, which
-        // gets invoked on the Envoy thread. We keep a weak_ptr to this filter instance
-        // so that, if the stream is canceled and the filter chain is torn down in the meantime,
-        // we will fail to aquire the weak_ptr lock and won't execute any callbacks on the resolved
+        // gets invoked on a separate thread. We keep a weak_ptr to this filter instance so that,
+        // if the stream is canceled and the filter chain is torn down in the meantime, we will
+        // fail to aquire the weak_ptr lock and won't execute any callbacks on the resolved
         // proxies.
         if (auto filter_ptr = weak_self.lock()) {
           filter_ptr->onProxyResolutionComplete(Network::ProxySettings::create(proxies));

mobile/library/common/internal_engine.cc

@@ -10,6 +10,7 @@
 #include "source/common/runtime/runtime_features.h"
 
 #include "absl/synchronization/notification.h"
+#include "library/common/network/proxy_api.h"
 #include "library/common/stats/utility.h"
 
 namespace Envoy {
@@ -131,6 +132,13 @@ envoy_status_t InternalEngine::main(std::shared_ptr<Envoy::OptionsImplBase> opti
       server_ = main_common->server();
       event_dispatcher_ = &server_->dispatcher();
 
+      // If proxy resolution APIs are configured on the Engine, set the main thread's dispatcher
+      // on the proxy resolver for handling callbacks.
+      auto* proxy_resolver = static_cast<Network::ProxyResolverApi*>(Api::External::retrieveApi("envoy_proxy_resolver", /*allow_absent=*/true));
+      if (proxy_resolver != nullptr) {
+        proxy_resolver->resolver->setDispatcher(event_dispatcher_);
+      }
+
       cv_.notifyAll();
     }
     END_TRY

mobile/library/common/network/BUILD

@@ -108,6 +108,7 @@ envoy_cc_library(
     repository = "@envoy",
     deps = [
         ":proxy_settings_lib",
+        "@envoy//envoy/event:dispatcher_interface",
     ],
 )
 
@@ -150,6 +151,9 @@ envoy_cc_library(
             "//library/common/apple:utility_lib",
             "//library/common/extensions/cert_validator/platform_bridge:c_types_lib",
             "//library/common/types:c_types_lib",
+            "@envoy//envoy/event:dispatcher_interface",
+            "@envoy//source/common/common:thread_impl_lib_posix",
+            "@envoy//source/common/common:thread_lib",
         ],
         "//conditions:default": [],
     }),

mobile/library/common/network/apple_pac_proxy_resolver.cc

@@ -13,13 +13,16 @@ namespace Network {
 
 namespace {
 
+CFStringRef pacRunLoopMode() {
+  static const CFStringRef runloop_mode = CFSTR("envoy.PacProxyResolver");
+  return runloop_mode;
+}
+
 // Creates a CFURLRef from a C++ string URL.
 CFURLRef createCFURL(const std::string& url_string) {
-  auto cf_url_string =
-      CFStringCreateWithCString(kCFAllocatorDefault, url_string.c_str(), kCFStringEncodingUTF8);
-  auto cf_url = CFURLCreateWithString(kCFAllocatorDefault, cf_url_string, /*baseURL=*/nullptr);
-  CFRelease(cf_url_string);
-  return cf_url;
+  return CFURLCreateWithBytes(kCFAllocatorDefault,
+                              reinterpret_cast<const UInt8*>(url_string.c_str()),
+                              url_string.length(), kCFStringEncodingUTF8, nullptr);
 }
 
 } // namespace
@@ -31,16 +34,15 @@ void proxyAutoConfigurationResultCallback(void* ptr, CFArrayRef cf_proxies, CFEr
   std::unique_ptr<ProxySettingsResolvedCallback> completion_callback(
       static_cast<ProxySettingsResolvedCallback*>(ptr));
 
-  std::vector<ProxySettings> proxies;
-  if (cf_error != nullptr || cf_proxies == nullptr) {
+  if (cf_error == nullptr) {
     ENVOY_BUG(cf_error != nullptr, Apple::toString(CFErrorCopyDescription(cf_error)));
-    // Treat the error case as if no proxy was configured. Seems to be consistent with what iOS
-    // system (URLSession) is doing.
-    (*completion_callback)(proxies);
-    return;
   }
 
-  for (int i = 0; i < CFArrayGetCount(cf_proxies); i++) {
+  // Treat the error case as if no proxy was configured. Seems to be consistent with what iOS
+  // system (URLSession) is doing.
+  const int num_proxy_entries = cf_proxies != nullptr ? CFArrayGetCount(cf_proxies) : 0;
+  std::vector<ProxySettings> proxies;
+  for (int i = 0; i < num_proxy_entries; i++) {
     CFDictionaryRef cf_dictionary =
         static_cast<CFDictionaryRef>(CFArrayGetValueAtIndex(cf_proxies, i));
     CFStringRef cf_proxy_type =
@@ -65,6 +67,8 @@ void proxyAutoConfigurationResultCallback(void* ptr, CFArrayRef cf_proxies, CFEr
   }
 
   (*completion_callback)(proxies);
+
+  CFRunLoopStop(CFRunLoopGetCurrent());
 }
 
 CFRunLoopSourceRef
@@ -80,13 +84,28 @@ ApplePacProxyResolver::createPacUrlResolverSource(CFURLRef cf_proxy_autoconfigur
 }
 
 void ApplePacProxyResolver::resolveProxies(
-    const std::string& target_url, const std::string& proxy_autoconfiguration_file_url,
+    const std::string& proxy_autoconfiguration_file_url, const std::string& target_url,
     ProxySettingsResolvedCallback proxy_resolution_completed) {
   CFURLRef cf_target_url = createCFURL(target_url);
   CFURLRef cf_proxy_autoconfiguration_file_url = createCFURL(proxy_autoconfiguration_file_url);
+  if (cf_target_url == nullptr || cf_proxy_autoconfiguration_file_url == nullptr) {
+    return;
+  }
 
   std::unique_ptr<ProxySettingsResolvedCallback> completion_callback =
       std::make_unique<ProxySettingsResolvedCallback>(std::move(proxy_resolution_completed));
+
+  // Work around <rdar://problem/5530166>. This dummy call to CFNetworkCopyProxiesForURL
+  // initializes some state within CFNetwork that is required by
+  // CFNetworkExecuteProxyAutoConfigurationURL.
+  CFDictionaryRef empty_dictionary =
+      CFDictionaryCreate(nullptr, nullptr, nullptr, 0, nullptr, nullptr);
+  CFArrayRef empty_result = CFNetworkCopyProxiesForURL(cf_target_url, empty_dictionary);
+  CFRelease(empty_dictionary);
+  if (empty_result) {
+    CFRelease(empty_result);
+  }
+
   // According to https://developer.apple.com/documentation/corefoundation/cfstreamclientcontext,
   // the version must be 0.
   auto context = std::make_unique<CFStreamClientContext>(
@@ -102,7 +121,9 @@ void ApplePacProxyResolver::resolveProxies(
   CFRunLoopSourceRef run_loop_source = createPacUrlResolverSource(
       cf_proxy_autoconfiguration_file_url, cf_target_url, context.release());
 
-  CFRunLoopAddSource(CFRunLoopGetCurrent(), run_loop_source, kCFRunLoopDefaultMode);
+  CFRunLoopAddSource(CFRunLoopGetCurrent(), run_loop_source, pacRunLoopMode());
+  CFRunLoopRunInMode(pacRunLoopMode(), DBL_MAX, false);
+  CFRunLoopRemoveSource(CFRunLoopGetCurrent(), run_loop_source, pacRunLoopMode());
 
   CFRelease(cf_target_url);
   CFRelease(cf_proxy_autoconfiguration_file_url);

mobile/library/common/network/apple_pac_proxy_resolver.h

@@ -27,13 +27,13 @@ class ApplePacProxyResolver {
   /**
    * Resolves proxy for a given URL using proxy auto configuration file that's hosted at a given
    * URL.
-   * @param target_url A request URL to resolve the proxy for.
    * @param proxy_autoconfiguration_file_url A URL at which a proxy configuration file is hosted.
+   * @param target_url A request URL to resolve the proxy for.
    * @param proxy_resolution_completed A function that's called with result proxies as its
    * arguments when proxy resolution completes.
    */
-  void resolveProxies(const std::string& target_url,
-                      const std::string& proxy_autoconfiguration_file_url,
+  void resolveProxies(const std::string& proxy_autoconfiguration_file_url,
+                      const std::string& target_url,
                       ProxySettingsResolvedCallback proxy_resolution_completed);
 
 protected:

mobile/library/common/network/apple_proxy_resolver.cc

@@ -1,14 +1,25 @@
 #include "library/common/network/apple_proxy_resolver.h"
 
 #include "source/common/common/assert.h"
+#include "source/common/common/posix/thread_impl.h"
 
 namespace Envoy {
 namespace Network {
 
 AppleProxyResolver::AppleProxyResolver()
     : proxy_settings_monitor_(
           std::make_unique<AppleSystemProxySettingsMonitor>(proxySettingsUpdater())),
-      pac_proxy_resolver_(std::make_unique<ApplePacProxyResolver>()) {}
+      pac_proxy_resolver_(std::make_unique<ApplePacProxyResolver>()),
+      thread_factory_(Thread::PosixThreadFactory::create()) {}
+
+AppleProxyResolver::~AppleProxyResolver() {
+  // Wait for the PAC resolution threads to finish.
+  for (auto& [_, thread] : pac_resolution_threads_) {
+    if (thread->joinable()) {
+      thread->join();
+    }
+  }
+}
 
 SystemProxySettingsReadCallback AppleProxyResolver::proxySettingsUpdater() {
   return SystemProxySettingsReadCallback(
@@ -23,11 +34,14 @@ void AppleProxyResolver::start() {
   started_ = true;
 }
 
+void AppleProxyResolver::setDispatcher(Event::Dispatcher* dispatcher) { dispatcher_ = dispatcher; }
+
 ProxyResolutionResult
 AppleProxyResolver::resolveProxy(const std::string& target_url_string,
                                  std::vector<ProxySettings>& proxies,
                                  ProxySettingsResolvedCallback proxy_resolution_completed) {
   ASSERT(started_, "AppleProxyResolver not started.");
+  ASSERT(dispatcher_ != nullptr, "Dispatcher not set on the AppleProxyResolver.");
 
   std::string pac_file_url;
   {
@@ -55,11 +69,26 @@ AppleProxyResolver::resolveProxy(const std::string& target_url_string,
   // implement a caching layer with TTLs for PAC file URL resolution within AppleProxyResolver
   // itself.
   ASSERT(!pac_file_url.empty(), "PAC file URL must not be empty if PAC is enabled.");
-  pac_proxy_resolver_->resolveProxies(
-      pac_file_url, target_url_string,
-      [proxy_resolution_completed](const std::vector<ProxySettings>& proxies) {
-        proxy_resolution_completed(proxies);
-      });
+  Thread::PosixThreadPtr thread = thread_factory_->createThread(
+      [this, pac_file_url, target_url_string, callback = std::move(proxy_resolution_completed)] {
+        pac_proxy_resolver_->resolveProxies(
+            pac_file_url, target_url_string,
+            [this, callback](const std::vector<ProxySettings>& proxies) {
+              auto pac_resolve_thread_id = thread_factory_->currentPthreadId();
+              dispatcher_->post([this, callback, proxies, pac_resolve_thread_id] {
+                auto thread_handle = pac_resolution_threads_.extract(pac_resolve_thread_id);
+                if (!thread_handle.empty()) {
+                  thread_handle.mapped()->join();
+                }
+
+                callback(proxies);
+              });
+            });
+      },
+      /* options= */ absl::nullopt, /* crash_on_failure=*/false);
+  Thread::ThreadId thread_id = thread->pthreadId();
+  pac_resolution_threads_[thread_id] = std::move(thread);
+
   return ProxyResolutionResult::ResultInProgress;
 }
 

mobile/library/common/network/apple_proxy_resolver.h

@@ -6,6 +6,12 @@
 #include <functional>
 #include <memory>
 
+#include "envoy/event/dispatcher.h"
+
+#include "source/common/common/posix/thread_impl.h"
+#include "source/common/common/thread.h"
+
+#include "absl/container/flat_hash_map.h"
 #include "library/common/network/apple_pac_proxy_resolver.h"
 #include "library/common/network/apple_system_proxy_settings_monitor.h"
 #include "library/common/network/proxy_resolver_interface.h"
@@ -20,7 +26,7 @@ namespace Network {
 class AppleProxyResolver : public ProxyResolver {
 public:
   AppleProxyResolver();
-  virtual ~AppleProxyResolver() = default;
+  virtual ~AppleProxyResolver();
 
   /**
    * Starts proxy resolver. It needs to be called prior to any proxy resolution attempt.
@@ -38,6 +44,8 @@ class AppleProxyResolver : public ProxyResolver {
   resolveProxy(const std::string& target_url_string, std::vector<ProxySettings>& proxies,
                ProxySettingsResolvedCallback proxy_resolution_completed) override;
 
+  virtual void setDispatcher(Event::Dispatcher* dispatcher) override;
+
   /*
    * Supplies a function that updates this instance's proxy settings.
    */
@@ -49,6 +57,9 @@ class AppleProxyResolver : public ProxyResolver {
   std::unique_ptr<AppleSystemProxySettingsMonitor> proxy_settings_monitor_;
   std::unique_ptr<ApplePacProxyResolver> pac_proxy_resolver_;
   absl::optional<SystemProxySettings> proxy_settings_;
+  std::unique_ptr<Thread::PosixThreadFactory> thread_factory_;
+  absl::flat_hash_map<Thread::ThreadId, Thread::PosixThreadPtr> pac_resolution_threads_;
+  Event::Dispatcher* dispatcher_;
   absl::Mutex mutex_;
   bool started_ = false;
 };

mobile/library/common/network/proxy_resolver_interface.h

@@ -1,5 +1,7 @@
 #pragma once
 
+#include "envoy/event/dispatcher.h"
+
 #include "library/common/network/proxy_settings.h"
 
 namespace Envoy {
@@ -28,6 +30,14 @@ class ProxyResolver {
   virtual ProxyResolutionResult
   resolveProxy(const std::string& target_url_string, std::vector<ProxySettings>& proxies,
                ProxySettingsResolvedCallback proxy_resolution_completed) PURE;
+
+  /**
+   * Sets the Event Dispatcher that a proxy resolver instance can use to post callbacks to.
+   * Typically, this would be the main Envoy thread's dispatcher (which is also the worker thread
+   * in Envoy Mobile).
+   * @param dispatcher The event dispatcher.
+   */
+  virtual void setDispatcher(Event::Dispatcher* dispatcher) PURE;
 };
 
 } // namespace Network

mobile/test/common/proxy/BUILD

@@ -9,7 +9,6 @@ envoy_cc_test_library(
     srcs = select({
         "@envoy//bazel:apple": [
             "test_apple_api_registration.cc",
-            "test_apple_pac_proxy_resolver.cc",
             "test_apple_proxy_resolver.cc",
             "test_apple_proxy_settings_monitor.cc",
         ],
@@ -18,7 +17,6 @@ envoy_cc_test_library(
     hdrs = select({
         "@envoy//bazel:apple": [
             "test_apple_api_registration.h",
-            "test_apple_pac_proxy_resolver.h",
             "test_apple_proxy_resolver.h",
             "test_apple_proxy_settings_monitor.h",
         ],

mobile/test/common/proxy/test_apple_api_registration.cc

@@ -1,6 +1,5 @@
 #include "test/common/proxy/test_apple_api_registration.h"
 
-#include "test/common/proxy/test_apple_pac_proxy_resolver.h"
 #include "test/common/proxy/test_apple_proxy_resolver.h"
 #include "test/common/proxy/test_apple_proxy_settings_monitor.h"
 
@@ -30,11 +29,6 @@ void registerTestAppleProxyResolver(absl::string_view host, int port, const bool
   test_resolver->setSettingsMonitorForTest(
       std::make_unique<Envoy::Network::TestAppleSystemProxySettingsMonitor>(
           std::string(host), port, use_pac_resolver, test_resolver->proxySettingsUpdater()));
-  if (use_pac_resolver) {
-    // Create a TestApplePacProxyResolver and set the test resolver to use it.
-    test_resolver->setPacResolverForTest(
-        std::make_unique<Envoy::Network::TestApplePacProxyResolver>(std::string(host), port));
-  }
   // Start the resolver, as we do when registering the envoy_proxy_resolver API.
   test_resolver->start();
   // Create a new test ProxyResolverApi.