Skip to content

Commit 94f48dd

Browse files
iangudgerbradfitz
iangudger
authored and
bradfitz
committed
net: fail fast for DNS rcode success with no answers of requested type
DNS responses which do not contain answers of the requested type return errNoSuchHost, the same error as rcode name error. Prior to golang.org/cl/37879, both cases resulted in no additional name servers being consulted for the question. That CL changed the behavior for both cases. Issue #25336 was filed about the rcode name error case and golang.org/cl/113815 fixed it. This CL fixes the no answers of requested type case as well. Fixes #27525 Change-Id: I52fadedcd195f16adf62646b76bea2ab3b15d117 Reviewed-on: https://go-review.googlesource.com/133675 Run-TryBot: Ian Gudger <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Brad Fitzpatrick <[email protected]>
1 parent da0d1a4 commit 94f48dd

File tree

2 files changed

+158
-100
lines changed

2 files changed

+158
-100
lines changed

‎src/net/dnsclient_unix.go

+70-45
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,20 @@ import (
2727
"golang_org/x/net/dns/dnsmessage"
2828
)
2929

30+
var (
31+
errLameReferral = errors.New("lame referral")
32+
errCannotUnmarshalDNSMessage = errors.New("cannot unmarshal DNS message")
33+
errCannotMarshalDNSMessage = errors.New("cannot marshal DNS message")
34+
errServerMisbehaving = errors.New("server misbehaving")
35+
errInvalidDNSResponse = errors.New("invalid DNS response")
36+
errNoAnswerFromDNSServer = errors.New("no answer from DNS server")
37+
38+
// errServerTemporarlyMisbehaving is like errServerMisbehaving, except
39+
// that when it gets translated to a DNSError, the IsTemporary field
40+
// gets set to true.
41+
errServerTemporarlyMisbehaving = errors.New("server misbehaving")
42+
)
43+
3044
func newRequest(q dnsmessage.Question) (id uint16, udpReq, tcpReq []byte, err error) {
3145
id = uint16(rand.Int()) ^ uint16(time.Now().UnixNano())
3246
b := dnsmessage.NewBuilder(make([]byte, 2, 514), dnsmessage.Header{ID: id, RecursionDesired: true})
@@ -105,14 +119,14 @@ func dnsStreamRoundTrip(c Conn, id uint16, query dnsmessage.Question, b []byte)
105119
var p dnsmessage.Parser
106120
h, err := p.Start(b[:n])
107121
if err != nil {
108-
return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("cannot unmarshal DNS message")
122+
return dnsmessage.Parser{}, dnsmessage.Header{}, errCannotUnmarshalDNSMessage
109123
}
110124
q, err := p.Question()
111125
if err != nil {
112-
return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("cannot unmarshal DNS message")
126+
return dnsmessage.Parser{}, dnsmessage.Header{}, errCannotUnmarshalDNSMessage
113127
}
114128
if !checkResponse(id, query, h, q) {
115-
return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("invalid DNS response")
129+
return dnsmessage.Parser{}, dnsmessage.Header{}, errInvalidDNSResponse
116130
}
117131
return p, h, nil
118132
}
@@ -122,7 +136,7 @@ func (r *Resolver) exchange(ctx context.Context, server string, q dnsmessage.Que
122136
q.Class = dnsmessage.ClassINET
123137
id, udpReq, tcpReq, err := newRequest(q)
124138
if err != nil {
125-
return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("cannot marshal DNS message")
139+
return dnsmessage.Parser{}, dnsmessage.Header{}, errCannotMarshalDNSMessage
126140
}
127141
for _, network := range []string{"udp", "tcp"} {
128142
ctx, cancel := context.WithDeadline(ctx, time.Now().Add(timeout))
@@ -147,31 +161,31 @@ func (r *Resolver) exchange(ctx context.Context, server string, q dnsmessage.Que
147161
return dnsmessage.Parser{}, dnsmessage.Header{}, mapErr(err)
148162
}
149163
if err := p.SkipQuestion(); err != dnsmessage.ErrSectionDone {
150-
return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("invalid DNS response")
164+
return dnsmessage.Parser{}, dnsmessage.Header{}, errInvalidDNSResponse
151165
}
152166
if h.Truncated { // see RFC 5966
153167
continue
154168
}
155169
return p, h, nil
156170
}
157-
return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("no answer from DNS server")
171+
return dnsmessage.Parser{}, dnsmessage.Header{}, errNoAnswerFromDNSServer
158172
}
159173

160174
// checkHeader performs basic sanity checks on the header.
161175
func checkHeader(p *dnsmessage.Parser, h dnsmessage.Header, name, server string) error {
176+
if h.RCode == dnsmessage.RCodeNameError {
177+
return errNoSuchHost
178+
}
179+
162180
_, err := p.AnswerHeader()
163181
if err != nil && err != dnsmessage.ErrSectionDone {
164-
return &DNSError{
165-
Err: "cannot unmarshal DNS message",
166-
Name: name,
167-
Server: server,
168-
}
182+
return errCannotUnmarshalDNSMessage
169183
}
170184

171185
// libresolv continues to the next server when it receives
172186
// an invalid referral response. See golang.org/issue/15434.
173187
if h.RCode == dnsmessage.RCodeSuccess && !h.Authoritative && !h.RecursionAvailable && err == dnsmessage.ErrSectionDone {
174-
return &DNSError{Err: "lame referral", Name: name, Server: server}
188+
return errLameReferral
175189
}
176190

177191
if h.RCode != dnsmessage.RCodeSuccess && h.RCode != dnsmessage.RCodeNameError {
@@ -180,11 +194,10 @@ func checkHeader(p *dnsmessage.Parser, h dnsmessage.Header, name, server string)
180194
// a name error and we didn't get success,
181195
// the server is behaving incorrectly or
182196
// having temporary trouble.
183-
err := &DNSError{Err: "server misbehaving", Name: name, Server: server}
184197
if h.RCode == dnsmessage.RCodeServerFailure {
185-
err.IsTemporary = true
198+
return errServerTemporarlyMisbehaving
186199
}
187-
return err
200+
return errServerMisbehaving
188201
}
189202

190203
return nil
@@ -194,28 +207,16 @@ func skipToAnswer(p *dnsmessage.Parser, qtype dnsmessage.Type, name, server stri
194207
for {
195208
h, err := p.AnswerHeader()
196209
if err == dnsmessage.ErrSectionDone {
197-
return &DNSError{
198-
Err: errNoSuchHost.Error(),
199-
Name: name,
200-
Server: server,
201-
}
210+
return errNoSuchHost
202211
}
203212
if err != nil {
204-
return &DNSError{
205-
Err: "cannot unmarshal DNS message",
206-
Name: name,
207-
Server: server,
208-
}
213+
return errCannotUnmarshalDNSMessage
209214
}
210215
if h.Type == qtype {
211216
return nil
212217
}
213218
if err := p.SkipAnswer(); err != nil {
214-
return &DNSError{
215-
Err: "cannot unmarshal DNS message",
216-
Name: name,
217-
Server: server,
218-
}
219+
return errCannotUnmarshalDNSMessage
219220
}
220221
}
221222
}
@@ -229,7 +230,7 @@ func (r *Resolver) tryOneName(ctx context.Context, cfg *dnsConfig, name string,
229230

230231
n, err := dnsmessage.NewName(name)
231232
if err != nil {
232-
return dnsmessage.Parser{}, "", errors.New("cannot marshal DNS message")
233+
return dnsmessage.Parser{}, "", errCannotMarshalDNSMessage
233234
}
234235
q := dnsmessage.Question{
235236
Name: n,
@@ -243,38 +244,62 @@ func (r *Resolver) tryOneName(ctx context.Context, cfg *dnsConfig, name string,
243244

244245
p, h, err := r.exchange(ctx, server, q, cfg.timeout)
245246
if err != nil {
246-
lastErr = &DNSError{
247+
dnsErr := &DNSError{
247248
Err: err.Error(),
248249
Name: name,
249250
Server: server,
250251
}
251252
if nerr, ok := err.(Error); ok && nerr.Timeout() {
252-
lastErr.(*DNSError).IsTimeout = true
253+
dnsErr.IsTimeout = true
253254
}
254255
// Set IsTemporary for socket-level errors. Note that this flag
255256
// may also be used to indicate a SERVFAIL response.
256257
if _, ok := err.(*OpError); ok {
257-
lastErr.(*DNSError).IsTemporary = true
258+
dnsErr.IsTemporary = true
258259
}
260+
lastErr = dnsErr
259261
continue
260262
}
261263

262-
// The name does not exist, so trying another server won't help.
263-
//
264-
// TODO: indicate this in a more obvious way, such as a field on DNSError?
265-
if h.RCode == dnsmessage.RCodeNameError {
266-
return dnsmessage.Parser{}, "", &DNSError{Err: errNoSuchHost.Error(), Name: name, Server: server}
267-
}
268-
269-
lastErr = checkHeader(&p, h, name, server)
270-
if lastErr != nil {
264+
if err := checkHeader(&p, h, name, server); err != nil {
265+
dnsErr := &DNSError{
266+
Err: err.Error(),
267+
Name: name,
268+
Server: server,
269+
}
270+
if err == errServerTemporarlyMisbehaving {
271+
dnsErr.IsTemporary = true
272+
}
273+
if err == errNoSuchHost {
274+
// The name does not exist, so trying
275+
// another server won't help.
276+
//
277+
// TODO: indicate this in a more
278+
// obvious way, such as a field on
279+
// DNSError?
280+
return p, server, dnsErr
281+
}
282+
lastErr = dnsErr
271283
continue
272284
}
273285

274-
lastErr = skipToAnswer(&p, qtype, name, server)
275-
if lastErr == nil {
286+
err = skipToAnswer(&p, qtype, name, server)
287+
if err == nil {
276288
return p, server, nil
277289
}
290+
lastErr = &DNSError{
291+
Err: err.Error(),
292+
Name: name,
293+
Server: server,
294+
}
295+
if err == errNoSuchHost {
296+
// The name does not exist, so trying another
297+
// server won't help.
298+
//
299+
// TODO: indicate this in a more obvious way,
300+
// such as a field on DNSError?
301+
return p, server, lastErr
302+
}
278303
}
279304
}
280305
return dnsmessage.Parser{}, "", lastErr

‎src/net/dnsclient_unix_test.go

+88-55
Original file line numberDiff line numberDiff line change
@@ -1427,28 +1427,35 @@ func TestDNSGoroutineRace(t *testing.T) {
14271427
}
14281428
}
14291429

1430+
func lookupWithFake(fake fakeDNSServer, name string, typ dnsmessage.Type) error {
1431+
r := Resolver{PreferGo: true, Dial: fake.DialContext}
1432+
1433+
resolvConf.mu.RLock()
1434+
conf := resolvConf.dnsConfig
1435+
resolvConf.mu.RUnlock()
1436+
1437+
ctx, cancel := context.WithCancel(context.Background())
1438+
defer cancel()
1439+
1440+
_, _, err := r.tryOneName(ctx, conf, name, typ)
1441+
return err
1442+
}
1443+
14301444
// Issue 8434: verify that Temporary returns true on an error when rcode
14311445
// is SERVFAIL
14321446
func TestIssue8434(t *testing.T) {
1433-
msg := dnsmessage.Message{
1434-
Header: dnsmessage.Header{
1435-
RCode: dnsmessage.RCodeServerFailure,
1447+
err := lookupWithFake(fakeDNSServer{
1448+
rh: func(n, _ string, q dnsmessage.Message, _ time.Time) (dnsmessage.Message, error) {
1449+
return dnsmessage.Message{
1450+
Header: dnsmessage.Header{
1451+
ID: q.ID,
1452+
Response: true,
1453+
RCode: dnsmessage.RCodeServerFailure,
1454+
},
1455+
Questions: q.Questions,
1456+
}, nil
14361457
},
1437-
}
1438-
b, err := msg.Pack()
1439-
if err != nil {
1440-
t.Fatal("Pack failed:", err)
1441-
}
1442-
var p dnsmessage.Parser
1443-
h, err := p.Start(b)
1444-
if err != nil {
1445-
t.Fatal("Start failed:", err)
1446-
}
1447-
if err := p.SkipAllQuestions(); err != nil {
1448-
t.Fatal("SkipAllQuestions failed:", err)
1449-
}
1450-
1451-
err = checkHeader(&p, h, "golang.org", "foo:53")
1458+
}, "golang.org.", dnsmessage.TypeALL)
14521459
if err == nil {
14531460
t.Fatal("expected an error")
14541461
}
@@ -1464,50 +1471,76 @@ func TestIssue8434(t *testing.T) {
14641471
}
14651472
}
14661473

1467-
// Issue 12778: verify that NXDOMAIN without RA bit errors as
1468-
// "no such host" and not "server misbehaving"
1474+
// TestNoSuchHost verifies that tryOneName works correctly when the domain does
1475+
// not exist.
1476+
//
1477+
// Issue 12778: verify that NXDOMAIN without RA bit errors as "no such host"
1478+
// and not "server misbehaving"
14691479
//
14701480
// Issue 25336: verify that NXDOMAIN errors fail fast.
1471-
func TestIssue12778(t *testing.T) {
1472-
lookups := 0
1473-
fake := fakeDNSServer{
1474-
rh: func(n, _ string, q dnsmessage.Message, _ time.Time) (dnsmessage.Message, error) {
1475-
lookups++
1476-
return dnsmessage.Message{
1477-
Header: dnsmessage.Header{
1478-
ID: q.ID,
1479-
Response: true,
1480-
RCode: dnsmessage.RCodeNameError,
1481-
RecursionAvailable: false,
1482-
},
1483-
Questions: q.Questions,
1484-
}, nil
1481+
//
1482+
// Issue 27525: verify that empty answers fail fast.
1483+
func TestNoSuchHost(t *testing.T) {
1484+
tests := []struct {
1485+
name string
1486+
f func(string, string, dnsmessage.Message, time.Time) (dnsmessage.Message, error)
1487+
}{
1488+
{
1489+
"NXDOMAIN",
1490+
func(n, _ string, q dnsmessage.Message, _ time.Time) (dnsmessage.Message, error) {
1491+
return dnsmessage.Message{
1492+
Header: dnsmessage.Header{
1493+
ID: q.ID,
1494+
Response: true,
1495+
RCode: dnsmessage.RCodeNameError,
1496+
RecursionAvailable: false,
1497+
},
1498+
Questions: q.Questions,
1499+
}, nil
1500+
},
1501+
},
1502+
{
1503+
"no answers",
1504+
func(n, _ string, q dnsmessage.Message, _ time.Time) (dnsmessage.Message, error) {
1505+
return dnsmessage.Message{
1506+
Header: dnsmessage.Header{
1507+
ID: q.ID,
1508+
Response: true,
1509+
RCode: dnsmessage.RCodeSuccess,
1510+
RecursionAvailable: false,
1511+
Authoritative: true,
1512+
},
1513+
Questions: q.Questions,
1514+
}, nil
1515+
},
14851516
},
14861517
}
1487-
r := Resolver{PreferGo: true, Dial: fake.DialContext}
1488-
1489-
resolvConf.mu.RLock()
1490-
conf := resolvConf.dnsConfig
1491-
resolvConf.mu.RUnlock()
14921518

1493-
ctx, cancel := context.WithCancel(context.Background())
1494-
defer cancel()
1495-
1496-
_, _, err := r.tryOneName(ctx, conf, ".", dnsmessage.TypeALL)
1519+
for _, test := range tests {
1520+
t.Run(test.name, func(t *testing.T) {
1521+
lookups := 0
1522+
err := lookupWithFake(fakeDNSServer{
1523+
rh: func(n, s string, q dnsmessage.Message, d time.Time) (dnsmessage.Message, error) {
1524+
lookups++
1525+
return test.f(n, s, q, d)
1526+
},
1527+
}, ".", dnsmessage.TypeALL)
14971528

1498-
if lookups != 1 {
1499-
t.Errorf("got %d lookups, wanted 1", lookups)
1500-
}
1529+
if lookups != 1 {
1530+
t.Errorf("got %d lookups, wanted 1", lookups)
1531+
}
15011532

1502-
if err == nil {
1503-
t.Fatal("expected an error")
1504-
}
1505-
de, ok := err.(*DNSError)
1506-
if !ok {
1507-
t.Fatalf("err = %#v; wanted a *net.DNSError", err)
1508-
}
1509-
if de.Err != errNoSuchHost.Error() {
1510-
t.Fatalf("Err = %#v; wanted %q", de.Err, errNoSuchHost.Error())
1533+
if err == nil {
1534+
t.Fatal("expected an error")
1535+
}
1536+
de, ok := err.(*DNSError)
1537+
if !ok {
1538+
t.Fatalf("err = %#v; wanted a *net.DNSError", err)
1539+
}
1540+
if de.Err != errNoSuchHost.Error() {
1541+
t.Fatalf("Err = %#v; wanted %q", de.Err, errNoSuchHost.Error())
1542+
}
1543+
})
15111544
}
15121545
}
15131546

0 commit comments

Comments
 (0)