proxy: fix index panic

proxy/optimisticresolver.go

@@ -53,7 +53,6 @@ func (s *optimisticResolver) ResolveOnce(dctx *DNSContext, key []byte) {
 	if _, ok := s.reqs.LoadOrStore(keyHexed, unit{}); ok {
 		return
 	}
-
 	defer s.reqs.Delete(keyHexed)
 
 	ok, err := s.resolve(dctx)

proxy/proxy.go

@@ -413,7 +413,6 @@ func (p *Proxy) replyFromUpstream(d *DNSContext) (ok bool, err error) {
 	}
 
 	start := time.Now()
-
 	// Perform the DNS request.
 	var reply *dns.Msg
 	var u upstream.Upstream
@@ -434,13 +433,22 @@ func (p *Proxy) replyFromUpstream(d *DNSContext) (ok bool, err error) {
 		reply, u, err = upstream.ExchangeParallel(p.Fallbacks, req)
 	}
 
-	if reply != nil {
+	if ok = reply != nil; ok {
 		// This branch handles the successfully exchanged response.
 
 		// Set upstream that have resolved the request to DNSContext.
 		d.Upstream = u
 		p.setMinMaxTTL(reply)
-		ok = true
+
+		// Explicitly construct the question section since some
+		// upstreams may respond with invalidly constructed messages
+		// which cause out-of-range panics afterwards.
+		//
+		// See https://github.com/AdguardTeam/AdGuardHome/issues/3551.
+		if len(req.Question) > 0 && len(reply.Question) == 0 {
+			reply.Question = make([]dns.Question, 1)
+			reply.Question[0] = req.Question[0]
+		}
 	} else {
 		reply = p.genServerFailure(req)
 		d.hasEDNS0 = false

proxy/proxy_cache.go

@@ -1,9 +1,8 @@
 package proxy
 
 import (
-	"net"
-
 	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/miekg/dns"
 )
 
@@ -33,25 +32,24 @@ func (p *Proxy) replyFromCache(d *DNSContext) (hit bool) {
 
 	if p.cache.optimistic && hit && expired {
 		// Build the minimal copy of current context to avoid data race.
-		minCtxCopy := &DNSContext{
-			// It is only readed inside the optimistic resolver.
+		minCtxClone := &DNSContext{
+			// It is only read inside the optimistic resolver.
 			CustomUpstreamConfig: d.CustomUpstreamConfig,
 			ecsReqMask:           d.ecsReqMask,
 		}
 		if ecsReqIP := d.ecsReqIP; ecsReqIP != nil {
-			minCtxCopy.ecsReqIP = make(net.IP, len(ecsReqIP))
-			copy(minCtxCopy.ecsReqIP, ecsReqIP)
+			minCtxClone.ecsReqIP = netutil.CloneIP(ecsReqIP)
 		}
 		if d.Req != nil {
 			req := d.Req.Copy()
 			addDO(req)
-			minCtxCopy.Req = req
+			minCtxClone.Req = req
 		}
 
 		if !withSubnet {
-			go p.shortFlighter.ResolveOnce(minCtxCopy, key)
+			go p.shortFlighter.ResolveOnce(minCtxClone, key)
 		} else {
-			go p.shortFlighterWithSubnet.ResolveOnce(minCtxCopy, key)
+			go p.shortFlighterWithSubnet.ResolveOnce(minCtxClone, key)
 		}
 	}
 

proxy/proxy_test.go

@@ -732,8 +732,10 @@ func TestResponseInRequest(t *testing.T) {
 // Server must respond with SERVFAIL to requests without a Question
 func TestNoQuestion(t *testing.T) {
 	dnsProxy := createTestProxy(t, nil)
-	err := dnsProxy.Start()
-	assert.Nil(t, err)
+	require.NoError(t, dnsProxy.Start())
+	t.Cleanup(func() {
+		require.NoError(t, dnsProxy.Stop())
+	})
 
 	addr := dnsProxy.Addr(ProtoUDP)
 	client := &dns.Client{Net: "udp", Timeout: 500 * time.Millisecond}
@@ -742,10 +744,79 @@ func TestNoQuestion(t *testing.T) {
 	req.Question = nil
 
 	r, _, err := client.Exchange(req, addr.String())
-	assert.Nil(t, err)
+	require.NoError(t, err)
+
 	assert.Equal(t, dns.RcodeServerFailure, r.Rcode)
+}
 
-	_ = dnsProxy.Stop()
+// funcUpstream is a mock upstream implementation to simplify testing.  It
+// allows assigning custom Exchange and Address methods.
+type funcUpstream struct {
+	exchangeFunc func(m *dns.Msg) (resp *dns.Msg, err error)
+	addressFunc  func() (addr string)
+}
+
+// Exchange implements upstream.Upstream interface for *funcUpstream.
+func (wu *funcUpstream) Exchange(m *dns.Msg) (*dns.Msg, error) {
+	if wu.exchangeFunc == nil {
+		return nil, nil
+	}
+
+	return wu.exchangeFunc(m)
+}
+
+// Address implements upstream.Upstream interface for *funcUpstream.
+func (wu *funcUpstream) Address() string {
+	if wu.addressFunc == nil {
+		return "stub"
+	}
+
+	return wu.addressFunc()
+}
+
+func TestProxy_ReplyFromUpstream_badResponse(t *testing.T) {
+	dnsProxy := createTestProxy(t, nil)
+	require.NoError(t, dnsProxy.Start())
+	t.Cleanup(func() {
+		require.NoError(t, dnsProxy.Stop())
+	})
+
+	exchangeFunc := func(m *dns.Msg) (resp *dns.Msg, err error) {
+		resp = &dns.Msg{}
+		resp.SetReply(m)
+		hdr := dns.RR_Header{
+			Name:   m.Question[0].Name,
+			Class:  dns.ClassINET,
+			Rrtype: uint16(dns.TypeA),
+		}
+		resp.Answer = append(resp.Answer, &dns.A{
+			Hdr: hdr,
+			A:   net.IP{1, 2, 3, 4},
+		})
+		// Make the response invalid.
+		resp.Question = []dns.Question{}
+
+		return resp, nil
+	}
+	u := &funcUpstream{
+		exchangeFunc: exchangeFunc,
+	}
+
+	d := &DNSContext{
+		CustomUpstreamConfig: &UpstreamConfig{Upstreams: []upstream.Upstream{u}},
+		Req:                  createHostTestMessage("host"),
+		Addr: &net.TCPAddr{
+			IP: net.IP{1, 2, 3, 0},
+		},
+	}
+
+	var err error
+	require.NotPanics(t, func() {
+		err = dnsProxy.Resolve(d)
+	})
+	require.NoError(t, err)
+
+	assert.Equal(t, d.Req.Question[0], d.Res.Question[0])
 }
 
 func TestExchangeCustomUpstreamConfig(t *testing.T) {