summaryrefslogtreecommitdiffhomepage
path: root/pkg/tcpip
diff options
context:
space:
mode:
authorGhanan Gowripalan <ghanan@google.com>2020-04-20 16:31:39 -0700
committergVisor bot <gvisor-bot@google.com>2020-04-20 16:32:44 -0700
commit782041509f4130e8e795b22379368239d5091c8f (patch)
tree81ed7df40421e1bce4f5c381ce91b79b5362ff11 /pkg/tcpip
parent1a597e01bed5d5fb30b3d444e0a23669c5587235 (diff)
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
Diffstat (limited to 'pkg/tcpip')
-rw-r--r--pkg/tcpip/timer.go8
-rw-r--r--pkg/tcpip/timer_test.go25
2 files changed, 31 insertions, 2 deletions
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()