refactor(dns): eagerly decode CNAME target into r.data, drop target field

Address review feedback on #189: - Remove Resource.target: Resource.Decode expands CNAME RDATA in place into r.data (uncompressed wire format) while the full message is still available and updates header.Length, storing the name bytes exactly once. - Add Resource.CNAMEView returning a length-bounded view of the expanded target; Message.WriteAnswers uses it. - Rename matchesHost to ResourceHeader.pertainsTo. - Make ResolveConfig.MaxCNAMEs explicit: drop the silent default in Client.StartResolve and let callers opt in (x/xnet). - Reduce test suite to regression coverage only. refs #189
2026-08-28 18:39:05 +00:00 · 2026-08-27 09:46:00 +09:00
parent a33e9c5fab
commit 8c49548440
4 changed files with 81 additions and 71 deletions
@@ -43,11 +43,7 @@ func (c *Client) StartResolve(localPort, txid uint16, cfg ResolveConfig) error {
 	if maxIPs == 0 {
 		maxIPs = uint16(nd)
 	}
-	maxCNAMEs := cfg.MaxCNAMEs
-	if maxCNAMEs == 0 {
-		maxCNAMEs = 16
-	}
-	maxAns := maxIPs + maxCNAMEs
+	maxAns := maxIPs + cfg.MaxCNAMEs
 	c.reset(localPort, txid, CQueryPending, cfg.EnableRecursion)
 	c.msg.LimitResourceDecoding(uint16(nd), maxAns, 0, 0)
 	c.msg.AddQuestions(cfg.Questions)
@@ -48,7 +48,6 @@ type Question struct {
 type Resource struct {
 	header ResourceHeader
 	data   []byte
-	target Name
 }

 // A ResourceHeader is the header of a DNS resource record. There are
@@ -300,50 +299,66 @@ func (m *Message) AppendTo(buf []byte, txid uint16, flags HeaderFlags) (_ []byte
 	return buf, nil
 }

+// WriteAnswers follows CNAMEs from host among the answers and writes the
+// resulting addresses into dst.
 func (m *Message) WriteAnswers(dst []netip.Addr, host string) (n uint16, err error) {
 	var alias Name
-	// Each pass follows at most one alias hop
-	// The pass limit also bounds CNAME cycles in malformed responses.
 	for range m.Answers {
-		var next Name
-		n = 0
+		var (
+			next     Name
+			hasAddrs bool
+		)
 		for i := range m.Answers {
-			if int(n) >= len(dst) {
-				break
-			}
 			ans := &m.Answers[i]
 			hdr := ans.Header()
-			matched := alias.Len() == 0 && hdr.Name.EqualString(host) ||
-				alias.Len() != 0 && NamesEqual(hdr.Name, alias)
-			if !matched {
+			if !hdr.pertainsTo(alias, host) {
 				continue
 			}
 			switch hdr.Type {
 			case TypeA, TypeAAAA:
-				var ok bool
-				dst[n], ok = netip.AddrFromSlice(ans.RawData())
-				if !ok {
-					err = lneto.ErrInvalidAddr
-					continue
-				} else {
-					n++
-				}
+				hasAddrs = true
 			case TypeCNAME:
-				if ans.target.Len() != 0 {
-					next = ans.target
+				if cname, ok := ans.CNAMEView(); ok && cname.Len() != 0 {
+					next = cname
 				}
 			}
 		}
-		if next.Len() == 0 || n > 0 {
-			// Either no new alias was found or addresses were resolved for
-			// the current name; a name cannot alias and hold records at once.
+		if hasAddrs || next.Len() == 0 {
 			break
 		}
 		alias = next
 	}
+
+	for i := range m.Answers {
+		if int(n) >= len(dst) {
+			return n, lneto.ErrExhausted
+		}
+		ans := &m.Answers[i]
+		hdr := ans.Header()
+		isAddr := hdr.Type == TypeA || hdr.Type == TypeAAAA
+		if !isAddr || !hdr.pertainsTo(alias, host) {
+			continue
+		}
+		var ok bool
+		dst[n], ok = netip.AddrFromSlice(ans.RawData())
+		if !ok {
+			err = lneto.ErrInvalidAddr
+		} else {
+			n++
+		}
+	}
 	return n, err
 }

+// pertainsTo reports whether the record's owner name is the given name: host
+// or one of the aliases resolved so far.
+func (h *ResourceHeader) pertainsTo(alias Name, host string) bool {
+	if alias.Len() == 0 {
+		return h.Name.EqualString(host)
+	}
+	return NamesEqual(h.Name, alias)
+}
+
 func (m *Message) Len() uint16 {
 	return SizeHeader + m.lenResources()
 }
@@ -423,7 +438,6 @@ func (h *ResourceHeader) String() string {
 func (r *Resource) Reset() {
 	r.header.Reset()
 	r.data = r.data[:0]
-	r.target.Reset()
 }

 func (r *Resource) Header() ResourceHeader { return r.header }
@@ -436,6 +450,13 @@ func (r *Resource) RawData() []byte {
 	return r.data[:length]
 }

+func (r *Resource) CNAMEView() (Name, bool) {
+	if r.header.Type == TypeCNAME {
+		return Name{data: r.RawData()}, true
+	}
+	return Name{}, false
+}
+
 func (q *Question) Reset() {
 	q.Name.Reset()
 	*q = Question{Name: q.Name} // Reuse Name's buffer.
@@ -485,16 +506,32 @@ func (r *Resource) Decode(b []byte, off uint16) (uint16, error) {
 	if r.header.Length > uint16(len(b[off:])) {
 		return off, errResourceLen
 	}
+	end := off + r.header.Length
+	r.data = append(r.data[:0], b[off:end]...)
 	if r.header.Type == TypeCNAME {
-		_, err = r.target.Decode(b, off)
-		if err != nil {
-			r.target.Reset() // Tolerate undecodable target; CNAME chain lookup skips it.
+		raw := b[off:end]
+		data, derr := expandName(r.data[:0], b, off)
+		if derr == nil {
+			r.data = data
+			r.header.Length = uint16(len(r.data))
+		} else {
+			r.data = append(r.data[:0], raw...)
 		}
-	} else {
-		r.target.Reset()
 	}
-	r.data = append(r.data[:0], b[off:off+r.header.Length]...)
-	return off + r.header.Length, nil
+	return end, nil
+}
+
+func expandName(buf []byte, msg []byte, off uint16) ([]byte, error) {
+	appended := 0
+	_, err := visitAllLabels(msg, off, func(label []byte) {
+		buf = append(buf, byte(len(label)))
+		buf = append(buf, label...)
+		appended += 1 + len(label)
+	}, allowCompression)
+	if err != nil {
+		return buf[:len(buf)-appended], err
+	}
+	return append(buf, 0), nil
 }

 func (r *Resource) appendTo(buf []byte) (_ []byte, err error) {
@@ -808,7 +845,6 @@ func (dst *Question) CopyFrom(q Question) {
 func (dst *Resource) CopyFrom(r Resource) {
 	dst.header.CopyFrom(r.header)
 	dst.data = append(dst.data[:0], r.data...)
-	dst.target.CopyFrom(r.target)
 }

 // SetA sets an A (IPv4 address) resource record, reusing internal buffers.
@@ -296,8 +296,8 @@ func TestClient_CNAMEResponse(t *testing.T) {
 	}
 }

-// Table-driven tests for Message.WriteAnswers covering answer reordering,
-// non-address record types and cyclic CNAME aliases.
+// Table-driven tests for Message.WriteAnswers covering answer reordering
+// and cyclic CNAME aliases.
 func TestMessage_WriteAnswers(t *testing.T) {
 	tests := []struct {
 		name     string
@@ -327,22 +327,6 @@ func TestMessage_WriteAnswers(t *testing.T) {
 			},
 			want: []netip.Addr{netip.AddrFrom4([4]byte{182, 22, 23, 124})},
 		},
-		{
-			name: "ignores non-address records",
-			host: "example.com",
-			response: []byte{
-				// Header: txid 0x5678, QR|RD|RA, QD=1 AN=2 NS=0 AR=0.
-				0x56, 0x78, 0x81, 0x80, 0x00, 0x01, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00,
-				// Question: example.com A IN.
-				0x07, 'e', 'x', 'a', 'm', 'p', 'l', 'e', 0x03, 'c', 'o', 'm', 0x00,
-				0x00, 0x01, 0x00, 0x01,
-				// Answer 1: (ptr to question) TXT IN ttl=60 rdlen=6 "hello".
-				0xc0, 0x0c, 0x00, 0x10, 0x00, 0x01, 0x00, 0x00, 0x00, 0x3c, 0x00, 0x06, 0x05, 'h', 'e', 'l', 'l', 'o',
-				// Answer 2: (ptr to question) A IN ttl=300 rdlen=4 192.0.2.1.
-				0xc0, 0x0c, 0x00, 0x01, 0x00, 0x01, 0x00, 0x00, 0x01, 0x2c, 0x00, 0x04, 0xc0, 0x00, 0x02, 0x01,
-			},
-			want: []netip.Addr{netip.AddrFrom4([4]byte{192, 0, 2, 1})},
-		},
 		{
 			name: "CNAME cycle terminates",
 			host: "a.com",
@@ -389,28 +373,21 @@ func TestClient_ReceivesDNSResponse(t *testing.T) {
 	const txid = uint16(12345)
 	const clientPort = uint16(54321)
 	const maxIPs = 4
-	const maxCNAMEs = 2
-	const maxDecoded = maxIPs + maxCNAMEs
-	allIPs := [8][4]byte{
+	allIPs := [5][4]byte{
 		{192, 0, 2, 1},
 		{192, 0, 2, 2},
 		{192, 0, 2, 3},
 		{192, 0, 2, 4},
 		{192, 0, 2, 5},
-		{192, 0, 2, 6},
-		{192, 0, 2, 7},
-		{192, 0, 2, 8},
 	}
 	tests := []struct {
 		name        string
 		responseIPs [][4]byte
-		wantDecoded int // Answers decoded (and copied by ResponseCopyTo).
-		wantAnswers int // Addresses returned by ResponseAnswerLookup.
+		wantAnswers int // Addresses returned by ResponseAnswerLookup and copied by ResponseCopyTo.
 	}{
-		{name: "single_answer", responseIPs: allIPs[:1], wantDecoded: 1, wantAnswers: 1},
-		{name: "multiple_answers", responseIPs: allIPs[:4], wantDecoded: 4, wantAnswers: 4},
-		{name: "answer_limit", responseIPs: allIPs[:5], wantDecoded: 5, wantAnswers: maxIPs},
-		{name: "decode_limit", responseIPs: allIPs[:8], wantDecoded: maxDecoded, wantAnswers: maxIPs},
+		{name: "single_answer", responseIPs: allIPs[:1], wantAnswers: 1},
+		{name: "multiple_answers", responseIPs: allIPs[:4], wantAnswers: 4},
+		{name: "answer_limit", responseIPs: allIPs[:5], wantAnswers: maxIPs},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -444,7 +421,7 @@ func TestClient_ReceivesDNSResponse(t *testing.T) {
 				}},
 				EnableRecursion: true,
 				MaxIPs:          maxIPs,
-				MaxCNAMEs:       maxCNAMEs,
+				MaxCNAMEs:       0, // No CNAME records in this response.
 			})
 			if err != nil {
 				t.Fatal("failed to start DNS resolve:", err)
@@ -487,8 +464,8 @@ func TestClient_ReceivesDNSResponse(t *testing.T) {
 			if !done {
 				t.Fatal("expected done=true")
 			}
-			if len(lookup.Answers) != tt.wantDecoded {
-				t.Fatalf("expected %d copied answers, got %d", tt.wantDecoded, len(lookup.Answers))
+			if len(lookup.Answers) != tt.wantAnswers {
+				t.Fatalf("expected %d copied answers, got %d", tt.wantAnswers, len(lookup.Answers))
 			}
 		})
 	}
@@ -631,6 +631,7 @@ func (s *StackAsync) StartLookupIPType(host string, qtype dns.Type) error {
 		},
 		EnableRecursion: true,
 		MaxIPs:          uint16(len(s.addrbufnip)),
+		MaxCNAMEs:       8,
 	})
 	if err != nil {
 		return err