plugin/file: fix less is not up to RFC 1034 and 4034 (#8503)

* plugin/file: fix less to follow RFC 1034 and RFC 4034 matching and ordering requirements

- Ensure comparison is left-justified
- Ensure case folding applies only to A-Z
- Decode \DDD without allocations

Signed-off-by: Ilya Kulakov <kulakov.ilya@gmail.com>

* plugin/file: faster exit for less when a == b

Avoid two calls and two reslices.

Signed-off-by: Ilya Kulakov <kulakov.ilya@gmail.com>

* plugin/file: consolidate less tests

Signed-off-by: Ilya Kulakov <kulakov.ilya@gmail.com>

* plugin/file: exit less early when there are no more labels

Signed-off-by: Ilya Kulakov <kulakov.ilya@gmail.com>

* plugin/file: match dns.PackDomainName in handling \-escapes

Compare unterminated names as root-terminating

Signed-off-by: Ilya Kulakov <kulakov.ilya@gmail.com>

* plugin/file: More tests of less.

Signed-off-by: Ilya Kulakov <kulakov.ilya@gmail.com>

---------

Signed-off-by: Ilya Kulakov <kulakov.ilya@gmail.com>

plugin/file/tree/less.go

@@ -1,58 +1,117 @@
 package tree
 
-import (
+// less returns <0 when a is less than b, 0 when they are equal and >0 when a is larger than b.
-	"bytes"
-	"strings"
-
-	"github.com/miekg/dns"
-)
-
-// less returns <0 when a is less than b, 0 when they are equal and
-// >0 when a is larger than b.
-// The function orders names in DNSSEC canonical order: RFC 4034s section-6.1
 //
-// See https://bert-hubert.blogspot.co.uk/2015/10/how-to-do-fast-canonical-ordering-of.html
+// Follows DNSSEC canonical ordering (RFC 4034, Section 6.1):
-// for a blog article on this implementation, although here we still go label by label.
+//   - `\DDD` byte is decoded before comparison
+//   - Uppercase A-Z letters are treated as if they were lowercase
+//   - Absence of octet sorts before zero value octet
 //
-// The values of a and b are *not* lowercased before the comparison!
+// Quirks:
+//   - Trailing `\` that escapes nothing is ignored
+//   - Leading `\` in `\D` and `\DD` is ignored
+//   - Non-FQDN names are assumed to be root-terminated
 func less(a, b string) int {
-	aj := len(a)
+	var (
-	bj := len(b)
+		adot, bdot   int
-	for {
+		aoff, boff   int
-		ai, oka := dns.PrevLabel(a[:aj], 1)
+		alast, blast = stripTrailingBackslash(a), stripTrailingBackslash(b)
-		bi, okb := dns.PrevLabel(b[:bj], 1)
+		ac, bc       byte
-		if oka && okb {
+	)
-			return 0
-		}
 
-		// sadly this []byte will allocate... TODO(miek): check if this is needed
+	if adot, _ = prevDot(a, alast); alast >= 0 && alast == adot {
-		// for a name, otherwise compare the strings.
+		alast--
-		ab := []byte(strings.ToLower(a[ai:aj]))
+	}
-		bb := []byte(strings.ToLower(b[bi:bj]))
+	if bdot, _ = prevDot(b, blast); blast >= 0 && blast == bdot {
-		doDDD(ab)
+		blast--
-		doDDD(bb)
-
-		res := bytes.Compare(ab, bb)
-		if res != 0 {
-			return res
-		}
-
-		aj, bj = ai, bi
 	}
-}
 
-func doDDD(b []byte) {
+	//   dot       off
-	lb := len(b)
+	//    ▼         ▼
-	for i := 0; i < lb; i++ {
+	//  my.exampledomain.com.
-		if i+3 < lb && b[i] == '\\' && isDigit(b[i+1]) && isDigit(b[i+2]) && isDigit(b[i+3]) {
+	//     ▲           ▲
-			b[i] = dddToByte(b[i:])
+	//   first        last
-			for j := i + 1; j < lb-3; j++ {
+
-				b[j] = b[j+3]
+	for alast >= 0 && blast >= 0 {
+		adot, aoff = prevDot(a, alast)
+		bdot, boff = prevDot(b, blast)
+
+		for aoff <= alast && boff <= blast {
+			ac, aoff = a[aoff], aoff+1
+			if ac == '\\' {
+				ac, aoff = nextEscapedByte(a, aoff, alast)
+			}
+			ac = foldCase(ac)
+
+			bc, boff = b[boff], boff+1
+			if bc == '\\' {
+				bc, boff = nextEscapedByte(b, boff, blast)
+			}
+			bc = foldCase(bc)
+
+			if ac != bc {
+				return int(ac) - int(bc)
 			}
-			lb -= 3
 		}
+
+		// Shorter label means less.
+		if d := (alast - aoff) - (blast - boff); d != 0 {
+			return d
+		}
+
+		alast = adot - 1
+		blast = bdot - 1
 	}
+
+	// Fewer labels means less.
+	return alast - blast
 }
 
-func isDigit(b byte) bool     { return b >= '0' && b <= '9' }
+// stripTrailingBackslash removes hanging backslash that escapes nothing.
-func dddToByte(s []byte) byte { return (s[1]-'0')*100 + (s[2]-'0')*10 + (s[3] - '0') }
+func stripTrailingBackslash(s string) (last int) {
+	last = len(s) - 1
+	for last >= 0 && s[last] == '\\' {
+		last--
+	}
+	if (len(s)-last)%2 == 0 { // `...\` vs `...\\`
+		return len(s) - 2
+	}
+	return len(s) - 1
+}
+
+// prevDot finds label-separator dot in [0, last].
+func prevDot(s string, last int) (dot, first int) {
+	for last >= 0 {
+		if s[last] != '.' {
+			last--
+			continue
+		}
+		off1 := last - 1
+		for off1 >= 0 && s[off1] == '\\' {
+			off1--
+		}
+		if (last-off1)%2 != 0 { // `a\.example` vs `a\\.example`
+			break
+		}
+		last = off1
+	}
+	return last, last + 1
+}
+
+// nextByte implements \DDD-aware and escape-aware advancement.
+func nextEscapedByte(s string, off, last int) (byte, int) {
+	if off+2 <= last {
+		d0, d1, d2 := s[off]-'0', s[off+1]-'0', s[off+2]-'0'
+		if d0 < 10 && d1 < 10 && d2 < 10 {
+			return d0*100 + d1*10 + d2, off + 3
+		}
+	}
+	return s[off], off + 1
+}
+
+func foldCase(c byte) byte {
+	if c-'A' < 26 {
+		c |= 0x20
+	}
+	return c
+}

plugin/file/tree/less_test.go

@@ -2,7 +2,8 @@ package tree
 
 import (
 	"bytes"
-	"sort"
+	"cmp"
+	"slices"
 	"strings"
 	"sync"
 	"testing"
@@ -10,24 +11,18 @@ import (
 	"github.com/miekg/dns"
 )
 
-type set []string
-
-func (p set) Len() int           { return len(p) }
-func (p set) Swap(i, j int)      { p[i], p[j] = p[j], p[i] }
-func (p set) Less(i, j int) bool { d := less(p[i], p[j]); return d <= 0 }
-
 func TestLess(t *testing.T) {
 	tests := []struct {
 		in  []string
 		out []string
 	}{
 		{
-			[]string{"aaa.powerdns.de", "bbb.powerdns.net.", "xxx.powerdns.com."},
+			[]string{"aaa.powerdns.de.", "bbb.powerdns.net.", "xxx.powerdns.com."},
-			[]string{"xxx.powerdns.com.", "aaa.powerdns.de", "bbb.powerdns.net."},
+			[]string{"xxx.powerdns.com.", "aaa.powerdns.de.", "bbb.powerdns.net."},
 		},
 		{
-			[]string{"aaa.POWERDNS.de", "bbb.PoweRdnS.net.", "xxx.powerdns.com."},
+			[]string{"aaa.POWERDNS.de.", "bbb.PoweRdnS.net.", "xxx.powerdns.com."},
-			[]string{"xxx.powerdns.com.", "aaa.POWERDNS.de", "bbb.PoweRdnS.net."},
+			[]string{"xxx.powerdns.com.", "aaa.POWERDNS.de.", "bbb.PoweRdnS.net."},
 		},
 		{
 			[]string{"aaa.aaaa.aa.", "aa.aaa.a.", "bbb.bbbb.bb."},
@@ -58,15 +53,7 @@ func TestLess(t *testing.T) {
 
 Tests:
 	for j, test := range tests {
-		// Need to lowercase these example as the Less function does lowercase for us anymore.
+		slices.SortFunc(test.in, less)
-		for i, b := range test.in {
-			test.in[i] = strings.ToLower(b)
-		}
-		for i, b := range test.out {
-			test.out[i] = strings.ToLower(b)
-		}
-
-		sort.Sort(set(test.in))
 		for i := range len(test.in) {
 			if test.in[i] != test.out[i] {
 				t.Errorf("Test %d: expected %s, got %s", j, test.out[i], test.in[i])
@@ -83,21 +70,6 @@ Tests:
 	}
 }
 
-func TestLess_EmptyVsName(t *testing.T) {
-	if d := less("", "a."); d >= 0 {
-		t.Fatalf("expected < 0, got %d", d)
-	}
-	if d := less("a.", ""); d <= 0 {
-		t.Fatalf("expected > 0, got %d", d)
-	}
-}
-
-func TestLess_EmptyVsEmpty(t *testing.T) {
-	if d := less("", ""); d != 0 {
-		t.Fatalf("expected 0, got %d", d)
-	}
-}
-
 // Test that concurrent calls to Less (which calls Elem.Name) do not race or panic.
 // See issue #7561 for reference.
 func TestLess_ConcurrentNameAccess(t *testing.T) {
@@ -121,37 +93,67 @@ func TestLess_ConcurrentNameAccess(t *testing.T) {
 	wg.Wait()
 }
 
-func BenchmarkLess(b *testing.B) {
+func TestLess_EdgeCases(t *testing.T) {
-	// The original less function, serving as the benchmark test baseline.
+	// For every case four variants are synthesized:
-	less0 := func(a, b string) int {
+	//  - a  b
-		i := 1
+	//  - a. b
-		aj := len(a)
+	//  - a  b.
-		bj := len(b)
+	//  - a. b.
-		for {
+	//
-			ai, oka := dns.PrevLabel(a, i)
+	// For each variant commutativity is tested.
-			bi, okb := dns.PrevLabel(b, i)
+	tests := []struct {
-			if oka && okb {
+		a, b     string
-				return 0
+		variants bool
+		want     int
+	}{
+		{``, ``, true, 0},
+		{``, `\000`, true, -1},
+		{``, `\.`, true, -1},
+		{`\.`, `\.`, true, 0},
+		{``, `example`, true, -1},
+		{`example`, `example`, true, 0},
+		{`a\.example`, `a.example`, true, -1},
+		{`a.example`, `a-b.example`, true, -1},
+		{`a.example`, `a*.example`, true, -1},
+		{`a.example`, `a\000.example`, true, -1},
+		{`a.eXaMpLe`, `a.example`, true, 0},
+		{`\000\0320 \"\046@*`, `\000\032\048\032\034\046\064\042`, true, 0},
+		{`<=>?@ABCDE`, `\060\061\062\063\064\065\066\067\068\069`, true, 0},
+		{`<=>?@ABCDE`, `\060\061\062\063\064\097\098\099\100\101`, true, 0},
+		{`café.example`, `CAFÉ.example`, true, 1}, // é (\195\169) > É (\195\137)
+		{`\\065.example`, `\\097.example`, true, -1},
+		{``, `\`, false, 0},
+		{`a\.b.example`, `a\046b.example`, true, 0},
+		{`0.example`, `\0.example`, true, 0},
+		{`01.example`, `\01.example`, true, 0},
+		{`a\.b.example`, `a\046b.example`, true, 0},
+		{`\\.example`, `\092.example`, true, 0},
+	}
+	for i, test := range tests {
+		variants := []struct{ a, b string }{
+			{test.a, test.b},
+			{test.a + `.`, test.b},
+			{test.a, test.b + `.`},
+			{test.a + `.`, test.b + `.`},
+		}
+		if !test.variants {
+			variants = variants[:1]
+		}
+
+		for _, variant := range variants {
+			if got := less(variant.a, variant.b); cmp.Compare(got, 0) != test.want {
+				t.Errorf("Test %d: expected less(%s, %s)=%d, got %d", i, variant.a, variant.b, test.want, cmp.Compare(got, 0))
 			}
 
-			// sadly this []byte will allocate... TODO(miek): check if this is needed
+			if got := less(variant.b, variant.a); cmp.Compare(got, 0) != -test.want {
-			// for a name, otherwise compare the strings.
+				t.Errorf("Test %d: expected less(%s, %s)=%d, got %d", i, variant.b, variant.a, -test.want, cmp.Compare(got, 0))
-			ab := []byte(strings.ToLower(a[ai:aj]))
-			bb := []byte(strings.ToLower(b[bi:bj]))
-			doDDD(ab)
-			doDDD(bb)
-
-			res := bytes.Compare(ab, bb)
-			if res != 0 {
-				return res
 			}
-
-			i++
-			aj, bj = ai, bi
 		}
 	}
+}
 
-	tests := []set{
+func BenchmarkLess(b *testing.B) {
+	tests := [][]string{
 		{"aaa.powerdns.de", "bbb.powerdns.net.", "xxx.powerdns.com."},
 		{"aaa.POWERDNS.de", "bbb.PoweRdnS.net.", "xxx.powerdns.com."},
 		{"aaa.aaaa.aa.", "aa.aaa.a.", "bbb.bbbb.bb."},
@@ -187,3 +189,47 @@ func BenchmarkLess(b *testing.B) {
 		}
 	})
 }
+
+// The original less function, serving as the benchmark test baseline.
+func less0(a, b string) int {
+	i := 1
+	aj := len(a)
+	bj := len(b)
+	for {
+		ai, oka := dns.PrevLabel(a, i)
+		bi, okb := dns.PrevLabel(b, i)
+		if oka && okb {
+			return 0
+		}
+
+		// sadly this []byte will allocate... TODO(miek): check if this is needed
+		// for a name, otherwise compare the strings.
+		ab := []byte(strings.ToLower(a[ai:aj]))
+		bb := []byte(strings.ToLower(b[bi:bj]))
+		doDDD(ab)
+		doDDD(bb)
+
+		res := bytes.Compare(ab, bb)
+		if res != 0 {
+			return res
+		}
+
+		i++
+		aj, bj = ai, bi
+	}
+}
+
+func doDDD(b []byte) {
+	lb := len(b)
+	for i := 0; i < lb; i++ {
+		if i+3 < lb && b[i] == '\\' && isDigit(b[i+1]) && isDigit(b[i+2]) && isDigit(b[i+3]) {
+			b[i] = dddToByte(b[i:])
+			for j := i + 1; j < lb-3; j++ {
+				b[j] = b[j+3]
+			}
+			lb -= 3
+		}
+	}
+}
+func isDigit(b byte) bool     { return b >= '0' && b <= '9' }
+func dddToByte(s []byte) byte { return (s[1]-'0')*100 + (s[2]-'0')*10 + (s[3] - '0') }