From c2144c44a33a373174624ede9f4f6ffe8334cf05 Mon Sep 17 00:00:00 2001
From: Nick Hill <mykola.khyl@gmail.com>
Date: Fri, 4 Oct 2024 15:11:46 -0500
Subject: [PATCH] net/dns/resolver: update (*forwarder).forwardWithDestChan to
 always return an error unless it sends a response to responseChan

We currently have two executions paths where (*forwarder).forwardWithDestChan
returns nil, rather than an error, without sending a DNS response to responseChan.

These paths are accompanied by a comment that reads:
// Returning an error will cause an internal retry, there is
// nothing we can do if parsing failed. Just drop the packet.
But it is not (or no longer longer) accurate: returning an error from forwardWithDestChan
does not currently cause a retry.

Moreover, although these paths are currently unreachable due to implementation details,
if (*forwarder).forwardWithDestChan were to return nil without sending a response to
responseChan, it would cause a deadlock at one call site and a panic at another.

Therefore, we update (*forwarder).forwardWithDestChan to return errors in those two paths
and remove comments that were no longer accurate and misleading.

Updates #cleanup
Updates #13571

Signed-off-by: Nick Hill <mykola.khyl@gmail.com>
---
 net/dns/resolver/forwarder.go      | 10 ++--------
 net/dns/resolver/forwarder_test.go | 17 +++++++++++------
 net/dns/resolver/tsdns_test.go     |  4 ++--
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/net/dns/resolver/forwarder.go b/net/dns/resolver/forwarder.go
index 0bf904070..c00dea1ae 100644
--- a/net/dns/resolver/forwarder.go
+++ b/net/dns/resolver/forwarder.go
@@ -920,10 +920,7 @@ func (f *forwarder) forwardWithDestChan(ctx context.Context, query packet, respo
 		metricDNSFwdDropBonjour.Add(1)
 		res, err := nxDomainResponse(query)
 		if err != nil {
-			f.logf("error parsing bonjour query: %v", err)
-			// Returning an error will cause an internal retry, there is
-			// nothing we can do if parsing failed. Just drop the packet.
-			return nil
+			return err
 		}
 		select {
 		case <-ctx.Done():
@@ -955,10 +952,7 @@ func (f *forwarder) forwardWithDestChan(ctx context.Context, query packet, respo
 
 			res, err := servfailResponse(query)
 			if err != nil {
-				f.logf("building servfail response: %v", err)
-				// Returning an error will cause an internal retry, there is
-				// nothing we can do if parsing failed. Just drop the packet.
-				return nil
+				return err
 			}
 			select {
 			case <-ctx.Done():
diff --git a/net/dns/resolver/forwarder_test.go b/net/dns/resolver/forwarder_test.go
index 09d810901..9c0964e93 100644
--- a/net/dns/resolver/forwarder_test.go
+++ b/net/dns/resolver/forwarder_test.go
@@ -7,7 +7,6 @@
 	"bytes"
 	"context"
 	"encoding/binary"
-	"errors"
 	"flag"
 	"fmt"
 	"io"
@@ -657,14 +656,20 @@ func TestForwarderTCPFallbackError(t *testing.T) {
 		}
 	})
 
-	_, err := runTestQuery(t, port, request, nil)
+	resp, err := runTestQuery(t, port, request, nil)
 	if !sawRequest.Load() {
 		t.Error("did not see DNS request")
 	}
-	if err == nil {
-		t.Error("wanted error, got nil")
-	} else if !errors.Is(err, errServerFailure) {
-		t.Errorf("wanted errServerFailure, got: %v", err)
+	if err != nil {
+		t.Fatalf("wanted nil, got %v", err)
+	}
+	var parser dns.Parser
+	respHeader, err := parser.Start(resp)
+	if err != nil {
+		t.Fatalf("parser.Start() failed: %v", err)
+	}
+	if got, want := respHeader.RCode, dns.RCodeServerFailure; got != want {
+		t.Errorf("wanted %v, got %v", want, got)
 	}
 }
 
diff --git a/net/dns/resolver/tsdns_test.go b/net/dns/resolver/tsdns_test.go
index e2c4750b5..d7b9fb360 100644
--- a/net/dns/resolver/tsdns_test.go
+++ b/net/dns/resolver/tsdns_test.go
@@ -1503,8 +1503,8 @@ func TestServfail(t *testing.T) {
 	r.SetConfig(cfg)
 
 	pkt, err := syncRespond(r, dnspacket("test.site.", dns.TypeA, noEdns))
-	if !errors.Is(err, errServerFailure) {
-		t.Errorf("err = %v, want %v", err, errServerFailure)
+	if err != nil {
+		t.Fatalf("err = %v, want nil", err)
 	}
 
 	wantPkt := []byte{