From 897b4ce64369def506754d58084a53d0b239adaf Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu Date: Mon, 17 Aug 2026 05:11:16 -0700 Subject: [PATCH] plugin/azure: don't hold zMu across zone Lookup (#8447) --- plugin/azure/azure.go | 7 ++- plugin/azure/azure_test.go | 108 +++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 1 deletion(-) diff --git a/plugin/azure/azure.go b/plugin/azure/azure.go index c7f8498ca..8d66905b7 100644 --- a/plugin/azure/azure.go +++ b/plugin/azure/azure.go @@ -319,10 +319,15 @@ func (h *Azure) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) m.Authoritative = true var result file.Result for _, z := range zones { + // Only the zone pointer itself needs to be guarded against a + // concurrent swap in updateZones; Lookup can run unlocked since it + // may block for a while resolving external names via upstream. h.zMu.RLock() - m.Answer, m.Ns, m.Extra, result = z.z.Lookup(ctx, state, qname) + zz := z.z h.zMu.RUnlock() + m.Answer, m.Ns, m.Extra, result = zz.Lookup(ctx, state, qname) + // record type exists for this name (NODATA). if len(m.Answer) != 0 || result == file.NoData { break diff --git a/plugin/azure/azure_test.go b/plugin/azure/azure_test.go index 9be2695f0..acaa4efa7 100644 --- a/plugin/azure/azure_test.go +++ b/plugin/azure/azure_test.go @@ -3,11 +3,16 @@ package azure import ( "context" "reflect" + "sync" "testing" + "time" + "github.com/coredns/coredns/core/dnsserver" + "github.com/coredns/coredns/plugin" "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" "github.com/coredns/coredns/plugin/test" "github.com/coredns/coredns/request" @@ -178,3 +183,106 @@ func TestAzure(t *testing.T) { } } } + +// TestServeDNSDoesNotHoldLockDuringUpstreamLookup guards against holding +// zMu.RLock() for the duration of a CNAME chase to an external name, which +// can block on a slow upstream response. Holding the lock that long starves +// the periodic zone update in updateZones (which needs zMu.Lock()), and +// every query behind it queues up, so a single slow upstream response can +// stall the whole plugin. +func TestServeDNSDoesNotHoldLockDuringUpstreamLookup(t *testing.T) { + const timeout = 5 * time.Second + + // entered signals that the upstream handler has been reached; release + // gates the handler's response so the test controls exactly when the + // in-flight query completes. releaseOnce lets every exit path (pass or + // fail) unblock the handler so no goroutine is left hanging. + entered := make(chan struct{}) + release := make(chan struct{}) + var releaseOnce sync.Once + releaseHandler := func() { releaseOnce.Do(func() { close(release) }) } + defer releaseHandler() + + cfg := &dnsserver.Config{ + Zone: ".", + Plugin: []plugin.Plugin{ + func(plugin.Handler) plugin.Handler { + return plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) { + close(entered) + <-release + m := new(dns.Msg) + m.SetReply(r) + m.Answer = []dns.RR{test.A("external.target. 300 IN A 5.6.7.8")} + w.WriteMsg(m) + return dns.RcodeSuccess, nil + }) + }, + }, + } + srv, err := dnsserver.NewServer("", []*dnsserver.Config{cfg}) + if err != nil { + t.Fatal(err) + } + ctx := context.WithValue(context.Background(), dnsserver.Key{}, srv) + + newZ := file.NewZone("example.org.", "") + newZ.Upstream = upstream.New() + for _, rr := range []string{ + "example.org. 300 IN SOA ns1-06.azure-dns.com. azuredns-hostmaster.microsoft.com. 1 3600 300 2419200 300", + "cname-ext.example.org. 300 IN CNAME external.target.", + } { + r, _ := dns.NewRR(rr) + newZ.Insert(r) + } + zs := make(map[string][]*zone) + zs["example.org."] = append(zs["example.org."], &zone{zone: "example.org.", z: newZ}) + + az := Azure{ + Next: testHandler(), + Fall: fall.Zero, + zoneNames: []string{"example.org."}, + zones: zs, + } + + req := new(dns.Msg) + req.SetQuestion("cname-ext.example.org.", dns.TypeA) + + done := make(chan struct{}) + go func() { + rec := dnstest.NewRecorder(&test.ResponseWriter{}) + az.ServeDNS(ctx, rec, req) + close(done) + }() + + select { + case <-entered: + // The query has reached the upstream handler and is now blocked on + // release, so a subsequent zMu.Lock() genuinely races the lookup. + case <-time.After(timeout): + t.Fatal("query never reached the upstream handler") + } + + lockAcquired := make(chan struct{}) + go func() { + // Stands in for the zone swap updateZones does every minute. + az.zMu.Lock() + zs["example.org."][0].z = file.NewZone("example.org.", "") + az.zMu.Unlock() + close(lockAcquired) + }() + + select { + case <-lockAcquired: + // zMu.Lock() wasn't blocked by the in-flight upstream lookup. + case <-time.After(timeout): + t.Fatal("zMu.Lock() blocked while a query awaited a slow upstream lookup") + } + + releaseHandler() + + select { + case <-done: + case <-time.After(timeout): + t.Fatal("ServeDNS did not complete after the upstream handler was released") + } +}