From 4139494783fe41b0251c50f82ed761ab5cda349d Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Thu, 22 Jun 2017 12:38:09 +0000 Subject: [PATCH 1/4] Avoid race conditions in DNSSnooper's cached domains --- probe/endpoint/dns_snooper_linux_amd64.go | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/probe/endpoint/dns_snooper_linux_amd64.go b/probe/endpoint/dns_snooper_linux_amd64.go index 6ebd99b5e..f2e131c4d 100644 --- a/probe/endpoint/dns_snooper_linux_amd64.go +++ b/probe/endpoint/dns_snooper_linux_amd64.go @@ -5,6 +5,7 @@ import ( "encoding/binary" "fmt" "math" + "sync" "time" log "github.com/Sirupsen/logrus" @@ -29,6 +30,14 @@ type DNSSnooper struct { decodingErrorCounts map[string]uint64 // for limiting } +type domainSet struct { + // Not worth using a RMutex since we don't expect a lot of contention + // and they are considerably larger (8 bytes vs 24 bytes), which would + // bloat the cache + sync.Mutex + domains map[string]struct{} +} + // NewDNSSnooper creates a new snooper of DNS queries func NewDNSSnooper() (*DNSSnooper, error) { pcapHandle, err := newPcapHandle() @@ -102,9 +111,12 @@ func (s *DNSSnooper) CachedNamesForIP(ip string) []string { return result } - for domain := range domains.(map[string]struct{}) { + domainSet := domains.(domainSet) + domainSet.Lock() + for domain := range domainSet.domains { result = append(result, domain) } + domainSet.Unlock() return result } @@ -269,10 +281,13 @@ func (s *DNSSnooper) processDNSMessage(dns *layers.DNS) { log.Debugf("DNSSnooper: caught DNS lookup: %s -> %v", newDomain, ips) for ip := range ips { if existingDomains, err := s.reverseDNSCache.Get(ip); err != nil { - s.reverseDNSCache.Set(ip, map[string]struct{}{newDomain: {}}) + s.reverseDNSCache.Set(ip, domainSet{domains: map[string]struct{}{newDomain: {}}}) } else { // TODO: Be smarter about the expiration of entries with pre-existing associated domains - existingDomains.(map[string]struct{})[newDomain] = struct{}{} + domainSet := existingDomains.(domainSet) + domainSet.Lock() + domainSet.domains[newDomain] = struct{}{} + domainSet.Unlock() } } } From 22a31fc5f10cc90e3afc34beb3d41f815c1c019b Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Thu, 22 Jun 2017 16:33:36 +0000 Subject: [PATCH 2/4] Review feedback --- probe/endpoint/dns_snooper_linux_amd64.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/probe/endpoint/dns_snooper_linux_amd64.go b/probe/endpoint/dns_snooper_linux_amd64.go index f2e131c4d..0aaa1113d 100644 --- a/probe/endpoint/dns_snooper_linux_amd64.go +++ b/probe/endpoint/dns_snooper_linux_amd64.go @@ -34,7 +34,7 @@ type domainSet struct { // Not worth using a RMutex since we don't expect a lot of contention // and they are considerably larger (8 bytes vs 24 bytes), which would // bloat the cache - sync.Mutex + mutex *sync.Mutex domains map[string]struct{} } @@ -112,11 +112,11 @@ func (s *DNSSnooper) CachedNamesForIP(ip string) []string { } domainSet := domains.(domainSet) - domainSet.Lock() + domainSet.mutex.Lock() for domain := range domainSet.domains { result = append(result, domain) } - domainSet.Unlock() + domainSet.mutex.Unlock() return result } @@ -281,13 +281,17 @@ func (s *DNSSnooper) processDNSMessage(dns *layers.DNS) { log.Debugf("DNSSnooper: caught DNS lookup: %s -> %v", newDomain, ips) for ip := range ips { if existingDomains, err := s.reverseDNSCache.Get(ip); err != nil { - s.reverseDNSCache.Set(ip, domainSet{domains: map[string]struct{}{newDomain: {}}}) + singleton := domainSet{ + domains: map[string]struct{}{newDomain: {}}, + mutex: &sync.Mutex{}, + } + s.reverseDNSCache.Set(ip, singleton) } else { // TODO: Be smarter about the expiration of entries with pre-existing associated domains domainSet := existingDomains.(domainSet) - domainSet.Lock() + domainSet.mutex.Lock() domainSet.domains[newDomain] = struct{}{} - domainSet.Unlock() + domainSet.mutex.Unlock() } } } From 9778be760bd07e632de4a07ee0643a40b4dc419d Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Fri, 23 Jun 2017 09:18:51 +0000 Subject: [PATCH 3/4] Use a global lock instead --- probe/endpoint/dns_snooper_linux_amd64.go | 36 ++++++++--------------- 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/probe/endpoint/dns_snooper_linux_amd64.go b/probe/endpoint/dns_snooper_linux_amd64.go index 0aaa1113d..3963b60bd 100644 --- a/probe/endpoint/dns_snooper_linux_amd64.go +++ b/probe/endpoint/dns_snooper_linux_amd64.go @@ -24,20 +24,15 @@ const ( // DNSSnooper is a snopper of DNS queries type DNSSnooper struct { - stop chan struct{} - pcapHandle *pcap.Handle + stop chan struct{} + pcapHandle *pcap.Handle + // gcache is goroutine-safe, but the + // values cached values aren't + reverseDNSMutex sync.RWMutex reverseDNSCache gcache.Cache decodingErrorCounts map[string]uint64 // for limiting } -type domainSet struct { - // Not worth using a RMutex since we don't expect a lot of contention - // and they are considerably larger (8 bytes vs 24 bytes), which would - // bloat the cache - mutex *sync.Mutex - domains map[string]struct{} -} - // NewDNSSnooper creates a new snooper of DNS queries func NewDNSSnooper() (*DNSSnooper, error) { pcapHandle, err := newPcapHandle() @@ -110,13 +105,11 @@ func (s *DNSSnooper) CachedNamesForIP(ip string) []string { if err != nil { return result } - - domainSet := domains.(domainSet) - domainSet.mutex.Lock() - for domain := range domainSet.domains { + s.reverseDNSMutex.RLock() + for domain := range domains.(map[string]struct{}) { result = append(result, domain) } - domainSet.mutex.Unlock() + s.reverseDNSMutex.RUnlock() return result } @@ -281,17 +274,12 @@ func (s *DNSSnooper) processDNSMessage(dns *layers.DNS) { log.Debugf("DNSSnooper: caught DNS lookup: %s -> %v", newDomain, ips) for ip := range ips { if existingDomains, err := s.reverseDNSCache.Get(ip); err != nil { - singleton := domainSet{ - domains: map[string]struct{}{newDomain: {}}, - mutex: &sync.Mutex{}, - } - s.reverseDNSCache.Set(ip, singleton) + s.reverseDNSCache.Set(ip, map[string]struct{}{newDomain: {}}) } else { // TODO: Be smarter about the expiration of entries with pre-existing associated domains - domainSet := existingDomains.(domainSet) - domainSet.mutex.Lock() - domainSet.domains[newDomain] = struct{}{} - domainSet.mutex.Unlock() + s.reverseDNSMutex.Lock() + existingDomains.(map[string]struct{})[newDomain] = struct{}{} + s.reverseDNSMutex.Unlock() } } } From 4006040cc196464c5a6fc8cdccf2c521dd55a62b Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Fri, 23 Jun 2017 23:52:30 +0000 Subject: [PATCH 4/4] Review feedback --- probe/endpoint/dns_snooper_linux_amd64.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/probe/endpoint/dns_snooper_linux_amd64.go b/probe/endpoint/dns_snooper_linux_amd64.go index 3963b60bd..2a9d6be58 100644 --- a/probe/endpoint/dns_snooper_linux_amd64.go +++ b/probe/endpoint/dns_snooper_linux_amd64.go @@ -26,8 +26,7 @@ const ( type DNSSnooper struct { stop chan struct{} pcapHandle *pcap.Handle - // gcache is goroutine-safe, but the - // values cached values aren't + // gcache is goroutine-safe, but the cached values aren't reverseDNSMutex sync.RWMutex reverseDNSCache gcache.Cache decodingErrorCounts map[string]uint64 // for limiting