ReconfigureDNS fixes, fixes macos exit mode

- ReconfigureDNS wasn't returning the old servers; made it void instead
  (the Apple code can just store a copy of the original upstream
  servers instead).
- Reconfiguring DNS reset the unbound context but didn't replace it, so
  a Down()/Up() would crash.
- Simplify Resolver() destructor to just call Down(), and make it final
  just so that no one tries to inherit from us (so that calling a
  virtual function from the destructor is safe).
- Rename CancelPendingQueries() to Down(); the former cancelled but also
  shut down the object, so the name seemed a bit misleading.
- Rename SetInternalState in Resolver_Base to ResetResolver, so that we
  aren't conflicting with ResetInternalState from Endpoint (which was a
  problem because TunEndpoint inherited from both; it could be resolved
  through the different argument type if we removed the default, but
  that seems gross).
- Make Resolver use a bare unbound context pointer rather than a
  shared_ptr; since Resolver (now) entirely manages it already we don't
  need an extra management layer, and it saves a bunch of `.get()`s.

llarp/apple/route_manager.cpp

@@ -31,10 +31,10 @@ namespace llarp::apple
     }
 
     if (enable)
-      saved_upstream_dns =
-          tun->ReconfigureDNS({SockAddr{127, 0, 0, 1, huint16_t{dns_trampoline_port}}});
+      tun->ReconfigureDNS({SockAddr{127, 0, 0, 1, {dns_trampoline_port}}});
     else
-      tun->ReconfigureDNS(std::move(saved_upstream_dns));
+      tun->ReconfigureDNS(router->GetConfig()->dns.m_upstreamDNS);
+
     trampoline_active = enable;
   }
 

llarp/apple/route_manager.hpp

@@ -48,7 +48,6 @@ namespace llarp::apple
    private:
     llarp::Context& context;
     bool trampoline_active = false;
-    std::vector<llarp::SockAddr> saved_upstream_dns;
     void
     check_trampoline(bool enable);
 

llarp/dns/server.cpp

@@ -113,9 +113,9 @@ namespace llarp::dns
     };
 
     /// Resolver_Base that uses libunbound
-    class Resolver : public Resolver_Base, public std::enable_shared_from_this<Resolver>
+    class Resolver final : public Resolver_Base, public std::enable_shared_from_this<Resolver>
     {
-      std::shared_ptr<ub_ctx> m_ctx;
+      ub_ctx* m_ctx = nullptr;
       std::weak_ptr<EventLoop> m_Loop;
 #ifdef _WIN32
       // windows is dumb so we do ub mainloop in a thread
@@ -179,7 +179,7 @@ namespace llarp::dns
         if (const auto port = dns.getPort(); port != 53)
           fmt::format_to(std::back_inserter(str), "@{}", port);
 
-        if (auto err = ub_ctx_set_fwd(m_ctx.get(), str.c_str()))
+        if (auto err = ub_ctx_set_fwd(m_ctx, str.c_str()))
         {
           throw std::runtime_error{
               fmt::format("cannot use {} as upstream dns: {}", str, ub_strerror(err))};
@@ -300,7 +300,7 @@ namespace llarp::dns
       void
       SetOpt(const std::string& key, const std::string& val)
       {
-        ub_ctx_set_option(m_ctx.get(), key.c_str(), val.c_str());
+        ub_ctx_set_option(m_ctx, key.c_str(), val.c_str());
       }
 
       // Wrapper around the above that takes 3+ arguments: the 2nd arg gets formatted with the
@@ -312,24 +312,21 @@ namespace llarp::dns
         SetOpt(key, fmt::format(format, std::forward<FmtArgs>(args)...));
       }
 
+      // Copy of the DNS config (a copy because on some platforms, like Apple, we change the applied
+      // upstream DNS settings when turning on/off exit mode).
       llarp::DnsConfig m_conf;
 
      public:
       explicit Resolver(const EventLoop_ptr& loop, llarp::DnsConfig conf)
-          : m_ctx{::ub_ctx_create(), ::ub_ctx_delete}, m_Loop{loop}, m_conf{std::move(conf)}
+          : m_Loop{loop}, m_conf{std::move(conf)}
       {
         Up(m_conf);
       }
 
-#ifdef _WIN32
-      virtual ~Resolver()
+      ~Resolver() override
       {
-        running = false;
-        runner.join();
+        Down();
       }
-#else
-      virtual ~Resolver() = default;
-#endif
 
       std::string_view
       ResolverName() const override
@@ -346,6 +343,10 @@ namespace llarp::dns
       void
       Up(const llarp::DnsConfig& conf)
       {
+        if (m_ctx)
+          throw std::logic_error{"Internal error: attempt to Up() dns server multiple times"};
+
+        m_ctx = ::ub_ctx_create();
         // set libunbound settings
 
         SetOpt("do-tcp:", "no");
@@ -357,7 +358,7 @@ namespace llarp::dns
         for (const auto& file : conf.m_hostfiles)
         {
           const auto str = file.u8string();
-          if (auto ret = ub_ctx_hosts(m_ctx.get(), str.c_str()))
+          if (auto ret = ub_ctx_hosts(m_ctx, str.c_str()))
           {
             throw std::runtime_error{
                 fmt::format("Failed to add host file {}: {}", file, ub_strerror(ret))};
@@ -367,15 +368,14 @@ namespace llarp::dns
         ConfigureUpstream(conf);
 
         // set async
-        ub_ctx_async(m_ctx.get(), 1);
+        ub_ctx_async(m_ctx, 1);
         // setup mainloop
 #ifdef _WIN32
         running = true;
-        runner = std::thread{[this, ctx = std::weak_ptr{m_ctx}]() {
+        runner = std::thread{[this]() {
           while (running)
           {
-            if (auto c = ctx.lock())
-              ub_wait(c.get());
+            ub_wait(ctx);
             std::this_thread::sleep_for(10ms);
           }
           if (auto c = ctx.lock())
@@ -386,10 +386,9 @@ namespace llarp::dns
         {
           if (auto loop_ptr = loop->MaybeGetUVWLoop())
           {
-            m_Poller = loop_ptr->resource<uvw::PollHandle>(ub_fd(m_ctx.get()));
-            m_Poller->on<uvw::PollEvent>([ptr = std::weak_ptr{m_ctx}](auto&, auto&) {
-              if (auto ctx = ptr.lock())
-                ub_process(ctx.get());
+            m_Poller = loop_ptr->resource<uvw::PollHandle>(ub_fd(m_ctx));
+            m_Poller->on<uvw::PollEvent>([this](auto&, auto&) {
+              ub_process(m_ctx);
             });
             m_Poller->start(uvw::PollHandle::Event::READABLE);
             return;
@@ -400,15 +399,19 @@ namespace llarp::dns
       }
 
       void
-      Down()
+      Down() override
       {
 #ifdef _WIN32
-        running = false;
-        runner.join();
+        if (running.exchange(false))
+          runner.join();
 #else
-        m_Poller->close();
+        if (m_Poller)
+          m_Poller->close();
 #endif
-        m_ctx.reset();
+        if (m_ctx) {
+          ::ub_ctx_delete(m_ctx);
+          m_ctx = nullptr;
+        }
       }
 
       int
@@ -418,20 +421,14 @@ namespace llarp::dns
       }
 
       void
-      ResetInternalState(std::optional<std::vector<SockAddr>> replace_upstream) override
+      ResetResolver(std::optional<std::vector<SockAddr>> replace_upstream) override
       {
         Down();
         if (replace_upstream)
-          m_conf.m_upstreamDNS = *replace_upstream;
+          m_conf.m_upstreamDNS = std::move(*replace_upstream);
         Up(m_conf);
       }
 
-      void
-      CancelPendingQueries() override
-      {
-        Down();
-      }
-
       bool
       WouldLoop(const SockAddr& to, const SockAddr& from) const override
       {
@@ -485,7 +482,7 @@ namespace llarp::dns
         }
         const auto& q = query.questions[0];
         if (auto err = ub_resolve_async(
-                m_ctx.get(),
+                m_ctx,
                 q.Name().c_str(),
                 q.qtype,
                 q.qclass,
@@ -642,7 +639,7 @@ namespace llarp::dns
     for (const auto& resolver : m_Resolvers)
     {
       if (auto ptr = resolver.lock())
-        ptr->CancelPendingQueries();
+        ptr->Down();
     }
   }
 
@@ -652,7 +649,7 @@ namespace llarp::dns
     for (const auto& resolver : m_Resolvers)
     {
       if (auto ptr = resolver.lock())
-        ptr->ResetInternalState();
+        ptr->ResetResolver();
     }
   }
 

llarp/dns/server.hpp

@@ -174,16 +174,17 @@ namespace llarp::dns
     virtual std::string_view
     ResolverName() const = 0;
 
-    /// reset state, replace upstream info with new info if desired
+    /// reset the resolver state, optionally replace upstream info with new info.  The default base
+    /// implementation does nothing.
     virtual void
-    ResetInternalState(std::optional<std::vector<SockAddr>> replace_upstream = std::nullopt)
+    ResetResolver([[maybe_unused]] std::optional<std::vector<SockAddr>> replace_upstream = std::nullopt)
     {
-      (void)replace_upstream;
-    };
+    }
 
-    /// cancel all pending requests and ceace further operation
+    /// cancel all pending requests and cease further operation.  Default operation is a no-op.
     virtual void
-    CancelPendingQueries(){};
+    Down(){}
+
     /// attempt to handle a dns message
     /// returns true if we consumed this query and it should not be processed again
     virtual bool
@@ -201,7 +202,7 @@ namespace llarp::dns
       (void)to;
       (void)from;
       return false;
-    };
+    }
   };
 
   // Base class for DNS proxy

llarp/handlers/exit.hpp

@@ -24,9 +24,6 @@ namespace llarp
         return "snode";
       }
 
-      void
-      CancelPendingQueries() override{};
-
       bool
       MaybeHookDNS(
           std::shared_ptr<dns::PacketSource_Base> source,

llarp/handlers/tun.cpp

@@ -260,7 +260,7 @@ namespace llarp
         m_DNS->Reset();
     }
 
-    std::vector<SockAddr>
+    void
     TunEndpoint::ReconfigureDNS(std::vector<SockAddr> servers)
     {
       if (m_DNS)
@@ -268,10 +268,9 @@ namespace llarp
         for (auto weak : m_DNS->GetAllResolvers())
         {
           if (auto ptr = weak.lock())
-            ptr->ResetInternalState(servers);
+            ptr->ResetResolver(servers);
         }
       }
-      return servers;
     }
 
     bool
@@ -903,7 +902,7 @@ namespace llarp
       return true;
     }
 
-    void TunEndpoint::ResetInternalState(std::optional<std::vector<SockAddr>>)
+    void TunEndpoint::ResetInternalState()
     {
       service::Endpoint::ResetInternalState();
     }

llarp/handlers/tun.hpp

@@ -67,9 +67,8 @@ namespace llarp
       void
       Thaw() override;
 
-      // Reconfigures DNS servers and restarts libunbound with the new servers.  Returns the old set
-      // of configured dns servers.
-      std::vector<SockAddr>
+      // Reconfigures DNS servers and restarts libunbound with the new servers.
+      void
       ReconfigureDNS(std::vector<SockAddr> servers);
 
       bool
@@ -202,8 +201,7 @@ namespace llarp
       ObtainIPForAddr(std::variant<service::Address, RouterID> addr) override;
 
       void
-      ResetInternalState(
-          std::optional<std::vector<SockAddr>> replace_upstream = std::nullopt) override;
+      ResetInternalState() override;
 
      protected:
       struct WritePacket