Skip to content

Commit

Permalink
Properly calculate compressed message lengths (miekg#833)
Browse files Browse the repository at this point in the history
* Remove fullSize return from compressionLenSearch

This wasn't used anywhere but TestCompressionLenSearch, and was very
wrong.

* Add generated compressedLen functions and use them

This replaces the confusing and complicated compressionLenSlice
function.

* Use compressedLenWithCompressionMap even for uncompressed

This leaves the len() functions unused and they'll soon be removed.

This also fixes the off-by-one error of compressedLen when a (Q)NAME
is ".".

* Use Len helper instead of RR.len private method

* Merge len and compressedLen functions

* Merge compressedLen helper into Msg.Len

* Remove compress bool from compressedLenWithCompressionMap

* Merge map insertion into compressionLenSearch

This eliminates the need to loop over the domain name twice when we're
compressing the name.

* Use compressedNameLen for NSEC.NextDomain

This was a mistake.

* Remove compress from RR.len

* Add test case for multiple questions length

* Add test case for MINFO and SOA compression

These are the only RRs with multiple compressible names within the same
RR, and they were previously broken.

* Rename compressedNameLen to domainNameLen

It also handles the length of uncompressed domain names.

* Use off directly instead of len(s[:off])

* Move initial maxCompressionOffset check out of compressionLenMapInsert

This should allow us to avoid the call overhead of
compressionLenMapInsert in certain limited cases and may result in a
slight performance increase.

compressionLenMapInsert still has a maxCompressionOffset check inside
the for loop.

* Rename compressedLenWithCompressionMap to msgLenWithCompressionMap

This better reflects that it also calculates the uncompressed length.

* Merge TestMsgCompressMINFO with TestMsgCompressSOA

They're both testing the same thing.

* Remove compressionLenMapInsert

compressionLenSearch does everything compressionLenMapInsert did anyway.

* Only call compressionLenSearch in one place in domainNameLen

* Split if statement in domainNameLen

The last two commits worsened the performance of domainNameLen
noticably, this change restores it's original performance.

name                            old time/op    new time/op    delta
MsgLength-12                       550ns ±13%     510ns ±21%    ~     (p=0.050 n=10+10)
MsgLengthNoCompression-12         26.9ns ± 2%    27.0ns ± 1%    ~     (p=0.198 n=9+10)
MsgLengthPack-12                  2.30µs ±12%    2.26µs ±16%    ~     (p=0.739 n=10+10)
MsgLengthMassive-12               32.9µs ± 7%    32.0µs ±10%    ~     (p=0.243 n=9+10)
MsgLengthOnlyQuestion-12          9.60ns ± 1%    9.20ns ± 1%  -4.16%  (p=0.000 n=9+9)

* Remove stray newline from TestMsgCompressionMultipleQuestions

* Remove stray newline in length_test.go

This was introduced when resolving merge conflicts.
  • Loading branch information
tmthrgd authored Nov 29, 2018
1 parent 2c03911 commit 778aa4f
Show file tree
Hide file tree
Showing 16 changed files with 321 additions and 679 deletions.
198 changes: 0 additions & 198 deletions compress_generate.go

This file was deleted.

15 changes: 10 additions & 5 deletions dns.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,13 @@ type RR interface {

// copy returns a copy of the RR
copy() RR
// len returns the length (in octets) of the uncompressed RR in wire format.
len() int

// len returns the length (in octets) of the compressed or uncompressed RR in wire format.
//
// If compression is nil, the uncompressed size will be returned, otherwise the compressed
// size will be returned and domain names will be added to the map for future compression.
len(off int, compression map[string]struct{}) int

// pack packs an RR into wire format.
pack([]byte, int, map[string]int, bool) (int, error)
}
Expand Down Expand Up @@ -70,15 +75,15 @@ func (h *RR_Header) String() string {
return s
}

func (h *RR_Header) len() int {
l := len(h.Name) + 1
func (h *RR_Header) len(off int, compression map[string]struct{}) int {
l := domainNameLen(h.Name, off, compression, true)
l += 10 // rrtype(2) + class(2) + ttl(4) + rdlength(2)
return l
}

// ToRFC3597 converts a known RR to the unknown RR representation from RFC 3597.
func (rr *RFC3597) ToRFC3597(r RR) error {
buf := make([]byte, r.len()*2)
buf := make([]byte, Len(r)*2)
off, err := PackRR(r, buf, 0, nil, false)
if err != nil {
return err
Expand Down
12 changes: 6 additions & 6 deletions dns_bench_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ func BenchmarkCopy(b *testing.B) {
func BenchmarkPackA(b *testing.B) {
a := &A{Hdr: RR_Header{Name: ".", Rrtype: TypeA, Class: ClassANY}, A: net.IPv4(127, 0, 0, 1)}

buf := make([]byte, a.len())
buf := make([]byte, Len(a))
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
Expand All @@ -189,7 +189,7 @@ func BenchmarkPackA(b *testing.B) {
func BenchmarkUnpackA(b *testing.B) {
a := &A{Hdr: RR_Header{Name: ".", Rrtype: TypeA, Class: ClassANY}, A: net.IPv4(127, 0, 0, 1)}

buf := make([]byte, a.len())
buf := make([]byte, Len(a))
PackRR(a, buf, 0, nil, false)
a = nil
b.ReportAllocs()
Expand All @@ -202,7 +202,7 @@ func BenchmarkUnpackA(b *testing.B) {
func BenchmarkPackMX(b *testing.B) {
m := &MX{Hdr: RR_Header{Name: ".", Rrtype: TypeA, Class: ClassANY}, Mx: "mx.miek.nl."}

buf := make([]byte, m.len())
buf := make([]byte, Len(m))
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
Expand All @@ -213,7 +213,7 @@ func BenchmarkPackMX(b *testing.B) {
func BenchmarkUnpackMX(b *testing.B) {
m := &MX{Hdr: RR_Header{Name: ".", Rrtype: TypeA, Class: ClassANY}, Mx: "mx.miek.nl."}

buf := make([]byte, m.len())
buf := make([]byte, Len(m))
PackRR(m, buf, 0, nil, false)
m = nil
b.ReportAllocs()
Expand All @@ -226,7 +226,7 @@ func BenchmarkUnpackMX(b *testing.B) {
func BenchmarkPackAAAAA(b *testing.B) {
aaaa := testRR(". IN A ::1")

buf := make([]byte, aaaa.len())
buf := make([]byte, Len(aaaa))
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
Expand All @@ -237,7 +237,7 @@ func BenchmarkPackAAAAA(b *testing.B) {
func BenchmarkUnpackAAAA(b *testing.B) {
aaaa := testRR(". IN A ::1")

buf := make([]byte, aaaa.len())
buf := make([]byte, Len(aaaa))
PackRR(aaaa, buf, 0, nil, false)
aaaa = nil
b.ReportAllocs()
Expand Down
8 changes: 4 additions & 4 deletions dns_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,10 @@ func TestPackNAPTR(t *testing.T) {
`apple.com. IN NAPTR 50 50 "se" "SIPS+D2T" "" _sips._tcp.apple.com.`,
} {
rr := testRR(n)
msg := make([]byte, rr.len())
msg := make([]byte, Len(rr))
if off, err := PackRR(rr, msg, 0, nil, false); err != nil {
t.Errorf("packing failed: %v", err)
t.Errorf("length %d, need more than %d", rr.len(), off)
t.Errorf("length %d, need more than %d", Len(rr), off)
}
}
}
Expand Down Expand Up @@ -279,8 +279,8 @@ func TestTKEY(t *testing.T) {
t.Fatal("Unable to decode TKEY")
}
// Make sure we get back the same length
if rr.len() != len(tkeyBytes) {
t.Fatalf("Lengths don't match %d != %d", rr.len(), len(tkeyBytes))
if Len(rr) != len(tkeyBytes) {
t.Fatalf("Lengths don't match %d != %d", Len(rr), len(tkeyBytes))
}
// make space for it with some fudge room
msg := make([]byte, tkeyLen+1000)
Expand Down
2 changes: 1 addition & 1 deletion dnssec.go
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,7 @@ func rawSignatureData(rrset []RR, s *RRSIG) (buf []byte, err error) {
x.Target = strings.ToLower(x.Target)
}
// 6.2. Canonical RR Form. (5) - origTTL
wire := make([]byte, r1.len()+1) // +1 to be safe(r)
wire := make([]byte, Len(r1)+1) // +1 to be safe(r)
off, err1 := PackRR(r1, wire, 0, nil, false)
if err1 != nil {
return nil, err1
Expand Down
4 changes: 2 additions & 2 deletions edns.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ func (rr *OPT) String() string {
return s
}

func (rr *OPT) len() int {
l := rr.Hdr.len()
func (rr *OPT) len(off int, compression map[string]struct{}) int {
l := rr.Hdr.len(off, compression)
for i := 0; i < len(rr.Option); i++ {
l += 4 // Account for 2-byte option code and 2-byte option length.
lo, _ := rr.Option[i].pack()
Expand Down
Loading

0 comments on commit 778aa4f

Please sign in to comment.