request.Scrub: test for rl==size case (#1631)

* request.Scrub: test for rl==size case

Make a test case for the new break statement in Scrub and also
account for the OPT record that may get re-added in SizeAndDo() -
otherwise we may break clients that expect this.

* Fix comment

1 files changed, 13 insertions, 8 deletions

diff --git a/request/request.go b/request/request.go
index 9c9512f3a..0629a2123 100644
--- a/request/request.go
+++ b/request/request.go
@@ -167,7 +167,6 @@ func (r *Request) SizeAndDo(m *dns.Msg) bool {
 	if odo {
 		o.SetDo()
 	}
-
 	m.Extra = append(m.Extra, o)
 	return true
 }
@@ -202,8 +201,13 @@ func (r *Request) Scrub(reply *dns.Msg) (*dns.Msg, Result) {
 		return reply, ScrubIgnored
 	}
 
+	// Account for the OPT record that gets added in SizeAndDo(), subtract that length.
+	sub := 0
+	if r.Do() {
+		sub = optLen
+	}
 	origExtra := reply.Extra
-	re := len(reply.Extra)
+	re := len(reply.Extra) - sub
 	l, m := 0, 0
 	for l < re {
 		m = (l + re) / 2
@@ -221,8 +225,8 @@ func (r *Request) Scrub(reply *dns.Msg) (*dns.Msg, Result) {
 			break
 		}
 	}
-	// We may come out of this loop with one rotation too many as we don't break on rl == size.
-	// I.e. m makes it too large, but m-1 works.
+
+	// We may come out of this loop with one rotation too many, m makes it too large, but m-1 works.
 	if rl > size && m > 0 {
 		reply.Extra = origExtra[:m-1]
 		rl = reply.Len()
@@ -252,16 +256,16 @@ func (r *Request) Scrub(reply *dns.Msg) (*dns.Msg, Result) {
 			break
 		}
 	}
-	// We may come out of this loop with one rotation too many as we don't break on rl == size.
-	// I.e. m makes it too large, but m-1 works.
+
+	// We may come out of this loop with one rotation too many, m makes it too large, but m-1 works.
 	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
 		// this extra m-1 step does make it fit in the client's buffer however.
 	}
 
-	// It now fits, but Truncated.
-	r.SizeAndDo(reply)
+	// It now fits, but Truncated. We can't call sizeAndDo() because that adds a new record (OPT)
+	// in the additional section.
 	reply.Truncated = true
 	return reply, ScrubAnswer
 }
@@ -389,4 +393,5 @@ const (
 	// TODO(miek): make this less awkward.
 	doTrue  = 1
 	doFalse = 2
+	optLen  = 12 // OPT record length.
 )