dnsforward: entitle lock, imp code

internal/dnsforward/access.go

@@ -142,14 +142,19 @@ type accessListJSON struct {
 	BlockedHosts      []string `json:"blocked_hosts"`
 }
 
-func (s *Server) handleAccessList(w http.ResponseWriter, r *http.Request) {
-	s.RLock()
-	j := accessListJSON{
-		AllowedClients:    s.conf.AllowedClients,
-		DisallowedClients: s.conf.DisallowedClients,
-		BlockedHosts:      s.conf.BlockedHosts,
+func (s *Server) accessListJSON() (j accessListJSON) {
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
+
+	return accessListJSON{
+		AllowedClients:    aghstrings.CloneSlice(s.conf.AllowedClients),
+		DisallowedClients: aghstrings.CloneSlice(s.conf.DisallowedClients),
+		BlockedHosts:      aghstrings.CloneSlice(s.conf.BlockedHosts),
 	}
-	s.RUnlock()
+}
+
+func (s *Server) handleAccessList(w http.ResponseWriter, r *http.Request) {
+	j := s.accessListJSON()
 
 	w.Header().Set("Content-Type", "application/json")
 	err := json.NewEncoder(w).Encode(j)
@@ -200,14 +205,16 @@ func (s *Server) handleAccessSet(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	s.Lock()
+	defer log.Debug("Access: updated lists: %d, %d, %d",
+		len(j.AllowedClients), len(j.DisallowedClients), len(j.BlockedHosts))
+
+	defer s.conf.ConfigModified()
+
+	s.serverLock.Lock()
+	defer s.serverLock.Unlock()
+
 	s.conf.AllowedClients = j.AllowedClients
 	s.conf.DisallowedClients = j.DisallowedClients
 	s.conf.BlockedHosts = j.BlockedHosts
 	s.access = a
-	s.Unlock()
-	s.conf.ConfigModified()
-
-	log.Debug("Access: updated lists: %d, %d, %d",
-		len(j.AllowedClients), len(j.DisallowedClients), len(j.BlockedHosts))
 }

internal/dnsforward/dns.go

@@ -443,24 +443,20 @@ func processFilteringBeforeRequest(ctx *dnsContext) (rc resultCode) {
 		return resultCodeSuccess // response is already set - nothing to do
 	}
 
-	s.RLock()
-	// Synchronize access to s.dnsFilter so it won't be suddenly uninitialized while in use.
-	// This could happen after proxy server has been stopped, but its workers are not yet exited.
-	//
-	// A better approach is for proxy.Stop() to wait until all its workers exit,
-	//  but this would require the Upstream interface to have Close() function
-	//  (to prevent from hanging while waiting for unresponsive DNS server to respond).
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
 
-	var err error
 	ctx.protectionEnabled = s.conf.ProtectionEnabled && s.dnsFilter != nil
-	if ctx.protectionEnabled {
-		if ctx.setts == nil {
-			ctx.setts = s.getClientRequestFilteringSettings(ctx)
-		}
-		ctx.result, err = s.filterDNSRequest(ctx)
+	if !ctx.protectionEnabled {
+		return resultCodeSuccess
 	}
-	s.RUnlock()
 
+	if ctx.setts == nil {
+		ctx.setts = s.getClientRequestFilteringSettings(ctx)
+	}
+
+	var err error
+	ctx.result, err = s.filterDNSRequest(ctx)
 	if err != nil {
 		ctx.err = err
 

internal/dnsforward/dnsforward.go

@@ -89,8 +89,9 @@ type Server struct {
 
 	isRunning bool
 
-	sync.RWMutex
 	conf ServerConfig
+	// serverLock protects Server.
+	serverLock sync.RWMutex
 }
 
 // defaultLocalDomainSuffix is the default suffix used to detect internal hosts
@@ -169,23 +170,24 @@ func NewCustomServer(internalProxy *proxy.Proxy) *Server {
 
 // Close - close object
 func (s *Server) Close() {
-	s.Lock()
+	s.serverLock.Lock()
+	defer s.serverLock.Unlock()
+
 	s.dnsFilter = nil
 	s.stats = nil
 	s.queryLog = nil
 	s.dnsProxy = nil
 
-	err := s.ipset.Close()
-	if err != nil {
+	if err := s.ipset.Close(); err != nil {
 		log.Error("closing ipset: %s", err)
 	}
-
-	s.Unlock()
 }
 
 // WriteDiskConfig - write configuration
 func (s *Server) WriteDiskConfig(c *FilteringConfig) {
-	s.RLock()
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
+
 	sc := s.conf.FilteringConfig
 	*c = sc
 	c.RatelimitWhitelist = aghstrings.CloneSlice(sc.RatelimitWhitelist)
@@ -194,13 +196,12 @@ func (s *Server) WriteDiskConfig(c *FilteringConfig) {
 	c.DisallowedClients = aghstrings.CloneSlice(sc.DisallowedClients)
 	c.BlockedHosts = aghstrings.CloneSlice(sc.BlockedHosts)
 	c.UpstreamDNS = aghstrings.CloneSlice(sc.UpstreamDNS)
-	s.RUnlock()
 }
 
 // RDNSSettings returns the copy of actual RDNS configuration.
 func (s *Server) RDNSSettings() (localPTRResolvers []string, resolveClients bool) {
-	s.RLock()
-	defer s.RUnlock()
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
 
 	return aghstrings.CloneSlice(s.conf.LocalPTRResolvers), s.conf.ResolveClients
 }
@@ -210,8 +211,9 @@ func (s *Server) RDNSSettings() (localPTRResolvers []string, resolveClients bool
 // Query log and Stats are not updated.
 // This method may be called before Start().
 func (s *Server) Resolve(host string) ([]net.IPAddr, error) {
-	s.RLock()
-	defer s.RUnlock()
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
+
 	return s.internalProxy.LookupIPAddr(host)
 }
 
@@ -234,8 +236,8 @@ const (
 
 // Exchange implements the RDNSExchanger interface for *Server.
 func (s *Server) Exchange(ip net.IP) (host string, err error) {
-	s.RLock()
-	defer s.RUnlock()
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
 
 	if !s.conf.ResolveClients {
 		return "", nil
@@ -286,8 +288,9 @@ func (s *Server) Exchange(ip net.IP) (host string, err error) {
 
 // Start starts the DNS server.
 func (s *Server) Start() error {
-	s.Lock()
-	defer s.Unlock()
+	s.serverLock.Lock()
+	defer s.serverLock.Unlock()
+
 	return s.startLocked()
 }
 
@@ -472,8 +475,9 @@ func (s *Server) Prepare(config *ServerConfig) error {
 
 // Stop stops the DNS server.
 func (s *Server) Stop() error {
-	s.Lock()
-	defer s.Unlock()
+	s.serverLock.Lock()
+	defer s.serverLock.Unlock()
+
 	return s.stopLocked()
 }
 
@@ -490,17 +494,18 @@ func (s *Server) stopLocked() error {
 	return nil
 }
 
-// IsRunning returns true if the DNS server is running
+// IsRunning returns true if the DNS server is running.
 func (s *Server) IsRunning() bool {
-	s.RLock()
-	defer s.RUnlock()
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
+
 	return s.isRunning
 }
 
-// Reconfigure applies the new configuration to the DNS server
+// Reconfigure applies the new configuration to the DNS server.
 func (s *Server) Reconfigure(config *ServerConfig) error {
-	s.Lock()
-	defer s.Unlock()
+	s.serverLock.Lock()
+	defer s.serverLock.Unlock()
 
 	log.Print("Start reconfiguring the server")
 	err := s.stopLocked()
@@ -525,14 +530,20 @@ func (s *Server) Reconfigure(config *ServerConfig) error {
 	return nil
 }
 
-// ServeHTTP is a HTTP handler method we use to provide DNS-over-HTTPS
+// ServeHTTP is a HTTP handler method we use to provide DNS-over-HTTPS.
 func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	s.RLock()
-	p := s.dnsProxy
-	s.RUnlock()
-	if p != nil { // an attempt to protect against race in case we're here after Close() was called
-		p.ServeHTTP(w, r)
-	}
+	var p *proxy.Proxy
+
+	defer func() {
+		if p != nil {
+			p.ServeHTTP(w, r)
+		}
+	}()
+
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
+
+	p = s.dnsProxy
 }
 
 // IsBlockedIP - return TRUE if this client should be blocked

internal/dnsforward/dnsforward_test.go

@@ -86,8 +86,8 @@ func createTestServer(
 	err = s.Prepare(nil)
 	require.NoError(t, err)
 
-	s.Lock()
-	defer s.Unlock()
+	s.serverLock.Lock()
+	defer s.serverLock.Unlock()
 
 	if localUps != nil {
 		s.localResolvers.Config.UpstreamConfig.Upstreams = []upstream.Upstream{localUps}

internal/dnsforward/filter.go

@@ -117,8 +117,40 @@ func (s *Server) filterDNSRequest(ctx *dnsContext) (*filtering.Result, error) {
 	return &res, err
 }
 
-// If response contains CNAME, A or AAAA records, we apply filtering to each canonical host name or IP address.
-// If this is a match, we set a new response in d.Res and return.
+// checkHostRulesSync checks host if needed performing an appropriate locks.  It
+// also synchronizes access to s.dnsFilter so it won't be suddenly uninitialized
+// while in use.  This could happen after proxy server has been stopped, but its
+// workers aren't exited yet.
+//
+// TODO(e.burkov): A better approach would be making Stop method waiting for all
+// its workers finished.  But it would require the upstream.Upstream to have the
+// Close method to prevent from hanging while waiting for unresponsive server to
+// respond.
+func (s *Server) checkHostRulesSync(host string, qtype uint16, setts *filtering.Settings) (
+	r *filtering.Result,
+	err error,
+) {
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
+
+	if s.dnsFilter == nil {
+		return nil, nil
+	}
+
+	var res filtering.Result
+	// Checking dnsFilter for nil is already done before calling the
+	// filterDNSResponse method.
+	res, err = s.dnsFilter.CheckHostRules(host, qtype, setts)
+	if err != nil {
+		return nil, err
+	}
+
+	return &res, err
+}
+
+// If response contains CNAME, A or AAAA records, we apply filtering to each
+// canonical host name or IP address.  If this is a match, we set a new response
+// in d.Res and return.
 func (s *Server) filterDNSResponse(ctx *dnsContext) (*filtering.Result, error) {
 	d := ctx.proxyCtx
 	for _, a := range d.Res.Answer {
@@ -141,22 +173,16 @@ func (s *Server) filterDNSResponse(ctx *dnsContext) (*filtering.Result, error) {
 			continue
 		}
 
-		s.RLock()
-		// Synchronize access to s.dnsFilter so it won't be suddenly uninitialized while in use.
-		// This could happen after proxy server has been stopped, but its workers are not yet exited.
-		if !s.conf.ProtectionEnabled || s.dnsFilter == nil {
-			s.RUnlock()
-			continue
-		}
-		res, err := s.dnsFilter.CheckHostRules(host, d.Req.Question[0].Qtype, ctx.setts)
-		s.RUnlock()
-
+		res, err := s.checkHostRulesSync(host, d.Req.Question[0].Qtype, ctx.setts)
 		if err != nil {
 			return nil, err
+		} else if res == nil {
+			continue
 		} else if res.IsFiltered {
-			d.Res = s.genDNSFilterMessage(d, &res)
+			d.Res = s.genDNSFilterMessage(d, res)
 			log.Debug("DNSFwd: Matched %s by response: %s", d.Req.Question[0].Name, host)
-			return &res, nil
+
+			return res, nil
 		}
 	}
 

internal/dnsforward/http.go

@@ -45,8 +45,8 @@ type dnsConfig struct {
 }
 
 func (s *Server) getDNSConfig() dnsConfig {
-	s.RLock()
-	defer s.RUnlock()
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
 
 	upstreams := aghstrings.CloneSliceOrEmpty(s.conf.UpstreamDNS)
 	upstreamFile := s.conf.UpstreamDNSFileName
@@ -280,8 +280,8 @@ func (s *Server) setConfigRestartable(dc dnsConfig) (restart bool) {
 }
 
 func (s *Server) setConfig(dc dnsConfig) (restart bool) {
-	s.Lock()
-	defer s.Unlock()
+	s.serverLock.Lock()
+	defer s.serverLock.Unlock()
 
 	if dc.ProtectionEnabled != nil {
 		s.conf.ProtectionEnabled = *dc.ProtectionEnabled

internal/dnsforward/stats.go

@@ -25,7 +25,9 @@ func processQueryLogsAndStats(ctx *dnsContext) (rc resultCode) {
 		shouldLog = false
 	}
 
-	s.RLock()
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
+
 	// Synchronize access to s.queryLog and s.stats so they won't be suddenly uninitialized while in use.
 	// This can happen after proxy server has been stopped, but its workers haven't yet exited.
 	if shouldLog && s.queryLog != nil {
@@ -61,7 +63,6 @@ func processQueryLogsAndStats(ctx *dnsContext) (rc resultCode) {
 	}
 
 	s.updateStats(ctx, elapsed, *ctx.result)
-	s.RUnlock()
 
 	return resultCodeSuccess
 }