From 782041509f4130e8e795b22379368239d5091c8f Mon Sep 17 00:00:00 2001
From: Ghanan Gowripalan <ghanan@google.com>
Date: Mon, 20 Apr 2020 16:31:39 -0700
Subject: Prevent race when reassigning CancellableTimer

Capture a timer's locker for each instance of a CancellableTimer so that
reassigning a tcpip.CancellableTimer does not cause a data race.

Reassigning a tcpip.CancellableTimer updates its underlying locker. When
a timer fires, it does a read of the timer's locker variable to lock it.
This read of the locker was not synchronized so a race existed where one
goroutine may reassign the timer (updating the locker) and another
handles the timer firing (attempts to lock the timer's locker).

Test: tcpip_test.TestCancellableTimerReassignment
PiperOrigin-RevId: 307499822
---
 pkg/tcpip/timer.go      |  8 ++++++--
 pkg/tcpip/timer_test.go | 25 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)

(limited to 'pkg')

diff --git a/pkg/tcpip/timer.go b/pkg/tcpip/timer.go
index f5f01f32f..67f66fc72 100644
--- a/pkg/tcpip/timer.go
+++ b/pkg/tcpip/timer.go
@@ -131,10 +131,14 @@ func (t *CancellableTimer) StopLocked() {
 func (t *CancellableTimer) Reset(d time.Duration) {
 	// Create a new instance.
 	earlyReturn := false
+
+	// Capture the locker so that updating the timer does not cause a data race
+	// when a timer fires and tries to obtain the lock (read the timer's locker).
+	locker := t.locker
 	t.instance = cancellableTimerInstance{
 		timer: time.AfterFunc(d, func() {
-			t.locker.Lock()
-			defer t.locker.Unlock()
+			locker.Lock()
+			defer locker.Unlock()
 
 			if earlyReturn {
 				// If we reach this point, it means that the timer fired while another
diff --git a/pkg/tcpip/timer_test.go b/pkg/tcpip/timer_test.go
index 2d20f7ef3..730134906 100644
--- a/pkg/tcpip/timer_test.go
+++ b/pkg/tcpip/timer_test.go
@@ -28,6 +28,31 @@ const (
 	longDuration   = 1 * time.Second
 )
 
+func TestCancellableTimerReassignment(t *testing.T) {
+	var timer tcpip.CancellableTimer
+	var wg sync.WaitGroup
+	var lock sync.Mutex
+
+	for i := 0; i < 2; i++ {
+		wg.Add(1)
+
+		go func() {
+			lock.Lock()
+			// Assigning a new timer value updates the timer's locker and function.
+			// This test makes sure there is no data race when reassigning a timer
+			// that has an active timer (even if it has been stopped as a stopped
+			// timer may be blocked on a lock before it can check if it has been
+			// stopped while another goroutine holds the same lock).
+			timer = tcpip.MakeCancellableTimer(&lock, func() {
+				wg.Done()
+			})
+			timer.Reset(shortDuration)
+			lock.Unlock()
+		}()
+	}
+	wg.Wait()
+}
+
 func TestCancellableTimerFire(t *testing.T) {
 	t.Parallel()
 
-- 
cgit v1.2.3