From 7ee9f096d1ec5650ed5d62f938fbc3c2bdccd9ac Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 14 Sep 2022 13:47:20 +0200 Subject: [PATCH] fix(nextlite): wrap DNSDecoder errors (#962) The simplest fix is to wrap such errors in dnsdecoder.go. Fixes https://github.com/ooni/probe/issues/2317. --- internal/netxlite/dnsdecoder.go | 35 ++++++++++++--------- internal/netxlite/dnsdecoder_test.go | 46 +++++++++++++++++++++++++++- 2 files changed, 65 insertions(+), 16 deletions(-) diff --git a/internal/netxlite/dnsdecoder.go b/internal/netxlite/dnsdecoder.go index a2935b1..d00ef1c 100644 --- a/internal/netxlite/dnsdecoder.go +++ b/internal/netxlite/dnsdecoder.go @@ -23,17 +23,22 @@ var ( ErrDNSIsQuery = errors.New("ooresolver: expected response but received query") ) +// dnsDecoderWrapError ensures we wrap the returned errors +func dnsDecoderWrapError(err error) error { + return MaybeNewErrWrapper(ClassifyResolverError, ResolveOperation, err) +} + // DecodeResponse implements model.DNSDecoder.DecodeResponse. func (d *DNSDecoderMiekg) DecodeResponse(data []byte, query model.DNSQuery) (model.DNSResponse, error) { reply := &dns.Msg{} if err := reply.Unpack(data); err != nil { - return nil, err + return nil, dnsDecoderWrapError(err) } if !reply.Response { - return nil, ErrDNSIsQuery + return nil, dnsDecoderWrapError(ErrDNSIsQuery) } if reply.Id != query.ID() { - return nil, ErrDNSReplyWithWrongQueryID + return nil, dnsDecoderWrapError(ErrDNSReplyWithWrongQueryID) } resp := &dnsResponse{ bytes: data, @@ -77,20 +82,20 @@ func (r *dnsResponse) rcodeToError() error { case dns.RcodeSuccess: return nil case dns.RcodeNameError: - return ErrOODNSNoSuchHost + return dnsDecoderWrapError(ErrOODNSNoSuchHost) case dns.RcodeRefused: - return ErrOODNSRefused + return dnsDecoderWrapError(ErrOODNSRefused) case dns.RcodeServerFailure: - return ErrOODNSServfail + return dnsDecoderWrapError(ErrOODNSServfail) default: - return ErrOODNSMisbehaving + return dnsDecoderWrapError(ErrOODNSMisbehaving) } } // DecodeHTTPS implements model.DNSResponse.DecodeHTTPS. func (r *dnsResponse) DecodeHTTPS() (*model.HTTPSSvc, error) { if err := r.rcodeToError(); err != nil { - return nil, err + return nil, err // error already wrapped } out := &model.HTTPSSvc{ ALPN: []string{}, // ensure it's not nil @@ -117,7 +122,7 @@ func (r *dnsResponse) DecodeHTTPS() (*model.HTTPSSvc, error) { } } if len(out.IPv4) <= 0 && len(out.IPv6) <= 0 { - return nil, ErrOODNSNoAnswer + return nil, dnsDecoderWrapError(ErrOODNSNoAnswer) } return out, nil } @@ -125,7 +130,7 @@ func (r *dnsResponse) DecodeHTTPS() (*model.HTTPSSvc, error) { // DecodeLookupHost implements model.DNSResponse.DecodeLookupHost. func (r *dnsResponse) DecodeLookupHost() ([]string, error) { if err := r.rcodeToError(); err != nil { - return nil, err + return nil, err // error already wrapped } var addrs []string for _, answer := range r.msg.Answer { @@ -143,7 +148,7 @@ func (r *dnsResponse) DecodeLookupHost() ([]string, error) { } } if len(addrs) <= 0 { - return nil, ErrOODNSNoAnswer + return nil, dnsDecoderWrapError(ErrOODNSNoAnswer) } return addrs, nil } @@ -151,7 +156,7 @@ func (r *dnsResponse) DecodeLookupHost() ([]string, error) { // DecodeNS implements model.DNSResponse.DecodeNS. func (r *dnsResponse) DecodeNS() ([]*net.NS, error) { if err := r.rcodeToError(); err != nil { - return nil, err + return nil, err // error already wrapped } out := []*net.NS{} for _, answer := range r.msg.Answer { @@ -161,7 +166,7 @@ func (r *dnsResponse) DecodeNS() ([]*net.NS, error) { } } if len(out) < 1 { - return nil, ErrOODNSNoAnswer + return nil, dnsDecoderWrapError(ErrOODNSNoAnswer) } return out, nil } @@ -169,7 +174,7 @@ func (r *dnsResponse) DecodeNS() ([]*net.NS, error) { // DecodeCNAME implements model.DNSResponse.DecodeCNAME. func (r *dnsResponse) DecodeCNAME() (string, error) { if err := r.rcodeToError(); err != nil { - return "", err + return "", err // error already wrapped } for _, answer := range r.msg.Answer { switch avalue := answer.(type) { @@ -177,7 +182,7 @@ func (r *dnsResponse) DecodeCNAME() (string, error) { return avalue.Target, nil } } - return "", ErrOODNSNoAnswer + return "", dnsDecoderWrapError(ErrOODNSNoAnswer) } var _ model.DNSDecoder = &DNSDecoderMiekg{} diff --git a/internal/netxlite/dnsdecoder_test.go b/internal/netxlite/dnsdecoder_test.go index 0d1d48b..6929817 100644 --- a/internal/netxlite/dnsdecoder_test.go +++ b/internal/netxlite/dnsdecoder_test.go @@ -12,14 +12,22 @@ import ( "github.com/ooni/probe-cli/v3/internal/runtimex" ) +func dnsDecoderErrorIsWrapped(err error) bool { + var errwrapper *ErrWrapper + return errors.As(err, &errwrapper) +} + func TestDNSDecoderMiekg(t *testing.T) { t.Run("DecodeResponse", func(t *testing.T) { t.Run("UnpackError", func(t *testing.T) { d := &DNSDecoderMiekg{} resp, err := d.DecodeResponse(nil, &mocks.DNSQuery{}) - if err == nil || err.Error() != "dns: overflow unpacking uint16" { + if err == nil || err.Error() != "unknown_failure: dns: overflow unpacking uint16" { t.Fatal("unexpected error", err) } + if !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } if resp != nil { t.Fatal("expected nil resp here") } @@ -33,6 +41,9 @@ func TestDNSDecoderMiekg(t *testing.T) { if !errors.Is(err, ErrDNSIsQuery) { t.Fatal("unexpected err", err) } + if !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } if resp != nil { t.Fatal("expected nil resp here") } @@ -53,6 +64,9 @@ func TestDNSDecoderMiekg(t *testing.T) { if !errors.Is(err, ErrDNSReplyWithWrongQueryID) { t.Fatal("unexpected error", err) } + if !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } if resp != nil { t.Fatal("expected nil resp here") } @@ -163,6 +177,9 @@ func TestDNSDecoderMiekg(t *testing.T) { if !errors.Is(err, io.err) { t.Fatal("unexpected err", err) } + if err != nil && !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } }) } }) @@ -187,6 +204,9 @@ func TestDNSDecoderMiekg(t *testing.T) { if !errors.Is(err, ErrOODNSRefused) { t.Fatal("unexpected err", err) } + if !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } if https != nil { t.Fatal("expected nil https result") } @@ -210,6 +230,9 @@ func TestDNSDecoderMiekg(t *testing.T) { if !errors.Is(err, ErrOODNSNoAnswer) { t.Fatal("unexpected err", err) } + if !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } if https != nil { t.Fatal("expected nil https results") } @@ -268,6 +291,9 @@ func TestDNSDecoderMiekg(t *testing.T) { if !errors.Is(err, ErrOODNSRefused) { t.Fatal("unexpected err", err) } + if !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } if len(ns) > 0 { t.Fatal("expected empty ns result") } @@ -291,6 +317,9 @@ func TestDNSDecoderMiekg(t *testing.T) { if !errors.Is(err, ErrOODNSNoAnswer) { t.Fatal("unexpected err", err) } + if !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } if len(ns) > 0 { t.Fatal("expected empty ns results") } @@ -343,6 +372,9 @@ func TestDNSDecoderMiekg(t *testing.T) { if !errors.Is(err, ErrOODNSRefused) { t.Fatal("unexpected err", err) } + if !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } if len(addrs) > 0 { t.Fatal("expected empty addrs result") } @@ -366,6 +398,9 @@ func TestDNSDecoderMiekg(t *testing.T) { if !errors.Is(err, ErrOODNSNoAnswer) { t.Fatal("unexpected err", err) } + if !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } if len(addrs) > 0 { t.Fatal("expected empty ns results") } @@ -456,6 +491,9 @@ func TestDNSDecoderMiekg(t *testing.T) { if !errors.Is(err, ErrOODNSNoAnswer) { t.Fatal("not the error we expected", err) } + if !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } if len(addrs) > 0 { t.Fatal("expected no addrs here") } @@ -482,6 +520,9 @@ func TestDNSDecoderMiekg(t *testing.T) { if !errors.Is(err, ErrOODNSNoAnswer) { t.Fatal("not the error we expected", err) } + if !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } if len(addrs) > 0 { t.Fatal("expected no addrs here") } @@ -532,6 +573,9 @@ func TestDNSDecoderMiekg(t *testing.T) { if !errors.Is(err, ErrOODNSNoAnswer) { t.Fatal("unexpected err", err) } + if !dnsDecoderErrorIsWrapped(err) { + t.Fatal("unwrapped error", err) + } if cname != "" { t.Fatal("expected empty cname result") }