Repository: miekg/dns
Pull request #1155 (2020-09-02 22:27:55)

Ignore responses with unexpected IDs
This fixes the following problem:

At time 0, we send a query with ID X from port P.

At time T, we time out the query due to lack of response, and then send a different query with ID Y.  By coincidence, the new query is sent from the same port number P (since port numbers are only 16 bits, this can happen with non-negligible probability when making queries at a high rate).

At time T+epsilon, we receive a response to the original query. Since the ID in this response is X, not Y, `exchange` returns `ErrId`, preventing the second query from succeeding.

Since UDP does not have connections, there's no guarantee that a response that we receive is actually associated with the query that we sent - the ID is needed to make the association. Therefore, it's more appropriate to ignore replies with unexpected IDs that to consider them an error.  Accordingly, this pull request changes `exchange` to ignore responses with mismatched IDs.  `exchange` continues waiting for a response with the expected ID and returns it when it arrives.  

codecov-commenter: # [Codecov](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=h1) Report
> Merging [#1155](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=desc) into [master](https://codecov.io/gh/miekg/dns/commit/efdec214966e482deb90512f7bdf4ad40ca52b3c?el=desc) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/miekg/dns/pull/1155/graphs/tree.svg?width=650&height=150&src=pr&token=uT9fYPSeKT)](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master    #1155   +/-   ##
=======================================
  Coverage   55.89%   55.89%           
=======================================
  Files          41       41           
  Lines       10141    10142    +1     
=======================================
+ Hits         5668     5669    +1     
  Misses       3468     3468           
  Partials     1005     1005           
```


| [Impacted Files](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [client.go](https://codecov.io/gh/miekg/dns/pull/1155/diff?src=pr&el=tree#diff-Y2xpZW50Lmdv) | `70.20% <100.00%> (+0.15%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=footer). Last update [efdec21...ddbee29](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

miekg: [ Quoting <notifications@github.com> in "[miekg/dns] Ignore responses with u..." ]
>This fixes the following problem:

ErrID is returned in this case and the caller needs to check for this. There are other
checks that can be done as well, such as double checking if the case of the domain names
matches what was sent.

nacking this pr, as it may be useful for debug programs to actually ignore the ID.

/close

codecov-io: # [Codecov](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=h1) Report
> Merging [#1155](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=desc) into [master](https://codecov.io/gh/miekg/dns/commit/efdec214966e482deb90512f7bdf4ad40ca52b3c?el=desc) will **increase** coverage by `1.84%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/miekg/dns/pull/1155/graphs/tree.svg?width=650&height=150&src=pr&token=uT9fYPSeKT)](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1155      +/-   ##
==========================================
+ Coverage   55.89%   57.74%   +1.84%     
==========================================
  Files          41       42       +1     
  Lines       10141    11413    +1272     
==========================================
+ Hits         5668     6590     +922     
- Misses       3468     3696     +228     
- Partials     1005     1127     +122     
```


| [Impacted Files](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [client.go](https://codecov.io/gh/miekg/dns/pull/1155/diff?src=pr&el=tree#diff-Y2xpZW50Lmdv) | `70.35% <100.00%> (+0.30%)` | :arrow_up: |
| [types.go](https://codecov.io/gh/miekg/dns/pull/1155/diff?src=pr&el=tree#diff-dHlwZXMuZ28=) | `74.44% <0.00%> (-0.20%)` | :arrow_down: |
| [svcb.go](https://codecov.io/gh/miekg/dns/pull/1155/diff?src=pr&el=tree#diff-c3ZjYi5nbw==) | `87.66% <0.00%> (ø)` | |
| [scan.go](https://codecov.io/gh/miekg/dns/pull/1155/diff?src=pr&el=tree#diff-c2Nhbi5nbw==) | `84.85% <0.00%> (+0.28%)` | :arrow_up: |
| [server.go](https://codecov.io/gh/miekg/dns/pull/1155/diff?src=pr&el=tree#diff-c2VydmVyLmdv) | `70.21% <0.00%> (+0.54%)` | :arrow_up: |
| [msg.go](https://codecov.io/gh/miekg/dns/pull/1155/diff?src=pr&el=tree#diff-bXNnLmdv) | `77.21% <0.00%> (+0.68%)` | :arrow_up: |
| [zduplicate.go](https://codecov.io/gh/miekg/dns/pull/1155/diff?src=pr&el=tree#diff-emR1cGxpY2F0ZS5nbw==) | `4.83% <0.00%> (+1.05%)` | :arrow_up: |
| [msg\_helpers.go](https://codecov.io/gh/miekg/dns/pull/1155/diff?src=pr&el=tree#diff-bXNnX2hlbHBlcnMuZ28=) | `70.63% <0.00%> (+2.42%)` | :arrow_up: |
| [zmsg.go](https://codecov.io/gh/miekg/dns/pull/1155/diff?src=pr&el=tree#diff-em1zZy5nbw==) | `45.29% <0.00%> (+5.26%)` | :arrow_up: |
| ... and [1 more](https://codecov.io/gh/miekg/dns/pull/1155/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=footer). Last update [efdec21...ddbee29](https://codecov.io/gh/miekg/dns/pull/1155?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

AGWA: I've made the requested changes.  Please take another look.
tmthrgd: `errors.As` breaks the really ancient Travis CI build. I'll open a PR to bump the tested version and then this can be merged after that.
miekg: this needs a rebase to pick up the travis changes 
AGWA: Should be good now. Please take another look.

Patch:
diff --git a/client.go b/client.go
index edd9368b..e7ff786a 100644
--- a/client.go
+++ b/client.go
@@ -185,9 +185,20 @@ func (c *Client) exchange(m *Msg, co *Conn) (r *Msg, rtt time.Duration, err erro
 	}
 
 	co.SetReadDeadline(time.Now().Add(c.getTimeoutForRequest(c.readTimeout())))
-	r, err = co.ReadMsg()
-	if err == nil && r.Id != m.Id {
-		err = ErrId
+	if _, ok := co.Conn.(net.PacketConn); ok {
+		for {
+			r, err = co.ReadMsg()
+			// Ignore replies with mismatched IDs because they might be
+			// responses to earlier queries that timed out.
+			if err != nil || r.Id == m.Id {
+				break
+			}
+		}
+	} else {
+		r, err = co.ReadMsg()
+		if err == nil && r.Id != m.Id {
+			err = ErrId
+		}
 	}
 	rtt = time.Since(t)
 	return r, rtt, err


Test patch:
diff --git a/client_test.go b/client_test.go
index b75d946c..11c8469d 100644
--- a/client_test.go
+++ b/client_test.go
@@ -3,6 +3,7 @@ package dns
 import (
 	"context"
 	"crypto/tls"
+	"errors"
 	"fmt"
 	"net"
 	"strconv"
@@ -162,6 +163,12 @@ func TestClientTLSSyncV4(t *testing.T) {
 	}
 }
 
+func isNetworkTimeout(err error) bool {
+	// TODO: when Go 1.14 support is dropped, do this: https://golang.org/doc/go1.15#net
+	var netError net.Error
+	return errors.As(err, &netError) && netError.Timeout()
+}
+
 func TestClientSyncBadID(t *testing.T) {
 	HandleFunc("miek.nl.", HelloServerBadID)
 	defer HandleRemove("miek.nl.")
@@ -175,12 +182,66 @@ func TestClientSyncBadID(t *testing.T) {
 	m := new(Msg)
 	m.SetQuestion("miek.nl.", TypeSOA)
 
+	c := &Client{
+		Timeout: 50 * time.Millisecond,
+	}
+	if _, _, err := c.Exchange(m, addrstr); err == nil || !isNetworkTimeout(err) {
+		t.Errorf("query did not time out")
+	}
+	// And now with plain Exchange().
+	if _, err = Exchange(m, addrstr); err == nil || !isNetworkTimeout(err) {
+		t.Errorf("query did not time out")
+	}
+}
+
+func TestClientSyncBadThenGoodID(t *testing.T) {
+	HandleFunc("miek.nl.", HelloServerBadThenGoodID)
+	defer HandleRemove("miek.nl.")
+
+	s, addrstr, err := RunLocalUDPServer(":0")
+	if err != nil {
+		t.Fatalf("unable to run test server: %v", err)
+	}
+	defer s.Shutdown()
+
+	m := new(Msg)
+	m.SetQuestion("miek.nl.", TypeSOA)
+
 	c := new(Client)
-	if _, _, err := c.Exchange(m, addrstr); err != ErrId {
-		t.Errorf("did not find a bad Id")
+	r, _, err := c.Exchange(m, addrstr)
+	if err != nil {
+		t.Errorf("failed to exchange: %v", err)
+	}
+	if r.Id != m.Id {
+		t.Errorf("failed to get response with expected Id")
 	}
 	// And now with plain Exchange().
-	if _, err := Exchange(m, addrstr); err != ErrId {
+	r, err = Exchange(m, addrstr)
+	if err != nil {
+		t.Errorf("failed to exchange: %v", err)
+	}
+	if r.Id != m.Id {
+		t.Errorf("failed to get response with expected Id")
+	}
+}
+
+func TestClientSyncTCPBadID(t *testing.T) {
+	HandleFunc("miek.nl.", HelloServerBadID)
+	defer HandleRemove("miek.nl.")
+
+	s, addrstr, err := RunLocalTCPServer(":0")
+	if err != nil {
+		t.Fatalf("unable to run test server: %v", err)
+	}
+	defer s.Shutdown()
+
+	m := new(Msg)
+	m.SetQuestion("miek.nl.", TypeSOA)
+
+	c := &Client{
+		Net: "tcp",
+	}
+	if _, _, err := c.Exchange(m, addrstr); err != ErrId {
 		t.Errorf("did not find a bad Id")
 	}
 }
diff --git a/server_test.go b/server_test.go
index 864b52f5..c76c7a77 100644
--- a/server_test.go
+++ b/server_test.go
@@ -35,6 +35,19 @@ func HelloServerBadID(w ResponseWriter, req *Msg) {
 	w.WriteMsg(m)
 }
 
+func HelloServerBadThenGoodID(w ResponseWriter, req *Msg) {
+	m := new(Msg)
+	m.SetReply(req)
+	m.Id++
+
+	m.Extra = make([]RR, 1)
+	m.Extra[0] = &TXT{Hdr: RR_Header{Name: m.Question[0].Name, Rrtype: TypeTXT, Class: ClassINET, Ttl: 0}, Txt: []string{"Hello world"}}
+	w.WriteMsg(m)
+
+	m.Id--
+	w.WriteMsg(m)
+}
+
 func HelloServerEchoAddrPort(w ResponseWriter, req *Msg) {
 	m := new(Msg)
 	m.SetReply(req)