From 570d149535f24c90dd1c944a0c6875349cbedddc Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Wed, 8 Sep 2021 17:20:46 +0300 Subject: [PATCH] proxy: fix index panic --- proxy/optimisticresolver.go | 1 - proxy/proxy.go | 14 +++++-- proxy/proxy_cache.go | 16 ++++---- proxy/proxy_test.go | 79 +++++++++++++++++++++++++++++++++++-- 4 files changed, 93 insertions(+), 17 deletions(-) diff --git a/proxy/optimisticresolver.go b/proxy/optimisticresolver.go index f70a4510d..2fb013445 100644 --- a/proxy/optimisticresolver.go +++ b/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) diff --git a/proxy/proxy.go b/proxy/proxy.go index 75035baa6..c6a6cbf7d 100644 --- a/proxy/proxy.go +++ b/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 diff --git a/proxy/proxy_cache.go b/proxy/proxy_cache.go index 0549de7d4..0ff395234 100644 --- a/proxy/proxy_cache.go +++ b/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) } } diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index 0653048e8..27f03780e 100644 --- a/proxy/proxy_test.go +++ b/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) {