From 77d515a1e43e5e377aefdbe8fd175557580924dc Mon Sep 17 00:00:00 2001 From: Volodymyr Komarov Date: Mon, 9 Mar 2020 00:07:17 +0200 Subject: [PATCH 1/4] Add more tests to increase test coverage. Fix some cases that could cause unexpected blocking. --- Makefile | 5 + scanner/host/scanner.go | 55 ++++++--- scanner/host/scanner_test.go | 211 +++++++++++++++++++++++++++++++++++ 3 files changed, 255 insertions(+), 16 deletions(-) create mode 100644 Makefile create mode 100644 scanner/host/scanner_test.go diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..ae01790 --- /dev/null +++ b/Makefile @@ -0,0 +1,5 @@ + +.PHONY: test + +test: + go test -v ./... diff --git a/scanner/host/scanner.go b/scanner/host/scanner.go index 96c23ac..72b22ee 100644 --- a/scanner/host/scanner.go +++ b/scanner/host/scanner.go @@ -14,28 +14,43 @@ import ( ) type Scanner struct { - mu sync.RWMutex - unique map[string]bool - Hosts []*Host - stop chan struct{} - Done chan struct{} - Error error + mu sync.RWMutex + unique map[string]bool + Hosts []*Host + started bool + stopped bool + stop chan struct{} + Done chan struct{} + Error error } func NewScanner() *Scanner { return &Scanner{ - mu: sync.RWMutex{}, - stop: make(chan struct{}), - unique: make(map[string]bool), - Hosts: make([]*Host, 0), - Done: make(chan struct{}), + mu: sync.RWMutex{}, + started: false, + stopped: false, + stop: make(chan struct{}), + unique: make(map[string]bool), + Hosts: make([]*Host, 0), + Done: make(chan struct{}), } } func (s *Scanner) Stop() { - s.stop <- struct{}{} - close(s.stop) - <-s.Done + if s.started { + s.stop <- struct{}{} + <-s.Done + } + + if !s.stopped { + close(s.stop) + + if !s.started { + close(s.Done) + } + } + + s.stopped = true } func (s *Scanner) finish(err error) { @@ -43,8 +58,10 @@ func (s *Scanner) finish(err error) { s.Error = err } - s.Done <- struct{}{} - close(s.Done) + if s.started && !s.stopped { + s.Done <- struct{}{} + close(s.Done) + } } func (s *Scanner) HasHost(host *Host) bool { @@ -161,6 +178,12 @@ func (s *Scanner) scanInterface(iface *net.Interface) error { // Push new Host once any correct response received // Work until 'stop' is closed. func (s *Scanner) listenARP(handle *pcap.Handle, iface *net.Interface) { + s.mu.Lock() + if !s.started { + s.started = true + } + s.mu.Unlock() + src := gopacket.NewPacketSource(handle, layers.LayerTypeEthernet) in := src.Packets() diff --git a/scanner/host/scanner_test.go b/scanner/host/scanner_test.go new file mode 100644 index 0000000..2da3b4a --- /dev/null +++ b/scanner/host/scanner_test.go @@ -0,0 +1,211 @@ +package host + +import ( + "testing" + "time" +) + +func TestNewScanner(t *testing.T) { + scanner := NewScanner() + if scanner == nil { + t.Errorf("Scanner instance is empty") + return + } + + if scanner.unique == nil { + t.Errorf("Unique host registry is not initialised") + } + + if scanner.started == true { + t.Errorf("Scanner has wrong started flag") + } + + if scanner.stopped == true { + t.Errorf("Scanner has wrong stopped flag") + } + + if scanner.Hosts == nil { + t.Errorf("Host storage is not initialised") + } + + if scanner.Done == nil { + t.Errorf("Done channel is not created") + } + + if scanner.stop == nil { + t.Errorf("Stop channel is not created") + } +} + +func TestScanner_StopEmpty(t *testing.T) { + scanner := NewScanner() + if scanner == nil { + t.Errorf("Scanner instance is empty") + return + } + + scanner.Stop() + + ok := true + select { + case _, ok = <-scanner.stop: + default: + } + + if ok { + t.Errorf("stop channel must be closed once Stop method called") + } +} + +func TestScanner_StopStartedStopped(t *testing.T) { + scanner := NewScanner() + if scanner == nil { + t.Errorf("Scanner instance is empty") + return + } + + go scanner.Scan() + scanner.Stop() + + ok := true + select { + case _, ok = <-scanner.stop: + default: + } + + if ok { + t.Errorf("stop channel must be closed once Stop method called") + } + + ok = true + select { + case _, ok = <-scanner.Done: + default: + } + if ok { + t.Errorf("done channel must be closed once Stop method finished") + } + + scanner.Stop() + + ok = true + select { + case _, ok = <-scanner.stop: + default: + } + if ok { + t.Errorf("stop channel must be closed once Stop method called") + } + + ok = true + select { + case _, ok = <-scanner.Done: + default: + } + if ok { + t.Errorf("done channel must be closed once Stop method finished") + } +} + +func TestScanner_StopWorking(t *testing.T) { + scanner := NewScanner() + if scanner == nil { + t.Errorf("Scanner instance is empty") + return + } + + // simulate fake scanner + go func(s *Scanner) { + s.started = true + + select { + case <-s.stop: + s.finish(nil) + } + }(scanner) + + time.Sleep(1 * time.Second) + + scanner.Stop() + + ok := true + select { + case _, ok = <-scanner.stop: + default: + } + + if ok { + t.Errorf("stop channel must be closed once Stop method called") + } + + ok = true + select { + case _, ok = <-scanner.Done: + default: + } + if ok { + t.Errorf("done channel must be closed once Stop method finished") + } +} + +func TestScanner_AddHost(t *testing.T) { + scanner := NewScanner() + if scanner == nil { + t.Errorf("Scanner instance is empty") + return + } + + host := Host{ + IP: "127.0.0.1", + MAC: "ff:ff:ff:ff:ff:ff", + } + + scanner.AddHost(&host) + + if len(scanner.Hosts) != 1 { + t.Errorf("Host is not added") + } + + if host.ID() != scanner.Hosts[0].ID() { + t.Errorf("Host is added but changed") + } + + if host.IP != scanner.Hosts[0].IP { + t.Errorf("Host is added but changed IP") + } + + if host.MAC != scanner.Hosts[0].MAC { + t.Errorf("Host is added but changed MAC") + } + + if len(scanner.unique) != 1 { + t.Errorf("Host is not registered to unique registry") + } + + if _, ok := scanner.unique[host.ID()]; !ok { + t.Errorf("Host is not registered to unique registry") + } +} + +func TestScanner_HasHost(t *testing.T) { + scanner := NewScanner() + if scanner == nil { + t.Errorf("Scanner instance is empty") + return + } + + host := Host{ + IP: "127.0.0.1", + MAC: "ff:ff:ff:ff:ff:ff", + } + + if scanner.HasHost(&host) { + t.Errorf("Host is wrongly detected as already added") + } + + scanner.AddHost(&host) + + if !scanner.HasHost(&host) { + t.Errorf("Host is not registered as already addded") + } +} From eb4e194e13bd26eb2dddff9c9f5f4490361ecaa7 Mon Sep 17 00:00:00 2001 From: Volodymyr Komarov Date: Mon, 9 Mar 2020 00:15:18 +0200 Subject: [PATCH 2/4] Simplify testing channels. Simplify simulating started scanner. --- scanner/host/scanner_test.go | 47 ++++++++---------------------------- 1 file changed, 10 insertions(+), 37 deletions(-) diff --git a/scanner/host/scanner_test.go b/scanner/host/scanner_test.go index 2da3b4a..ebb5b68 100644 --- a/scanner/host/scanner_test.go +++ b/scanner/host/scanner_test.go @@ -46,13 +46,9 @@ func TestScanner_StopEmpty(t *testing.T) { scanner.Stop() - ok := true select { - case _, ok = <-scanner.stop: + case <-scanner.stop: default: - } - - if ok { t.Errorf("stop channel must be closed once Stop method called") } } @@ -67,42 +63,29 @@ func TestScanner_StopStartedStopped(t *testing.T) { go scanner.Scan() scanner.Stop() - ok := true select { - case _, ok = <-scanner.stop: + case <-scanner.stop: default: - } - - if ok { t.Errorf("stop channel must be closed once Stop method called") } - ok = true select { - case _, ok = <-scanner.Done: + case <-scanner.Done: default: - } - if ok { t.Errorf("done channel must be closed once Stop method finished") } scanner.Stop() - ok = true select { - case _, ok = <-scanner.stop: + case <-scanner.stop: default: - } - if ok { t.Errorf("stop channel must be closed once Stop method called") } - ok = true select { - case _, ok = <-scanner.Done: + case <-scanner.Done: default: - } - if ok { t.Errorf("done channel must be closed once Stop method finished") } } @@ -117,33 +100,23 @@ func TestScanner_StopWorking(t *testing.T) { // simulate fake scanner go func(s *Scanner) { s.started = true - - select { - case <-s.stop: - s.finish(nil) - } + <-s.stop + s.finish(nil) }(scanner) - time.Sleep(1 * time.Second) + time.Sleep(1 * time.Millisecond) scanner.Stop() - ok := true select { - case _, ok = <-scanner.stop: + case <-scanner.stop: default: - } - - if ok { t.Errorf("stop channel must be closed once Stop method called") } - ok = true select { - case _, ok = <-scanner.Done: + case <-scanner.Done: default: - } - if ok { t.Errorf("done channel must be closed once Stop method finished") } } From a581c9ad7950a39b4d0cfbc9f7e66fd29152c3b8 Mon Sep 17 00:00:00 2001 From: Volodymyr Komarov Date: Mon, 9 Mar 2020 00:45:05 +0200 Subject: [PATCH 3/4] Add comments to exported structs, functions. Refactor exported structs and methods. --- .golangci.yml | 6 ++++++ hosts.go | 4 ++-- main.go | 4 ++-- ports.go | 4 ++-- scanner/host/host.go | 3 +++ scanner/host/scanner.go | 20 +++++++++++++------- scanner/host/scanner_test.go | 8 ++++---- 7 files changed, 32 insertions(+), 17 deletions(-) create mode 100644 .golangci.yml diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..bfc0be9 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,6 @@ +service: + analyzed-paths: + - scanner/... + golangci-lint-version: 1.12.x + prepare: + - sudo apt-get install libpcap-dev diff --git a/hosts.go b/hosts.go index f973197..78abf9a 100644 --- a/hosts.go +++ b/hosts.go @@ -9,11 +9,11 @@ import ( "github.com/vokomarov/netshark/scanner/host" ) -type HostsCommand struct { +type hostsCommand struct { Timeout int `short:"t" long:"timeout" default:"5" description:"Timeout in seconds to wait for ARP responses."` } -func (c *HostsCommand) Execute(_ []string) error { +func (c *hostsCommand) Execute(_ []string) error { fmt.Printf("Scanning Hosts..\n") quit := make(chan os.Signal, 1) diff --git a/main.go b/main.go index ce78d8c..ae7d653 100644 --- a/main.go +++ b/main.go @@ -10,8 +10,8 @@ import ( var parser *flags.Parser type scanCommand struct { - HostsCommand HostsCommand `command:"hosts" description:"Scan all available neighbor hosts of current local network"` - PortsCommand PortsCommand `command:"ports" description:"Scan open ports on a host"` + HostsCommand hostsCommand `command:"hosts" description:"Scan all available neighbor hosts of current local network"` + PortsCommand portsCommand `command:"ports" description:"Scan open ports on a host"` } func registerCommands(parser *flags.Parser) { diff --git a/ports.go b/ports.go index bad95b9..07d7353 100644 --- a/ports.go +++ b/ports.go @@ -4,10 +4,10 @@ import ( "fmt" ) -type PortsCommand struct { +type portsCommand struct { } -func (c *PortsCommand) Execute(_ []string) error { +func (c *portsCommand) Execute(_ []string) error { fmt.Printf("Scanning Ports..\n") return nil } diff --git a/scanner/host/host.go b/scanner/host/host.go index 1b0d4c4..85b800e 100644 --- a/scanner/host/host.go +++ b/scanner/host/host.go @@ -6,12 +6,15 @@ import ( "io" ) +// Host struct host information about discovered network client type Host struct { id string IP string MAC string } +// ID will generate unique MD5 hash of host by his properties +// and cache generated hash for future usage func (h *Host) ID() string { if h.id == "" { hash := md5.New() diff --git a/scanner/host/scanner.go b/scanner/host/scanner.go index 72b22ee..da4671c 100644 --- a/scanner/host/scanner.go +++ b/scanner/host/scanner.go @@ -13,6 +13,8 @@ import ( "github.com/google/gopacket/pcap" ) +// Scanner provide container for control local network scanning +// process and checking results type Scanner struct { mu sync.RWMutex unique map[string]bool @@ -24,6 +26,7 @@ type Scanner struct { Error error } +// NewScanner will initialise new instance of Scanner func NewScanner() *Scanner { return &Scanner{ mu: sync.RWMutex{}, @@ -36,6 +39,9 @@ func NewScanner() *Scanner { } } +// Stop perform manually stopping of scan process with blocking +// until stopping is not finished in case of scanning already started +// Safe to call before or after scanning started or stopped func (s *Scanner) Stop() { if s.started { s.stop <- struct{}{} @@ -64,7 +70,7 @@ func (s *Scanner) finish(err error) { } } -func (s *Scanner) HasHost(host *Host) bool { +func (s *Scanner) hasHost(host *Host) bool { s.mu.RLock() defer s.mu.RUnlock() @@ -75,8 +81,7 @@ func (s *Scanner) HasHost(host *Host) bool { return false } -// Push new detected Host to the list of all detected -func (s *Scanner) AddHost(host *Host) *Scanner { +func (s *Scanner) addHost(host *Host) *Scanner { s.mu.Lock() defer s.mu.Unlock() @@ -86,9 +91,10 @@ func (s *Scanner) AddHost(host *Host) *Scanner { return s } -// Detect system interfaces and go over each one to detect IP addresses -// and read/write ARP packets +// Scan will detect system interfaces and go over each one to detect +// IP addresses to read/write ARP packets // Blocked until every interfaces unable to write packets or stop call +// so typically should be run as a goroutine func (s *Scanner) Scan() { interfaces, err := net.Interfaces() if err != nil { @@ -216,8 +222,8 @@ func (s *Scanner) listenARP(handle *pcap.Handle, iface *net.Interface) { MAC: fmt.Sprintf("%v", net.HardwareAddr(arp.SourceHwAddress)), } - if !s.HasHost(&host) { - s.AddHost(&host) + if !s.hasHost(&host) { + s.addHost(&host) } } } diff --git a/scanner/host/scanner_test.go b/scanner/host/scanner_test.go index ebb5b68..6a1efd8 100644 --- a/scanner/host/scanner_test.go +++ b/scanner/host/scanner_test.go @@ -133,7 +133,7 @@ func TestScanner_AddHost(t *testing.T) { MAC: "ff:ff:ff:ff:ff:ff", } - scanner.AddHost(&host) + scanner.addHost(&host) if len(scanner.Hosts) != 1 { t.Errorf("Host is not added") @@ -172,13 +172,13 @@ func TestScanner_HasHost(t *testing.T) { MAC: "ff:ff:ff:ff:ff:ff", } - if scanner.HasHost(&host) { + if scanner.hasHost(&host) { t.Errorf("Host is wrongly detected as already added") } - scanner.AddHost(&host) + scanner.addHost(&host) - if !scanner.HasHost(&host) { + if !scanner.hasHost(&host) { t.Errorf("Host is not registered as already addded") } } From 7038f88df988bfb05341078a525fe2e702069be2 Mon Sep 17 00:00:00 2001 From: Volodymyr Komarov Date: Mon, 9 Mar 2020 01:00:48 +0200 Subject: [PATCH 4/4] Linter fixes and configuration --- .golangci.yml | 6 ++++-- hosts.go | 1 + main.go | 8 ++++---- ports.go | 1 + 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index bfc0be9..69ca7db 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,6 +1,8 @@ service: analyzed-paths: - scanner/... - golangci-lint-version: 1.12.x + golangci-lint-version: 1.23.x prepare: - - sudo apt-get install libpcap-dev + - apt-get update && apt-get install -y libpcap-dev + suggested-changes: + disabled: true diff --git a/hosts.go b/hosts.go index 78abf9a..bb37293 100644 --- a/hosts.go +++ b/hosts.go @@ -13,6 +13,7 @@ type hostsCommand struct { Timeout int `short:"t" long:"timeout" default:"5" description:"Timeout in seconds to wait for ARP responses."` } +// Execute will run the command func (c *hostsCommand) Execute(_ []string) error { fmt.Printf("Scanning Hosts..\n") diff --git a/main.go b/main.go index ae7d653..f0fd881 100644 --- a/main.go +++ b/main.go @@ -33,11 +33,11 @@ func main() { if flagsErr, ok := err.(*flags.Error); ok && flagsErr.Type == flags.ErrHelp { os.Exit(0) return - } else { - fmt.Printf("Error: %v\n", err) - os.Exit(1) - return } + + fmt.Printf("Error: %v\n", err) + os.Exit(1) + return } os.Exit(0) diff --git a/ports.go b/ports.go index 07d7353..b8f53a7 100644 --- a/ports.go +++ b/ports.go @@ -7,6 +7,7 @@ import ( type portsCommand struct { } +// Execute will run the command func (c *portsCommand) Execute(_ []string) error { fmt.Printf("Scanning Ports..\n") return nil