Merge pull request #531 from safing/fix/nameserver-service-and-conflicts

Fix nameserver servive and conflicts

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
 }
 

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(&notifications.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(&notifications.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)
 }