From 25eb456b57ef7bfd64e2d2338adb1d8f973e5238 Mon Sep 17 00:00:00 2001 From: Ilya Kulakov Date: Wed, 16 Sep 2026 17:51:15 -0700 Subject: [PATCH] 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 * plugin/file: faster exit for less when a == b Avoid two calls and two reslices. Signed-off-by: Ilya Kulakov * plugin/file: consolidate less tests Signed-off-by: Ilya Kulakov * plugin/file: exit less early when there are no more labels Signed-off-by: Ilya Kulakov * plugin/file: match dns.PackDomainName in handling \-escapes Compare unterminated names as root-terminating Signed-off-by: Ilya Kulakov * plugin/file: More tests of less. Signed-off-by: Ilya Kulakov --------- Signed-off-by: Ilya Kulakov --- plugin/file/tree/less.go | 149 +++++++++++++++++++++--------- plugin/file/tree/less_test.go | 166 ++++++++++++++++++++++------------ 2 files changed, 210 insertions(+), 105 deletions(-) diff --git a/plugin/file/tree/less.go b/plugin/file/tree/less.go index 7668ff2ef..bde738b55 100644 --- a/plugin/file/tree/less.go +++ b/plugin/file/tree/less.go @@ -1,58 +1,117 @@ package tree -import ( - "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 +// less returns <0 when a is less than b, 0 when they are equal and >0 when a is larger than b. // -// See https://bert-hubert.blogspot.co.uk/2015/10/how-to-do-fast-canonical-ordering-of.html -// for a blog article on this implementation, although here we still go label by label. +// Follows DNSSEC canonical ordering (RFC 4034, Section 6.1): +// - `\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) - bj := len(b) - for { - ai, oka := dns.PrevLabel(a[:aj], 1) - bi, okb := dns.PrevLabel(b[:bj], 1) - if oka && okb { - return 0 - } + var ( + adot, bdot int + aoff, boff int + alast, blast = stripTrailingBackslash(a), stripTrailingBackslash(b) + ac, bc byte + ) - // 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 - } - - aj, bj = ai, bi + if adot, _ = prevDot(a, alast); alast >= 0 && alast == adot { + alast-- + } + if bdot, _ = prevDot(b, blast); blast >= 0 && blast == bdot { + blast-- } -} -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] + // dot off + // ▼ ▼ + // my.exampledomain.com. + // ▲ ▲ + // first last + + 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' } -func dddToByte(s []byte) byte { return (s[1]-'0')*100 + (s[2]-'0')*10 + (s[3] - '0') } +// stripTrailingBackslash removes hanging backslash that escapes nothing. +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 +} diff --git a/plugin/file/tree/less_test.go b/plugin/file/tree/less_test.go index 0f8738c6e..c75c48530 100644 --- a/plugin/file/tree/less_test.go +++ b/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{"xxx.powerdns.com.", "aaa.powerdns.de", "bbb.powerdns.net."}, + []string{"aaa.powerdns.de.", "bbb.powerdns.net.", "xxx.powerdns.com."}, + []string{"xxx.powerdns.com.", "aaa.powerdns.de.", "bbb.powerdns.net."}, }, { - []string{"aaa.POWERDNS.de", "bbb.PoweRdnS.net.", "xxx.powerdns.com."}, - []string{"xxx.powerdns.com.", "aaa.POWERDNS.de", "bbb.PoweRdnS.net."}, + []string{"aaa.POWERDNS.de.", "bbb.PoweRdnS.net.", "xxx.powerdns.com."}, + []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. - 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)) + slices.SortFunc(test.in, less) 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) { - // The original less function, serving as the benchmark test baseline. - less0 := func(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 +func TestLess_EdgeCases(t *testing.T) { + // For every case four variants are synthesized: + // - a b + // - a. b + // - a b. + // - a. b. + // + // For each variant commutativity is tested. + tests := []struct { + a, b string + 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 - // 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 + if got := less(variant.b, variant.a); cmp.Compare(got, 0) != -test.want { + t.Errorf("Test %d: expected less(%s, %s)=%d, got %d", i, variant.b, variant.a, -test.want, cmp.Compare(got, 0)) } - - 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') }