From f7ce6a1fdbf60ab6632c307e4f28b4492a96f90c Mon Sep 17 00:00:00 2001
From: David Wragg <david@wragg.org>
Date: Tue, 25 Aug 2015 15:37:27 +0100
Subject: [PATCH] Injected packets shouldn't happen in handleCapturedPacket

Check for them just in case, but don't take an extra lock to do it.
---
 router/bridge.go    |  3 ++-
 router/mac_cache.go | 29 +++++++++++++++++++++++------
 router/router.go    | 21 +++++++++++----------
 3 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/router/bridge.go b/router/bridge.go
index 603d8687..00481a0a 100644
--- a/router/bridge.go
+++ b/router/bridge.go
@@ -5,7 +5,8 @@ type Bridge interface {
 	// Inject a packet to be delivered locally
 	InjectPacket(PacketKey) FlowOp
 
-	// Start consuming packets from the bridge
+	// Start consuming packets from the bridge.  Injected packets
+	// should not be included.
 	StartConsumingPackets(BridgeConsumer) error
 
 	String() string
diff --git a/router/mac_cache.go b/router/mac_cache.go
index a798c48a..d0eb7684 100644
--- a/router/mac_cache.go
+++ b/router/mac_cache.go
@@ -28,32 +28,49 @@ func NewMacCache(maxAge time.Duration, onExpiry func(net.HardwareAddr, *Peer)) *
 	return cache
 }
 
-func (cache *MacCache) Enter(mac net.HardwareAddr, peer *Peer) bool {
+func (cache *MacCache) add(mac net.HardwareAddr, peer *Peer, force bool) (bool, *Peer) {
 	key := macint(mac)
 	now := time.Now()
+
 	cache.RLock()
 	entry, found := cache.table[key]
 	if found && entry.peer == peer && now.Before(entry.lastSeen.Add(cache.maxAge/10)) {
 		cache.RUnlock()
-		return false
+		return false, nil
 	}
 	cache.RUnlock()
+
 	cache.Lock()
 	defer cache.Unlock()
+
 	entry, found = cache.table[key]
 	if !found {
 		cache.table[key] = &MacCacheEntry{lastSeen: now, peer: peer}
-		return true
+		return true, nil
 	}
+
 	if entry.peer != peer {
-		entry.lastSeen = now
+		if !force {
+			return false, entry.peer
+		}
+
 		entry.peer = peer
-		return true
 	}
+
 	if now.After(entry.lastSeen.Add(cache.maxAge / 10)) {
 		entry.lastSeen = now
 	}
-	return false
+
+	return false, nil
+}
+
+func (cache *MacCache) Add(mac net.HardwareAddr, peer *Peer) (bool, *Peer) {
+	return cache.add(mac, peer, false)
+}
+
+func (cache *MacCache) AddForced(mac net.HardwareAddr, peer *Peer) bool {
+	newMac, _ := cache.add(mac, peer, true)
+	return newMac
 }
 
 func (cache *MacCache) Lookup(mac net.HardwareAddr) *Peer {
diff --git a/router/router.go b/router/router.go
index 481efd32..7c8cc62d 100644
--- a/router/router.go
+++ b/router/router.go
@@ -103,17 +103,18 @@ func (router *Router) UsingPassword() bool {
 func (router *Router) handleCapturedPacket(key PacketKey) FlowOp {
 	router.PacketLogging.LogPacket("Captured", key)
 	srcMac := net.HardwareAddr(key.SrcMAC[:])
-	srcPeer := router.Macs.Lookup(srcMac)
 
-	// We need to filter out frames we injected ourselves. For such
-	// frames, the srcMAC will have been recorded as associated with a
-	// different peer.
-	if srcPeer != nil && srcPeer != router.Ourself.Peer {
-		return nil
-	}
-
-	if router.Macs.Enter(srcMac, router.Ourself.Peer) {
+	switch newSrcMac, conflictPeer := router.Macs.Add(srcMac, router.Ourself.Peer); {
+	case newSrcMac:
 		log.Println("Discovered local MAC", srcMac)
+
+	case conflictPeer != nil:
+		// The MAC cache has an entry for the source MAC
+		// associated with another peer.  This probably means
+		// we are seeing a frame we injected ourself.  That
+		// shouldn't happen, but discard it just in case.
+		log.Errorln("Captured frame from MAC (", srcMac, ") associated with another peer", conflictPeer)
+		return nil
 	}
 
 	// Discard STP broadcasts
@@ -183,7 +184,7 @@ func (router *Router) handleForwardedPacket(key ForwardPacketKey) FlowOp {
 
 	srcMac := net.HardwareAddr(key.SrcMAC[:])
 	dstMac := net.HardwareAddr(key.DstMAC[:])
-	if router.Macs.Enter(srcMac, key.SrcPeer) {
+	if router.Macs.AddForced(srcMac, key.SrcPeer) {
 		log.Println("Discovered remote MAC", srcMac, "at", key.SrcPeer)
 	}