From 741c513e51c6951ba22b37e74b7273ae6d585047 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Wed, 16 Dec 2020 22:14:36 -0800
Subject: [PATCH] wgengine/tsdns: fix error response marshaling, improve bad
 query logs

Updates #995

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
---
 wgengine/tsdns/tsdns.go      | 28 ++++++++++++++++++----------
 wgengine/tsdns/tsdns_test.go | 10 ++++++++++
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/wgengine/tsdns/tsdns.go b/wgengine/tsdns/tsdns.go
index d260c8041..9ca03f39e 100644
--- a/wgengine/tsdns/tsdns.go
+++ b/wgengine/tsdns/tsdns.go
@@ -381,22 +381,26 @@ func marshalResponse(resp *response) ([]byte, error) {
 
 	builder := dns.NewBuilder(nil, resp.Header)
 
-	err := builder.StartQuestions()
-	if err != nil {
-		return nil, err
-	}
+	isSuccess := resp.Header.RCode == dns.RCodeSuccess
 
-	err = builder.Question(resp.Question)
-	if err != nil {
-		return nil, err
+	if resp.Question.Type != 0 || isSuccess {
+		err := builder.StartQuestions()
+		if err != nil {
+			return nil, err
+		}
+
+		err = builder.Question(resp.Question)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	// Only successful responses contain answers.
-	if resp.Header.RCode != dns.RCodeSuccess {
+	if !isSuccess {
 		return builder.Finish()
 	}
 
-	err = builder.StartAnswers()
+	err := builder.StartAnswers()
 	if err != nil {
 		return nil, err
 	}
@@ -576,7 +580,11 @@ func (r *Resolver) respond(query []byte) ([]byte, error) {
 	err := parseQuery(query, resp)
 	// We will not return this error: it is the sender's fault.
 	if err != nil {
-		r.logf("parsing query: %v", err)
+		if errors.Is(err, dns.ErrSectionDone) {
+			r.logf("parseQuery(%02x): no DNS questions", query)
+		} else {
+			r.logf("parseQuery(%02x): %v", query, err)
+		}
 		resp.Header.RCode = dns.RCodeFormatError
 		return marshalResponse(resp)
 	}
diff --git a/wgengine/tsdns/tsdns_test.go b/wgengine/tsdns/tsdns_test.go
index b072fc710..2df2e35d7 100644
--- a/wgengine/tsdns/tsdns_test.go
+++ b/wgengine/tsdns/tsdns_test.go
@@ -748,3 +748,13 @@ func BenchmarkFull(b *testing.B) {
 		})
 	}
 }
+
+func TestMarshalResponseFormatError(t *testing.T) {
+	resp := new(response)
+	resp.Header.RCode = dns.RCodeFormatError
+	v, err := marshalResponse(resp)
+	if err != nil {
+		t.Errorf("marshal error: %v", err)
+	}
+	t.Logf("response: %q", v)
+}