fix(file): fix data race in tree Elem.Name (#7574)

Eagerly set name in newElem and make Name() read-only to avoid
racy lazy writes under concurrent lookups. Add tests for empty-name
comparisons and concurrent access to Less/Name(). In addition,
regression tests to CloudDNS plugin.

Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>

plugin/clouddns/clouddns_test.go

@@ -4,8 +4,10 @@ import (
 	"context"
 	"errors"
 	"reflect"
+	"sync"
 	"testing"
 
+	"github.com/coredns/coredns/plugin/file"
 	"github.com/coredns/coredns/plugin/pkg/dnstest"
 	"github.com/coredns/coredns/plugin/pkg/fall"
 	"github.com/coredns/coredns/plugin/pkg/upstream"
@@ -325,3 +327,132 @@ func TestCloudDNS(t *testing.T) {
 		}
 	}
 }
+
+// TestCloudDNSConcurrentServeDNS stresses r.ServeDNS directly to trigger
+// concurrent Elem.Name() initialization from the file plugin.
+func TestCloudDNSConcurrentServeDNS(t *testing.T) {
+	ctx := context.Background()
+
+	r, err := New(ctx,
+		fakeGCPClient{},
+		map[string][]string{
+			"org.": {"sample-project-1:sample-zone-2", "sample-project-1:sample-zone-1"},
+		},
+		&upstream.Upstream{},
+	)
+	if err != nil {
+		t.Fatalf("Failed to create Cloud DNS: %v", err)
+	}
+	if err := r.Run(ctx); err != nil {
+		t.Fatalf("Failed to initialize Cloud DNS: %v", err)
+	}
+
+	queries := []struct {
+		qname string
+		qtype uint16
+	}{
+		{"example.org", dns.TypeA},
+		{"example.org", dns.TypeAAAA},
+		{"sample.example.org", dns.TypeA},
+		{"a.www.example.org", dns.TypeA},
+		{"split-example.org", dns.TypeA},
+		{"other-example.org", dns.TypeA},
+		{"_dummy._tcp.example.org.", dns.TypeSRV},
+	}
+
+	var wg sync.WaitGroup
+
+	// Concurrently refresh zones to race with Lookup reads.
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		for range 50 {
+			_ = r.updateZones(ctx)
+		}
+	}()
+
+	const workers = 32
+	const iterations = 200
+	for w := range workers {
+		wg.Add(1)
+		go func(id int) {
+			defer wg.Done()
+			for i := range iterations {
+				tc := queries[(id+i)%len(queries)]
+				req := new(dns.Msg)
+				req.SetQuestion(dns.Fqdn(tc.qname), tc.qtype)
+				rec := dnstest.NewRecorder(&test.ResponseWriter{})
+				code, err := r.ServeDNS(ctx, rec, req)
+				if err != nil {
+					t.Errorf("ServeDNS error: %v", err)
+					return
+				}
+				if code != dns.RcodeSuccess {
+					t.Errorf("unexpected return code: %v", code)
+					return
+				}
+			}
+		}(w)
+	}
+
+	wg.Wait()
+}
+
+// TestCloudDNSConcurrentLookupNameCache stresses hostedZone.z.Lookup
+// directly to trigger concurrent Elem.Name() initialization from the file plugin.
+func TestCloudDNSConcurrentLookupNameCache(t *testing.T) {
+	ctx := context.Background()
+
+	r, err := New(ctx,
+		fakeGCPClient{},
+		map[string][]string{
+			"org.": {"sample-project-1:sample-zone-2", "sample-project-1:sample-zone-1"},
+		},
+		&upstream.Upstream{})
+	if err != nil {
+		t.Fatalf("Failed to create Cloud DNS: %v", err)
+	}
+	if err := r.Run(ctx); err != nil {
+		t.Fatalf("Failed to initialize Cloud DNS: %v", err)
+	}
+
+	// Use a fixed set of qnames that exist in the zones to maximize reuse of the same Elems.
+	queries := []struct {
+		qname string
+		qtype uint16
+	}{
+		{"example.org.", dns.TypeA},
+		{"example.org.", dns.TypeAAAA},
+		{"sample.example.org.", dns.TypeA},
+		{"a.www.example.org.", dns.TypeA},
+		{"split-example.org.", dns.TypeA},
+		{"other-example.org.", dns.TypeA},
+		{"_dummy._tcp.example.org.", dns.TypeSRV},
+	}
+
+	// Fan-out goroutines that repeatedly call Lookup on the same Zone pointer.
+	const workers = 32
+	const iterations = 300
+
+	var wg sync.WaitGroup
+	for _, hostedZones := range r.zones {
+		for _, hz := range hostedZones {
+			z := hz.z
+			wg.Add(workers)
+			for w := range workers {
+				go func(id int, zptr *file.Zone) {
+					defer wg.Done()
+					for i := range iterations {
+						tc := queries[(id+i)%len(queries)]
+						req := new(dns.Msg)
+						req.SetQuestion(tc.qname, tc.qtype)
+						rec := dnstest.NewRecorder(&test.ResponseWriter{})
+						state := request.Request{W: rec, Req: req}
+						_, _, _, _ = zptr.Lookup(ctx, state, tc.qname)
+					}
+				}(w, z)
+			}
+		}
+	}
+	wg.Wait()
+}

plugin/file/tree/elem.go

@@ -12,6 +12,8 @@ type Elem struct {
 func newElem(rr dns.RR) *Elem {
 	e := Elem{m: make(map[uint16][]dns.RR)}
 	e.m[rr.Header().Rrtype] = []dns.RR{rr}
+	// Eagerly set the cached owner name to avoid racy lazy writes later.
+	e.name = rr.Header().Name
 	return &e
 }
 
@@ -56,12 +58,12 @@ func (e *Elem) All() []dns.RR {
 
 // Name returns the name for this node.
 func (e *Elem) Name() string {
+	// Read-only: name is eagerly set in newElem and should not be mutated here.
 	if e.name != "" {
 		return e.name
 	}
 	for _, rrs := range e.m {
-		e.name = rrs[0].Header().Name
-		return e.name
+		return rrs[0].Header().Name
 	}
 	return ""
 }

plugin/file/tree/elem_test.go

@@ -0,0 +1,41 @@
+package tree
+
+import (
+	"testing"
+
+	"github.com/miekg/dns"
+)
+
+// Test that Name() falls back to reading from the stored RRs when the cached name is empty.
+func TestElemName_FallbackWhenCachedEmpty(t *testing.T) {
+	rr, err := dns.NewRR("a.example. 3600 IN A 1.2.3.4")
+	if err != nil {
+		t.Fatalf("failed to create RR: %v", err)
+	}
+
+	// Build via newElem to ensure m is populated
+	e := newElem(rr)
+	got := e.Name()
+	want := "a.example."
+	if got != want {
+		t.Fatalf("unexpected name; want %q, got %q", want, got)
+	}
+
+	// clear the cached name
+	e.name = ""
+
+	got = e.Name()
+	want = "a.example."
+	if got != want {
+		t.Fatalf("unexpected name; want %q, got %q", want, got)
+	}
+
+	// clear the map
+	e.m = make(map[uint16][]dns.RR, 0)
+
+	got = e.Name()
+	want = ""
+	if got != want {
+		t.Fatalf("unexpected name after clearing RR map; want %q, got %q", want, got)
+	}
+}

plugin/file/tree/less_test.go

@@ -3,7 +3,10 @@ package tree
 import (
 	"sort"
 	"strings"
+	"sync"
 	"testing"
+
+	"github.com/miekg/dns"
 )
 
 type set []string
@@ -78,3 +81,41 @@ Tests:
 		}
 	}
 }
+
+func TestLess_EmptyVsName(t *testing.T) {
+	if d := less("", "a."); d >= 0 {
+		t.Fatalf("expected < 0, got %d", d)
+	}
+	if d := less("a.", ""); d <= 0 {
+		t.Fatalf("expected > 0, got %d", d)
+	}
+}
+
+func TestLess_EmptyVsEmpty(t *testing.T) {
+	if d := less("", ""); d != 0 {
+		t.Fatalf("expected 0, got %d", d)
+	}
+}
+
+// Test that concurrent calls to Less (which calls Elem.Name) do not race or panic.
+// See issue #7561 for reference.
+func TestLess_ConcurrentNameAccess(t *testing.T) {
+	rr, err := dns.NewRR("a.example. 3600 IN A 1.2.3.4")
+	if err != nil {
+		t.Fatalf("failed to create RR: %v", err)
+	}
+	e := newElem(rr)
+
+	const n = 200
+	var wg sync.WaitGroup
+	wg.Add(n)
+	for range n {
+		go func() {
+			defer wg.Done()
+			// Compare the same name repeatedly; previously this could race due to lazy Name() writes.
+			_ = Less(e, "a.example.")
+			_ = e.Name()
+		}()
+	}
+	wg.Wait()
+}