From 11893a780a0a8c2ed557c8b0a63abc87c6506f83 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 14 Feb 2022 13:50:54 +0100 Subject: [PATCH 1/2] Fix nameserver listener restarts --- nameserver/module.go | 98 +++++++++++++++++++++++++++++--------------- 1 file changed, 64 insertions(+), 34 deletions(-) diff --git a/nameserver/module.go b/nameserver/module.go index f136fac7..7bc177e4 100644 --- a/nameserver/module.go +++ b/nameserver/module.go @@ -6,6 +6,7 @@ import ( "net" "os" "strconv" + "sync" "github.com/miekg/dns" @@ -17,8 +18,12 @@ import ( ) var ( - module *modules.Module - stopListener func() error + module *modules.Module + + stopListeners bool + stopListener1 func() error + stopListener2 func() error + stopListenersLock sync.Mutex ) func init() { @@ -42,11 +47,13 @@ func start() error { return err } + // Get listen addresses. ip1, ip2, port, err := getListenAddresses(nameserverAddressConfig()) if err != nil { return fmt.Errorf("failed to parse nameserver listen address: %w", err) } + // Get own hostname. hostname, err = os.Hostname() if err != nil { log.Warningf("nameserver: failed to get hostname: %s", err) @@ -56,8 +63,7 @@ func start() error { // Start listener(s). if ip2 == nil { // Start a single listener. - dnsServer := startListener(ip1, port) - stopListener = dnsServer.Shutdown + startListener(ip1, port, true) // Set nameserver matcher in firewall to fast-track dns queries. if ip1.Equal(net.IPv4zero) || ip1.Equal(net.IPv6zero) { @@ -73,22 +79,11 @@ func start() error { return firewall.SetNameserverIPMatcher(func(ip net.IP) bool { return ip.Equal(ip1) }) - } // Dual listener. - dnsServer1 := startListener(ip1, port) - dnsServer2 := startListener(ip2, port) - stopListener = func() error { - // Shutdown both listeners. - err1 := dnsServer1.Shutdown() - err2 := dnsServer2.Shutdown() - // Return first error. - if err1 != nil { - return err1 - } - return err2 - } + startListener(ip1, port, true) + startListener(ip2, port, false) // Fast track dns queries destined for one of the listener IPs. return firewall.SetNameserverIPMatcher(func(ip net.IP) bool { @@ -96,20 +91,46 @@ func start() error { }) } -func startListener(ip net.IP, port uint16) *dns.Server { - // Create DNS server. - dnsServer := &dns.Server{ - Addr: net.JoinHostPort( - ip.String(), - strconv.Itoa(int(port)), - ), - Net: "udp", - } - dns.HandleFunc(".", handleRequestAsWorker) - +func startListener(ip net.IP, port uint16, first bool) { // Start DNS server as service worker. - log.Infof("nameserver: starting to listen on %s", dnsServer.Addr) module.StartServiceWorker("dns resolver", 0, func(ctx context.Context) error { + // Create DNS server. + dnsServer := &dns.Server{ + Addr: net.JoinHostPort( + ip.String(), + strconv.Itoa(int(port)), + ), + Net: "udp", + Handler: dns.HandlerFunc(handleRequestAsWorker), + } + + // Register stop function. + func() { + stopListenersLock.Lock() + defer stopListenersLock.Unlock() + + // Check if we should stop + if stopListeners { + _ = dnsServer.Shutdown() + dnsServer = nil + return + } + + // Register stop function. + if first { + stopListener1 = dnsServer.Shutdown + } else { + stopListener2 = dnsServer.Shutdown + } + }() + + // Check if we should stop. + if dnsServer == nil { + return nil + } + + // Start listening. + log.Infof("nameserver: starting to listen on %s", dnsServer.Addr) err := dnsServer.ListenAndServe() if err != nil { // check if we are shutting down @@ -124,16 +145,25 @@ func startListener(ip net.IP, port uint16) *dns.Server { } return err }) - - return dnsServer } func stop() error { - if stopListener != nil { - if err := stopListener(); err != nil { - log.Warningf("nameserver: failed to stop: %s", err) + stopListenersLock.Lock() + defer stopListenersLock.Unlock() + + // Stop listeners. + stopListeners = true + if stopListener1 != nil { + if err := stopListener1(); err != nil { + log.Warningf("nameserver: failed to stop listener1: %s", err) } } + if stopListener2 != nil { + if err := stopListener2(); err != nil { + log.Warningf("nameserver: failed to stop listener2: %s", err) + } + } + return nil } From b2a1956e5c644fa385fede79bcbaa76374aec442 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 14 Feb 2022 13:51:10 +0100 Subject: [PATCH 2/2] Fix nameserver takeover loop --- nameserver/takeover.go | 55 +++++++++++++++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 9 deletions(-) diff --git a/nameserver/takeover.go b/nameserver/takeover.go index 5446e8ef..fa1b65af 100644 --- a/nameserver/takeover.go +++ b/nameserver/takeover.go @@ -5,6 +5,7 @@ import ( "net" "os" "strconv" + "time" "github.com/safing/portbase/log" "github.com/safing/portbase/modules" @@ -13,13 +14,20 @@ import ( "github.com/safing/portmaster/network/state" ) -var commonResolverIPs = []net.IP{ - net.IPv4zero, - net.IPv4(127, 0, 0, 1), // default - net.IPv4(127, 0, 0, 53), // some resolvers on Linux - net.IPv6zero, - net.IPv6loopback, -} +var ( + commonResolverIPs = []net.IP{ + net.IPv4zero, + net.IPv4(127, 0, 0, 1), // default + net.IPv4(127, 0, 0, 53), // some resolvers on Linux + net.IPv6zero, + net.IPv6loopback, + } + + // lastKilledPID holds the PID of the last killed conflicting service. + // It is only accessed by checkForConflictingService, which is only called by + // the nameserver worker. + lastKilledPID int +) func checkForConflictingService(ip net.IP, port uint16) error { // Evaluate which IPs to check. @@ -32,14 +40,16 @@ func checkForConflictingService(ip net.IP, port uint16) error { // Check if there is another resolver when need to take over. var killed int + var killingFailed bool ipsToCheckLoop: for _, resolverIP := range ipsToCheck { pid, err := takeover(resolverIP, port) switch { case err != nil: // Log the error and let the worker try again. - log.Infof("nameserver: could not stop conflicting service: %s", err) - return nil + log.Infof("nameserver: failed to stop conflicting service: %s", err) + killingFailed = true + break ipsToCheckLoop case pid != 0: // Conflicting service identified and killed! killed = pid @@ -47,10 +57,35 @@ ipsToCheckLoop: } } + // Notify user of failed killing or repeated kill. + if killingFailed || (killed != 0 && killed == lastKilledPID) { + // Notify the user that we failed to kill something. + notifications.Notify(¬ifications.Notification{ + EventID: "namserver:failed-to-kill-conflicting-service", + Type: notifications.Error, + Title: "Failed to Stop Conflicting DNS Client", + Message: "The Portmaster failed to stop a conflicting DNS client to gain required system integration. If there is another DNS Client (Nameserver; Resolver) on this device, please disable it.", + ShowOnSystem: true, + AvailableActions: []*notifications.Action{ + { + ID: "ack", + Text: "OK", + }, + { + Text: "Open Docs", + Type: notifications.ActionTypeOpenURL, + Payload: "https://docs.safing.io/portmaster/install/status/software-compatibility", + }, + }, + }) + return nil + } + // Check if something was killed. if killed == 0 { return nil } + lastKilledPID = killed // Notify the user that we killed something. notifications.Notify(¬ifications.Notification{ @@ -76,6 +111,8 @@ ipsToCheckLoop: }) // Restart nameserver via service-worker logic. + // Wait shortly so that the other process can shut down. + time.Sleep(10 * time.Millisecond) return fmt.Errorf("%w: stopped conflicting name service with pid %d", modules.ErrRestartNow, killed) }