diff --git a/internal/dnsforward/access.go b/internal/dnsforward/access.go index f27eb667249..62c1e35b9a2 100644 --- a/internal/dnsforward/access.go +++ b/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)) } diff --git a/internal/dnsforward/dns.go b/internal/dnsforward/dns.go index 5fbd4070769..44a3dd9c6c0 100644 --- a/internal/dnsforward/dns.go +++ b/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 diff --git a/internal/dnsforward/dnsforward.go b/internal/dnsforward/dnsforward.go index cf31500cc48..1c90e28f01a 100644 --- a/internal/dnsforward/dnsforward.go +++ b/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 diff --git a/internal/dnsforward/dnsforward_test.go b/internal/dnsforward/dnsforward_test.go index 8cf48ff49c2..5cdf315e1ce 100644 --- a/internal/dnsforward/dnsforward_test.go +++ b/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} diff --git a/internal/dnsforward/filter.go b/internal/dnsforward/filter.go index 1bacc98c358..660511c5a76 100644 --- a/internal/dnsforward/filter.go +++ b/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 } } diff --git a/internal/dnsforward/http.go b/internal/dnsforward/http.go index 35c1d9d4fac..36a6be0648a 100644 --- a/internal/dnsforward/http.go +++ b/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 diff --git a/internal/dnsforward/stats.go b/internal/dnsforward/stats.go index f03f6f6470b..d0eb39ca8e0 100644 --- a/internal/dnsforward/stats.go +++ b/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 }