querylog: fix race; refactor

internal/querylog/decode.go

@@ -247,55 +247,20 @@ var resultHandlers = map[string]logEntryHandler{
 }
 
 func decodeResultRuleKey(key string, i int, dec *json.Decoder, ent *logEntry) {
+	var vToken json.Token
 	switch key {
 	case "FilterListID":
-		vToken, err := dec.Token()
-		if err != nil {
-			if err != io.EOF {
-				log.Debug("decodeResultRuleKey %s err: %s", key, err)
-			}
-
-			return
-		}
-
-		if len(ent.Result.Rules) < i+1 {
-			ent.Result.Rules = append(ent.Result.Rules, &filtering.ResultRule{})
-		}
-
+		ent.Result.Rules, vToken = decodeVTokenAndAddRule(key, i, dec, ent.Result.Rules)
 		if n, ok := vToken.(json.Number); ok {
 			ent.Result.Rules[i].FilterListID, _ = n.Int64()
 		}
 	case "IP":
-		vToken, err := dec.Token()
-		if err != nil {
-			if err != io.EOF {
-				log.Debug("decodeResultRuleKey %s err: %s", key, err)
-			}
-
-			return
-		}
-
-		if len(ent.Result.Rules) < i+1 {
-			ent.Result.Rules = append(ent.Result.Rules, &filtering.ResultRule{})
-		}
-
+		ent.Result.Rules, vToken = decodeVTokenAndAddRule(key, i, dec, ent.Result.Rules)
 		if ipStr, ok := vToken.(string); ok {
 			ent.Result.Rules[i].IP = net.ParseIP(ipStr)
 		}
 	case "Text":
-		vToken, err := dec.Token()
-		if err != nil {
-			if err != io.EOF {
-				log.Debug("decodeResultRuleKey %s err: %s", key, err)
-			}
-
-			return
-		}
-
-		if len(ent.Result.Rules) < i+1 {
-			ent.Result.Rules = append(ent.Result.Rules, &filtering.ResultRule{})
-		}
-
+		ent.Result.Rules, vToken = decodeVTokenAndAddRule(key, i, dec, ent.Result.Rules)
 		if s, ok := vToken.(string); ok {
 			ent.Result.Rules[i].Text = s
 		}
@@ -304,6 +269,30 @@ func decodeResultRuleKey(key string, i int, dec *json.Decoder, ent *logEntry) {
 	}
 }
 
+func decodeVTokenAndAddRule(
+	key string,
+	i int,
+	dec *json.Decoder,
+	rules []*filtering.ResultRule,
+) (newRules []*filtering.ResultRule, vToken json.Token) {
+	newRules = rules
+
+	vToken, err := dec.Token()
+	if err != nil {
+		if err != io.EOF {
+			log.Debug("decodeResultRuleKey %s err: %s", key, err)
+		}
+
+		return newRules, nil
+	}
+
+	if len(rules) < i+1 {
+		newRules = append(newRules, &filtering.ResultRule{})
+	}
+
+	return newRules, vToken
+}
+
 func decodeResultRules(dec *json.Decoder, ent *logEntry) {
 	for {
 		delimToken, err := dec.Token()

internal/querylog/http.go

@@ -54,10 +54,7 @@ func (l *queryLog) handleQueryLog(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// search for the log entries
 	entries, oldest := l.search(params)
-
-	// convert log entries to JSON
 	data := l.entriesToJSON(entries, oldest)
 
 	_ = aghhttp.WriteJSONResponse(w, r, data)

internal/querylog/json.go

@@ -1,7 +1,6 @@
 package querylog
 
 import (
-	"fmt"
 	"strconv"
 	"strings"
 	"time"
@@ -159,55 +158,23 @@ type dnsAnswer struct {
 	TTL   uint32 `json:"ttl"`
 }
 
-func answerToMap(a *dns.Msg) (answers []*dnsAnswer) {
-	if a == nil || len(a.Answer) == 0 {
+func answerToMap(msg *dns.Msg) (answers []*dnsAnswer) {
+	if msg == nil || len(msg.Answer) == 0 {
 		return nil
 	}
 
-	answers = make([]*dnsAnswer, 0, len(a.Answer))
-	for _, k := range a.Answer {
-		header := k.Header()
-		answer := &dnsAnswer{
+	answers = make([]*dnsAnswer, 0, len(msg.Answer))
+	for _, rr := range msg.Answer {
+		header := rr.Header()
+		a := &dnsAnswer{
 			Type: dns.TypeToString[header.Rrtype],
-			TTL:  header.Ttl,
+			// Remove the header string from the answer value since it's mostly
+			// unnecessary in the log.
+			Value: strings.TrimPrefix(rr.String(), header.String()),
+			TTL:   header.Ttl,
 		}
 
-		// Some special treatment for some well-known types.
-		//
-		// TODO(a.garipov): Consider just calling String() for everyone
-		// instead.
-		switch v := k.(type) {
-		case nil:
-			// Probably unlikely, but go on.
-		case *dns.A:
-			answer.Value = v.A.String()
-		case *dns.AAAA:
-			answer.Value = v.AAAA.String()
-		case *dns.MX:
-			answer.Value = fmt.Sprintf("%v %v", v.Preference, v.Mx)
-		case *dns.CNAME:
-			answer.Value = v.Target
-		case *dns.NS:
-			answer.Value = v.Ns
-		case *dns.SPF:
-			answer.Value = strings.Join(v.Txt, "\n")
-		case *dns.TXT:
-			answer.Value = strings.Join(v.Txt, "\n")
-		case *dns.PTR:
-			answer.Value = v.Ptr
-		case *dns.SOA:
-			answer.Value = fmt.Sprintf("%v %v %v %v %v %v %v", v.Ns, v.Mbox, v.Serial, v.Refresh, v.Retry, v.Expire, v.Minttl)
-		case *dns.CAA:
-			answer.Value = fmt.Sprintf("%v %v \"%v\"", v.Flag, v.Tag, v.Value)
-		case *dns.HINFO:
-			answer.Value = fmt.Sprintf("\"%v\" \"%v\"", v.Cpu, v.Os)
-		case *dns.RRSIG:
-			answer.Value = fmt.Sprintf("%v %v %v %v %v %v %v %v %v", dns.TypeToString[v.TypeCovered], v.Algorithm, v.Labels, v.OrigTtl, v.Expiration, v.Inception, v.KeyTag, v.SignerName, v.Signature)
-		default:
-			answer.Value = v.String()
-		}
-
-		answers = append(answers, answer)
+		answers = append(answers, a)
 	}
 
 	return answers

internal/querylog/qlog.go

@@ -31,7 +31,8 @@ type queryLog struct {
 
 	// bufferLock protects buffer.
 	bufferLock sync.RWMutex
-	// buffer contains recent log entries.
+	// buffer contains recent log entries.  The entries in this buffer must not
+	// be modified.
 	buffer []*logEntry
 
 	fileFlushLock sync.Mutex // synchronize a file-flushing goroutine and main thread
@@ -100,6 +101,13 @@ type logEntry struct {
 	AuthenticatedData bool `json:"AD,omitempty"`
 }
 
+// shallowClone returns a shallow clone of e.
+func (e *logEntry) shallowClone() (clone *logEntry) {
+	cloneVal := *e
+
+	return &cloneVal
+}
+
 func (l *queryLog) Start() {
 	if l.conf.HTTPRegister != nil {
 		l.initWeb()

internal/querylog/search.go

@@ -52,7 +52,9 @@ func (l *queryLog) searchMemory(params *searchParams, cache clientCache) (entrie
 	// Go through the buffer in the reverse order, from newer to older.
 	var err error
 	for i := len(l.buffer) - 1; i >= 0; i-- {
-		e := l.buffer[i]
+		// A shallow clone is enough, since the only thing that this loop
+		// modifies is the client field.
+		e := l.buffer[i].shallowClone()
 
 		e.client, err = l.client(e.ClientID, e.IP.String(), cache)
 		if err != nil {

internal/querylog/searchcriterion.go

@@ -1,6 +1,7 @@
 package querylog
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/filtering"
@@ -118,7 +119,7 @@ func (c *searchCriterion) match(entry *logEntry) bool {
 	case ctTerm:
 		return c.ctDomainOrClientCase(entry)
 	case ctFilteringStatus:
-		return c.ctFilteringStatusCase(entry.Result)
+		return c.ctFilteringStatusCase(entry.Result.Reason, entry.Result.IsFiltered)
 	}
 
 	return false
@@ -141,54 +142,70 @@ func (c *searchCriterion) ctDomainOrClientCase(e *logEntry) bool {
 	return ctDomainOrClientCaseNonStrict(c.value, c.asciiVal, clientID, name, host, ip)
 }
 
-func (c *searchCriterion) ctFilteringStatusCase(res filtering.Result) bool {
+// ctFilteringStatusCase returns true if the result matches the value.
+func (c *searchCriterion) ctFilteringStatusCase(
+	reason filtering.Reason,
+	isFiltered bool,
+) (matched bool) {
 	switch c.value {
 	case filteringStatusAll:
 		return true
-
-	case filteringStatusFiltered:
-		return res.IsFiltered ||
-			res.Reason.In(
-				filtering.NotFilteredAllowList,
-				filtering.Rewritten,
-				filtering.RewrittenAutoHosts,
-				filtering.RewrittenRule,
-			)
-
-	case filteringStatusBlocked:
-		return res.IsFiltered &&
-			res.Reason.In(filtering.FilteredBlockList, filtering.FilteredBlockedService)
-
-	case filteringStatusBlockedService:
-		return res.IsFiltered && res.Reason == filtering.FilteredBlockedService
-
-	case filteringStatusBlockedParental:
-		return res.IsFiltered && res.Reason == filtering.FilteredParental
-
-	case filteringStatusBlockedSafebrowsing:
-		return res.IsFiltered && res.Reason == filtering.FilteredSafeBrowsing
-
+	case
+		filteringStatusBlocked,
+		filteringStatusBlockedParental,
+		filteringStatusBlockedSafebrowsing,
+		filteringStatusBlockedService,
+		filteringStatusFiltered,
+		filteringStatusSafeSearch:
+		return isFiltered && c.isFilteredWithReason(reason)
 	case filteringStatusWhitelisted:
-		return res.Reason == filtering.NotFilteredAllowList
-
+		return reason == filtering.NotFilteredAllowList
 	case filteringStatusRewritten:
-		return res.Reason.In(
+		return reason.In(
 			filtering.Rewritten,
 			filtering.RewrittenAutoHosts,
 			filtering.RewrittenRule,
 		)
-
-	case filteringStatusSafeSearch:
-		return res.IsFiltered && res.Reason == filtering.FilteredSafeSearch
-
 	case filteringStatusProcessed:
-		return !res.Reason.In(
+		return !reason.In(
 			filtering.FilteredBlockList,
 			filtering.FilteredBlockedService,
 			filtering.NotFilteredAllowList,
 		)
-
 	default:
 		return false
 	}
 }
+
+// isFilteredWithReason returns true if reason matches the criterion value.
+// c.value must be one of:
+//
+//   - filteringStatusBlocked
+//   - filteringStatusBlockedParental
+//   - filteringStatusBlockedSafebrowsing
+//   - filteringStatusBlockedService
+//   - filteringStatusFiltered
+//   - filteringStatusSafeSearch
+func (c *searchCriterion) isFilteredWithReason(reason filtering.Reason) (matched bool) {
+	switch c.value {
+	case filteringStatusBlocked:
+		return reason.In(filtering.FilteredBlockList, filtering.FilteredBlockedService)
+	case filteringStatusBlockedParental:
+		return reason == filtering.FilteredParental
+	case filteringStatusBlockedSafebrowsing:
+		return reason == filtering.FilteredSafeBrowsing
+	case filteringStatusBlockedService:
+		return reason == filtering.FilteredBlockedService
+	case filteringStatusFiltered:
+		return reason.In(
+			filtering.NotFilteredAllowList,
+			filtering.Rewritten,
+			filtering.RewrittenAutoHosts,
+			filtering.RewrittenRule,
+		)
+	case filteringStatusSafeSearch:
+		return reason == filtering.FilteredSafeSearch
+	default:
+		panic(fmt.Errorf("unexpected value %q", c.value))
+	}
+}

scripts/make/go-lint.sh

@@ -161,7 +161,7 @@ run_linter "$GO" vet ./...
 run_linter govulncheck ./...
 
 # Apply more lax standards to the code we haven't properly refactored yet.
-run_linter gocyclo --over 17 ./internal/querylog/
+run_linter gocyclo --over 14 ./internal/querylog/
 run_linter gocyclo --over 13\
 	./internal/dhcpd\
 	./internal/filtering/\