fix truncation bug (#2261)

* fix truncation bug

* Generate records with generic RRs

* Remove SoundCloud from test name

* Comment for binary-search -1 adjustment

Explain why the binary search may have exited with
a reply size that is too large by one record.

* Refactor to remove sub variable

patch suggested by miek removes unnecessary sub
variable for removing a single line from the
reply.Extra length.

request/request.go

@@ -250,18 +250,16 @@ func (r *Request) Scrub(reply *dns.Msg) *dns.Msg {
 	}
 
 	// Account for the OPT record that gets added in SizeAndDo(), subtract that length.
-	sub := 0
+	re := len(reply.Extra)
 	if r.Req.IsEdns0() != nil {
-		sub = optLen
+		size -= optLen
+		// re can never be 0 because we have an OPT RR.
+		re--
 	}
 
-	// subtract to make spaces for re-added EDNS0 OPT RR.
-	re := len(reply.Extra) - sub
-	size -= sub
-
 	l, m := 0, 0
 	origExtra := reply.Extra
-	for l < re {
+	for l <= re {
 		m = (l + re) / 2
 		reply.Extra = origExtra[:m]
 		rl = reply.Len()
@@ -278,13 +276,18 @@ func (r *Request) Scrub(reply *dns.Msg) *dns.Msg {
 		}
 	}
 
-	// We may come out of this loop with one rotation too many, m makes it too large, but m-1 works.
+	// The binary search only breaks on an exact match, which will be
+	// pretty rare. Normally, the loop will exit when l > re, meaning that
+	// in the previous iteration either:
+	// rl < size: no need to do anything.
+	// rl > size: the final size is too large, and if m > 0, the preceeding
+	// iteration the size was too small. Select that preceeding size.
 	if rl > size && m > 0 {
 		reply.Extra = origExtra[:m-1]
 		rl = reply.Len()
 	}
 
-	if rl < size {
+	if rl <= size {
 		r.SizeAndDo(reply)
 		return reply
 	}
@@ -292,7 +295,7 @@ func (r *Request) Scrub(reply *dns.Msg) *dns.Msg {
 	ra := len(reply.Answer)
 	l, m = 0, 0
 	origAnswer := reply.Answer
-	for l < ra {
+	for l <= ra {
 		m = (l + ra) / 2
 		reply.Answer = origAnswer[:m]
 		rl = reply.Len()
@@ -309,7 +312,12 @@ func (r *Request) Scrub(reply *dns.Msg) *dns.Msg {
 		}
 	}
 
-	// We may come out of this loop with one rotation too many, m makes it too large, but m-1 works.
+	// The binary search only breaks on an exact match, which will be
+	// pretty rare. Normally, the loop will exit when l > ra, meaning that
+	// in the previous iteration either:
+	// rl < size: no need to do anything.
+	// rl > size: the final size is too large, and if m > 0, the preceeding
+	// iteration the size was too small. Select that preceeding size.
 	if rl > size && m > 0 {
 		reply.Answer = origAnswer[:m-1]
 		// No need to recalc length, as we don't use it. We set truncated anyway. Doing

request/request_test.go

@@ -159,6 +159,36 @@ func TestRequestScrubExtraRegression(t *testing.T) {
 	}
 }
 
+func TestTruncation(t *testing.T) {
+	for bufsize := 1024; bufsize <= 4096; bufsize += 12 {
+		m := new(dns.Msg)
+		m.SetQuestion("http.service.tcp.srv.k8s.example.org", dns.TypeSRV)
+		m.SetEdns0(uint16(bufsize), true)
+		req := Request{W: &test.ResponseWriter{}, Req: m}
+
+		reply := new(dns.Msg)
+		reply.SetReply(m)
+
+		for i := 0; i < 61; i++ {
+			reply.Answer = append(reply.Answer, test.SRV(fmt.Sprintf("http.service.tcp.srv.k8s.example.org. 5 IN SRV 0 0 80 10-144-230-%d.default.pod.k8s.example.org.", i)))
+		}
+
+		for i := 0; i < 5; i++ {
+			reply.Extra = append(reply.Extra, test.A(fmt.Sprintf("ip-10-10-52-5%d.subdomain.example.org. 5 IN A 10.10.52.5%d", i, i)))
+		}
+
+		for i := 0; i < 5; i++ {
+			reply.Ns = append(reply.Ns, test.NS(fmt.Sprintf("srv.subdomain.example.org. 5 IN NS ip-10-10-33-6%d.subdomain.example.org.", i)))
+		}
+
+		req.Scrub(reply)
+		want, got := req.Size(), reply.Len()
+		if want < got {
+			t.Fatalf("Want scrub to reduce message length below %d bytes, got %d bytes", want, got)
+		}
+	}
+}
+
 func TestRequestScrubAnswerExact(t *testing.T) {
 	m := new(dns.Msg)
 	m.SetQuestion("large.example.com.", dns.TypeSRV)