Skip to content

Commit

Permalink
[backport] fix(nextlite): wrap DNSDecoder errors (#962)
Browse files Browse the repository at this point in the history 
The simplest fix is to wrap such errors in dnsdecoder.go.

Fixes ooni/probe#2317.
  • Loading branch information
@bassosimone
bassosimone committed Sep 14, 2022
1 parent 1b21ac5 commit a5681a6
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 16 deletions.
35 changes: 20 additions & 15 deletions internal/netxlite/dnsdecoder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -117,15 +122,15 @@ 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
}

// 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 {
Expand All @@ -143,15 +148,15 @@ func (r *dnsResponse) DecodeLookupHost() ([]string, error) {
}
}
if len(addrs) <= 0 {
return nil, ErrOODNSNoAnswer
return nil, dnsDecoderWrapError(ErrOODNSNoAnswer)
}
return addrs, nil
}

// 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 {
Expand All @@ -161,23 +166,23 @@ func (r *dnsResponse) DecodeNS() ([]*net.NS, error) {
}
}
if len(out) < 1 {
return nil, ErrOODNSNoAnswer
return nil, dnsDecoderWrapError(ErrOODNSNoAnswer)
}
return out, nil
}

// 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) {
case *dns.CNAME:
return avalue.Target, nil
}
}
return "", ErrOODNSNoAnswer
return "", dnsDecoderWrapError(ErrOODNSNoAnswer)
}

var _ model.DNSDecoder = &DNSDecoderMiekg{}
Expand Down
46 changes: 45 additions & 1 deletion internal/netxlite/dnsdecoder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand All @@ -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")
}
Expand All @@ -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")
}
Expand Down Expand Up @@ -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)
}
})
}
})
Expand All @@ -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")
}
Expand All @@ -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")
}
Expand Down Expand Up @@ -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")
}
Expand All @@ -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")
}
Expand Down Expand Up @@ -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")
}
Expand All @@ -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")
}
Expand Down Expand Up @@ -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")
}
Expand All @@ -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")
}
Expand Down Expand Up @@ -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")
}
Expand Down

0 comments on commit a5681a6

Please sign in to comment.