From c1ad186588cbda5df8159fe15fb21a37dde87012 Mon Sep 17 00:00:00 2001 From: Tom Thorogood Date: Thu, 29 Nov 2018 09:48:02 +1030 Subject: [PATCH 1/4] Use compressionMapsEqual in TestPackDomainNameCompressionMap --- msg_test.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/msg_test.go b/msg_test.go index 0d8e5196..c8ff41d3 100644 --- a/msg_test.go +++ b/msg_test.go @@ -214,8 +214,15 @@ func TestUnpackDomainName(t *testing.T) { } func TestPackDomainNameCompressionMap(t *testing.T) { - msg := make([]byte, 256) + expected := map[string]struct{}{ + `www.this.is.\131an.example.org.`: struct{}{}, + `is.\131an.example.org.`: struct{}{}, + "\x83an.example.org.": struct{}{}, + `example.org.`: struct{}{}, + `org.`: struct{}{}, + } + msg := make([]byte, 256) for _, compress := range []bool{true, false} { compression := make(map[string]int) @@ -224,16 +231,8 @@ func TestPackDomainNameCompressionMap(t *testing.T) { t.Fatalf("PackDomainName failed: %v", err) } - for _, dname := range []string{ - `www.this.is.\131an.example.org.`, - `is.\131an.example.org.`, - "\x83an.example.org.", - `example.org.`, - `org.`, - } { - if _, ok := compression[dname]; !ok { - t.Errorf("expected to find %q in compression map", dname) - } + if !compressionMapsEqual(expected, compression) { + t.Errorf("expected compression maps to be equal; expected %v, got %v", expected, compression) } } } From 07ae768ab11faef931d3e1523023ee1a5287292f Mon Sep 17 00:00:00 2001 From: Tom Thorogood Date: Thu, 29 Nov 2018 09:49:18 +1030 Subject: [PATCH 2/4] Put escaped names into compression map in PackDomainName --- msg.go | 29 ++++++++++------------------- msg_test.go | 10 +++++----- 2 files changed, 15 insertions(+), 24 deletions(-) diff --git a/msg.go b/msg.go index b1cc4e43..94159d03 100644 --- a/msg.go +++ b/msg.go @@ -223,9 +223,11 @@ func packDomainName(s string, msg []byte, off int, compression map[string]int, c // Emit sequence of counted strings, chopping at dots. var ( - begin int - bs []byte - wasDot bool + begin int + compBegin int + compOff int + bs []byte + wasDot bool ) loop: for i := 0; i < ls; i++ { @@ -251,9 +253,11 @@ loop: bs[i] = dddToByte(bs[i+1:]) copy(bs[i+1:ls-3], bs[i+4:]) ls -= 3 + compOff += 3 } else { copy(bs[i:ls-1], bs[i+1:]) ls-- + compOff++ } wasDot = false @@ -279,17 +283,7 @@ loop: // We should only compress when compress is true, but we should also still pick // up names that can be used for *future* compression(s). if compression != nil && !isRootLabel(s, bs, begin, ls) { - var ( - p int - ok bool - ) - if bs == nil { - p, ok = compression[s[begin:]] - } else { - p, ok = compression[string(bs[begin:ls])] - } - - if ok { + if p, ok := compression[s[compBegin:]]; ok { // The first hit is the longest matching dname // keep the pointer offset we get back and store // the offset of the current name, because that's @@ -302,11 +296,7 @@ loop: } } else if off < maxCompressionOffset { // Only offsets smaller than maxCompressionOffset can be used. - if bs == nil { - compression[s[begin:]] = off - } else { - compression[string(bs[begin:ls])] = off - } + compression[s[compBegin:]] = off } } @@ -324,6 +314,7 @@ loop: labels++ begin = i + 1 + compBegin = begin + compOff default: wasDot = false } diff --git a/msg_test.go b/msg_test.go index c8ff41d3..70dca3c3 100644 --- a/msg_test.go +++ b/msg_test.go @@ -215,11 +215,11 @@ func TestUnpackDomainName(t *testing.T) { func TestPackDomainNameCompressionMap(t *testing.T) { expected := map[string]struct{}{ - `www.this.is.\131an.example.org.`: struct{}{}, - `is.\131an.example.org.`: struct{}{}, - "\x83an.example.org.": struct{}{}, - `example.org.`: struct{}{}, - `org.`: struct{}{}, + `www\.this.is.\131an.example.org.`: struct{}{}, + `is.\131an.example.org.`: struct{}{}, + `\131an.example.org.`: struct{}{}, + `example.org.`: struct{}{}, + `org.`: struct{}{}, } msg := make([]byte, 256) From d27f0d3482b2cf0f8c610e47bada76eec058fee4 Mon Sep 17 00:00:00 2001 From: Tom Thorogood Date: Thu, 29 Nov 2018 09:53:00 +1030 Subject: [PATCH 3/4] Add a test case to cover escaping in the compression map --- length_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/length_test.go b/length_test.go index 17bca197..825af2a3 100644 --- a/length_test.go +++ b/length_test.go @@ -382,3 +382,26 @@ func TestMsgCompressLengthLargeRecordsAllValues(t *testing.T) { } } } + +func TestMsgCompressLengthEscapingMatch(t *testing.T) { + // Although slightly non-optimal, "example.org." and "ex\\097mple.org." + // are not considered equal in the compression map, even though \097 is + // a valid escaping of a. This test ensures that the Len code and the + // Pack don't disagree on this. + + msg := new(Msg) + msg.Compress = true + msg.SetQuestion("www.example.org.", TypeA) + msg.Answer = append(msg.Answer, &NS{Hdr: RR_Header{Name: "ex\\097mple.org.", Rrtype: TypeNS, Class: ClassINET}, Ns: "ns.example.org."}) + + predicted := msg.Len() + buf, err := msg.Pack() + if err != nil { + t.Error(err) + } + // Len doesn't account for escaping when calculating the length *yet* so + // we're off by three here. This will be fixed in a follow up change. + if predicted != len(buf)+3 { + t.Fatalf("predicted compressed length is wrong: predicted %d, actual %d", predicted, len(buf)) + } +} From 56118562d7547f3265c19f501805ab71e7ae5974 Mon Sep 17 00:00:00 2001 From: Tom Thorogood Date: Thu, 29 Nov 2018 09:58:18 +1030 Subject: [PATCH 4/4] Fix typo in TestMsgCompressLengthEscapingMatch comment --- length_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/length_test.go b/length_test.go index 825af2a3..6dacb40a 100644 --- a/length_test.go +++ b/length_test.go @@ -387,7 +387,7 @@ func TestMsgCompressLengthEscapingMatch(t *testing.T) { // Although slightly non-optimal, "example.org." and "ex\\097mple.org." // are not considered equal in the compression map, even though \097 is // a valid escaping of a. This test ensures that the Len code and the - // Pack don't disagree on this. + // Pack code don't disagree on this. msg := new(Msg) msg.Compress = true