From eca2de27c4e53490c657130c93eb1922bb279d9e Mon Sep 17 00:00:00 2001
From: Ayke van Laethem <aykevanlaethem@gmail.com>
Date: Tue, 10 Sep 2024 11:35:16 +0200
Subject: [PATCH] fix: avoid race conditions

These race conditions may seem harmless, and maybe they are unlikely to
cause real trouble, but:

 1. There is a theoretical chance they do. A partially written time
    value in one goroutine may result in an invalid (and very wrong)
    time in another, leading to misbehaving code.
 2. They introduce noise, and make it more difficult to find&fix other
    race conditions.
---
 ap/ap.go         | 10 ++++++++--
 dealer/dealer.go | 12 ++++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/ap/ap.go b/ap/ap.go
index cd17ffc..a7bde39 100644
--- a/ap/ap.go
+++ b/ap/ap.go
@@ -47,6 +47,7 @@ type Accesspoint struct {
 	recvChans         map[PacketType][]chan Packet
 	recvChansLock     sync.RWMutex
 	lastPongAck       time.Time
+	lastPongAckLock   sync.Mutex
 
 	// connMu is held for writing when performing reconnection and for reading mainly when accessing welcome
 	// or sending packets. If it's not held, a valid connection (and APWelcome) is available. Be careful not to deadlock
@@ -263,7 +264,9 @@ loop:
 				}
 			case PacketTypePongAck:
 				log.Tracef("received accesspoint pong ack")
+				ap.lastPongAckLock.Lock()
 				ap.lastPongAck = time.Now()
+				ap.lastPongAckLock.Unlock()
 				continue
 			default:
 				ap.recvChansLock.RLock()
@@ -323,8 +326,11 @@ loop:
 		case <-ap.pongAckTickerStop:
 			break loop
 		case <-ticker.C:
-			if time.Since(ap.lastPongAck) > pongAckInterval {
-				log.Errorf("did not receive last pong ack from accesspoint, %.0fs passed", time.Since(ap.lastPongAck).Seconds())
+			ap.lastPongAckLock.Lock()
+			timePassed := time.Since(ap.lastPongAck)
+			ap.lastPongAckLock.Unlock()
+			if timePassed > pongAckInterval {
+				log.Errorf("did not receive last pong ack from accesspoint, %.0fs passed", timePassed.Seconds())
 
 				// closing the connection should make the read on the "recvLoop" fail,
 				// continue hoping for a new connection
diff --git a/dealer/dealer.go b/dealer/dealer.go
index 6677585..8aed78f 100644
--- a/dealer/dealer.go
+++ b/dealer/dealer.go
@@ -30,6 +30,7 @@ type Dealer struct {
 	recvLoopStop   chan struct{}
 	recvLoopOnce   sync.Once
 	lastPong       time.Time
+	lastPongLock   sync.Mutex
 
 	// connMu is held for writing when performing reconnection and for reading when accessing the conn.
 	// If it's not held, a valid connection is available. Be careful not to deadlock anything with this.
@@ -125,8 +126,11 @@ loop:
 		case <-d.pingTickerStop:
 			break loop
 		case <-ticker.C:
-			if time.Since(d.lastPong) > pingInterval+timeout {
-				log.Errorf("did not receive last pong from dealer, %.0fs passed", time.Since(d.lastPong).Seconds())
+			d.lastPongLock.Lock()
+			timePassed := time.Since(d.lastPong)
+			d.lastPongLock.Unlock()
+			if timePassed > pingInterval+timeout {
+				log.Errorf("did not receive last pong from dealer, %.0fs passed", timePassed.Seconds())
 
 				// closing the connection should make the read on the "recvLoop" fail,
 				// continue hoping for a new connection
@@ -199,7 +203,9 @@ loop:
 				// we never receive ping messages
 				break
 			case "pong":
+				d.lastPongLock.Lock()
 				d.lastPong = time.Now()
+				d.lastPongLock.Unlock()
 				log.Tracef("received dealer pong")
 				break
 			default:
@@ -266,7 +272,9 @@ func (d *Dealer) reconnect() error {
 		return err
 	}
 
+	d.lastPongLock.Lock()
 	d.lastPong = time.Now()
+	d.lastPongLock.Unlock()
 	// restart the recv loop
 	go d.recvLoop()