[GH-ISSUE #204] device: data race on Timer.duration between timer expiry and Mod/IsPending #94

New issue

Open

opened 2026-10-13 21:03:32 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-13 21:03:32 +00:00

Owner

Originally created by @StarProxima on GitHub (Oct 9, 2026).
Original GitHub issue: https://github.com/amnezia-vpn/amneziawg-go/issues/204

Version: v3.1.20260828 (master at b5928ef). The code comes from 457d920 ("feat: amneziawg 3.0") and is in every tag since v3.0.0.

What happens

The expiry function of a Timer reads timer.duration under modifyingLock, releases the lock and only then clears the field (device/timers.go#L34-L41):

		timer.modifyingLock.Lock()
		if timer.duration == 0 {
			timer.modifyingLock.Unlock()
			return
		}
		duration := timer.duration
		timer.modifyingLock.Unlock()
		timer.duration = 0

Mod, Del and IsPending access the same field under modifyingLock (L49-L74). Nothing orders the write on line 41 with them, which has two consequences:

It is a data race. Any program that runs a device under go test -race can hit it; we did once in the tests of an Xray-core integration, with the write on line 41 in the report.
A Mod that runs between the unlock and the write loses its duration. The timer is armed again, but duration is back to 0, so when it fires, the expiry function returns early without calling the handler, and IsPending reports false until then. On 32-bit platforms the unsynchronized int64 access can also tear. The window is a few instructions wide and most handlers re-arm their own timer, so I have not seen a visible effect beyond the race reports.

Before 3.0 the timer cleared its pending state before unlocking, as wireguard-go still does with isPending (wireguard-go device/timers.go).

Reproduction

Add to device/:

package device

import (
	"testing"
	"time"
)

func TestTimerExpiryRace(t *testing.T) {
	var peer Peer
	timer := peer.NewTimer(func(*Peer, time.Duration) {})
	defer timer.DelSync()

	deadline := time.Now().Add(5 * time.Second)
	for i := 0; i < 100; i++ {
		timer.Mod(time.Microsecond)
		for timer.IsPending() {
			if time.Now().After(deadline) {
				t.Fatal("the timer did not expire")
			}
		}
	}
}

$ go test -race ./device/ -run '^TestTimerExpiryRace$' -count=1
WARNING: DATA RACE
Read at 0x00c00010cbc8 by goroutine 8:
  github.com/amnezia-vpn/amneziawg-go/v3/device.(*Timer).IsPending()
      device/timers.go:73 +0x7c
  github.com/amnezia-vpn/amneziawg-go/v3/device.TestTimerExpiryRace()
      device/timers_test.go:25 +0x138

Previous write at 0x00c00010cbc8 by goroutine 10:
  github.com/amnezia-vpn/amneziawg-go/v3/device.(*Peer).NewTimer.func1()
      device/timers.go:41 +0xec
...
--- FAIL: TestTimerExpiryRace (0.01s)
    testing.go:1712: race detected during execution of test

The loop waits in IsPending until the expiry clears the duration, so the read always follows the unlock on line 40 and the race detector sees it every time: 100 failures in 100 separate runs (macOS arm64, Go 1.26.4). Separate processes, because the detector reports a given race only once per process.

Expected

No race: the expiry clears the duration while it still holds modifyingLock, and a Mod that runs after the expiry read the duration keeps its value.

#194 includes a fix for this in its last commit ("device: fix race between timer expiry and reset", which restores a separate isPending flag), together with four unrelated backports from wireguard-go. The test above passes on the head of #194.

Originally created by @StarProxima on GitHub (Oct 9, 2026). Original GitHub issue: https://github.com/amnezia-vpn/amneziawg-go/issues/204 **Version:** v3.1.20260828 (`master` at b5928ef). The code comes from 457d920 ("feat: amneziawg 3.0") and is in every tag since v3.0.0. ## What happens The expiry function of a `Timer` reads `timer.duration` under `modifyingLock`, releases the lock and only then clears the field ([device/timers.go#L34-L41](https://github.com/amnezia-vpn/amneziawg-go/blob/v3.1.20260828/device/timers.go#L34-L41)): ```go timer.modifyingLock.Lock() if timer.duration == 0 { timer.modifyingLock.Unlock() return } duration := timer.duration timer.modifyingLock.Unlock() timer.duration = 0 ``` `Mod`, `Del` and `IsPending` access the same field under `modifyingLock` ([L49-L74](https://github.com/amnezia-vpn/amneziawg-go/blob/v3.1.20260828/device/timers.go#L49-L74)). Nothing orders the write on line 41 with them, which has two consequences: 1. It is a data race. Any program that runs a device under `go test -race` can hit it; we did once in the tests of an Xray-core integration, with the write on line 41 in the report. 2. A `Mod` that runs between the unlock and the write loses its duration. The timer is armed again, but `duration` is back to 0, so when it fires, the expiry function returns early without calling the handler, and `IsPending` reports false until then. On 32-bit platforms the unsynchronized `int64` access can also tear. The window is a few instructions wide and most handlers re-arm their own timer, so I have not seen a visible effect beyond the race reports. Before 3.0 the timer cleared its pending state before unlocking, as wireguard-go still does with `isPending` ([wireguard-go device/timers.go](https://github.com/WireGuard/wireguard-go/blob/2631ce99a06f27120d581611cf125d68bc6aa565/device/timers.go#L28-L46)). ## Reproduction Add to `device/`: ```go package device import ( "testing" "time" ) func TestTimerExpiryRace(t *testing.T) { var peer Peer timer := peer.NewTimer(func(*Peer, time.Duration) {}) defer timer.DelSync() deadline := time.Now().Add(5 * time.Second) for i := 0; i < 100; i++ { timer.Mod(time.Microsecond) for timer.IsPending() { if time.Now().After(deadline) { t.Fatal("the timer did not expire") } } } } ``` ``` $ go test -race ./device/ -run '^TestTimerExpiryRace$' -count=1 WARNING: DATA RACE Read at 0x00c00010cbc8 by goroutine 8: github.com/amnezia-vpn/amneziawg-go/v3/device.(*Timer).IsPending() device/timers.go:73 +0x7c github.com/amnezia-vpn/amneziawg-go/v3/device.TestTimerExpiryRace() device/timers_test.go:25 +0x138 Previous write at 0x00c00010cbc8 by goroutine 10: github.com/amnezia-vpn/amneziawg-go/v3/device.(*Peer).NewTimer.func1() device/timers.go:41 +0xec ... --- FAIL: TestTimerExpiryRace (0.01s) testing.go:1712: race detected during execution of test ``` The loop waits in `IsPending` until the expiry clears the duration, so the read always follows the unlock on line 40 and the race detector sees it every time: 100 failures in 100 separate runs (macOS arm64, Go 1.26.4). Separate processes, because the detector reports a given race only once per process. ## Expected No race: the expiry clears the duration while it still holds `modifyingLock`, and a `Mod` that runs after the expiry read the duration keeps its value. #194 includes a fix for this in its last commit ("device: fix race between timer expiry and reset", which restores a separate `isPending` flag), together with four unrelated backports from wireguard-go. The test above passes on the head of #194.

No labels

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

amnezia-vpn/amneziawg-go#94

No description provided.

Rows
Columns