amnezia-vpn/amneziawg-go

Fork 0

mirror of https://github.com/amnezia-vpn/amneziawg-go.git synced 2026-10-13 21:34:25 +00:00

[PR #205] fix: clear the timer duration under modifyingLock #203

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/amnezia-vpn/amneziawg-go/pull/205
Author: @StarProxima
Created: 10/9/2026
Status: 🔄 Open

Base: master ← Head: fix/timer-duration-race

📝 Commits (1)

0421ab3 fix: clear the timer duration under modifyingLock

📊 Changes

2 files changed (+32 additions, -1 deletions)

View changed files

📝 device/timers.go (+1 -1)
➕ device/timers_test.go (+31 -0)

📄 Description

Fixes #204.

What changes

In the expiry function of Timer, timer.duration = 0 moves before modifyingLock.Unlock() (device/timers.go#L39-L41):

 		duration := timer.duration
-		timer.modifyingLock.Unlock()
 		timer.duration = 0
+		timer.modifyingLock.Unlock()

It also adds TestTimerExpiryRace in device/timers_test.go, which fails under the race detector without the change.

Why this way

Mod, Del and IsPending access duration under modifyingLock; the expiry wrote it after unlocking. Besides the race report, a Mod that ran between the unlock and the write lost its duration: the timer stayed armed with duration == 0, so its next expiry returned without calling the handler. Clearing the field under the lock is the order wireguard-go keeps for isPending and the order this code had before 3.0.

The change keeps duration as both the pending flag and the value passed to the handler, so it touches one line.

Relation to #194

The last commit of #194 fixes the same race differently: it restores a separate isPending flag. That also changes one more case: with duration as the flag, Mod(0) leaves the timer not pending and its expiry does nothing, and Mod(0) is reachable when a range such as keepalive_timeout = 0-10 picks 0. This PR does not change that case. If #194 is merged first, the test from this PR still applies to it: it passes on the head of #194 (0 failures in 100 runs).

How it was tested

macOS 26 arm64, Go 1.26.4.

Check	`master` (`b5928ef`) with the test only	This PR
`TestTimerExpiryRace` under `-race`, 100 separate processes	100 of 100 fail, `DATA RACE` between `timers.go:41` (write) and `timers.go:73` (`IsPending`)	0 of 100 fail
`TestTimerExpiryRace` under `-race`, `-count=1000` in one process		pass
`TestTimerExpiryRace` without `-race`, `-count=1000`	pass	pass

Separate processes, because the race detector reports a given race only once per process.

The rest of the suite on this branch:

gofmt -l . is clean, go build ./... passes.
go vet ./... reports only the copylocks warning at device/pools_test.go:67, as on master (#157, #192).
go test ./... -count=1 and go test -race ./... -count=1 pass except TestAWGDevicePing, which fails on master as well because of #168 (20 of 20 runs on this machine). With a fix for #168 on top, go test -race ./... -count=1 passes in full, and TestAWGDevicePing passes 10 of 10 under -race.
Cross-compiled with CGO_ENABLED=0 for linux/amd64, arm64, 386, arm; windows/amd64, arm64, 386; darwin/amd64, arm64; freebsd/amd64; openbsd/amd64; and the library packages for android/arm64, arm, amd64 and ios/arm64. outline/ is left out of these builds because golang.getoutline.org/sdk/x/smart needs cgo on unix targets, which is the same on master.
In an Xray-core integration that runs amneziawg-go for its WireGuard outbound and inbound, its package tests pass under -race with this change and a fix for #168.

Not tested

Runtime on Linux, Windows, Android or iOS; those targets were only cross-compiled.
The lost Mod described above was found by reading the code; I have no test that hits the window between the unlock and the write.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/amnezia-vpn/amneziawg-go/pull/205 **Author:** [@StarProxima](https://github.com/StarProxima) **Created:** 10/9/2026 **Status:** 🔄 Open **Base:** `master` ← **Head:** `fix/timer-duration-race` --- ### 📝 Commits (1) - [`0421ab3`](https://github.com/amnezia-vpn/amneziawg-go/commit/0421ab3db604c66f4f4895f9c677f0309481d889) fix: clear the timer duration under modifyingLock ### 📊 Changes **2 files changed** (+32 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `device/timers.go` (+1 -1) ➕ `device/timers_test.go` (+31 -0) </details> ### 📄 Description Fixes #204. ## What changes In the expiry function of `Timer`, `timer.duration = 0` moves before `modifyingLock.Unlock()` ([device/timers.go#L39-L41](https://github.com/amnezia-vpn/amneziawg-go/blob/v3.1.20260828/device/timers.go#L39-L41)): ```diff duration := timer.duration - timer.modifyingLock.Unlock() timer.duration = 0 + timer.modifyingLock.Unlock() ``` It also adds `TestTimerExpiryRace` in `device/timers_test.go`, which fails under the race detector without the change. ## Why this way `Mod`, `Del` and `IsPending` access `duration` under `modifyingLock`; the expiry wrote it after unlocking. Besides the race report, a `Mod` that ran between the unlock and the write lost its duration: the timer stayed armed with `duration == 0`, so its next expiry returned without calling the handler. Clearing the field under the lock is the order wireguard-go keeps for `isPending` and the order this code had before 3.0. The change keeps `duration` as both the pending flag and the value passed to the handler, so it touches one line. ## Relation to #194 The last commit of #194 fixes the same race differently: it restores a separate `isPending` flag. That also changes one more case: with `duration` as the flag, `Mod(0)` leaves the timer not pending and its expiry does nothing, and `Mod(0)` is reachable when a range such as `keepalive_timeout = 0-10` picks 0. This PR does not change that case. If #194 is merged first, the test from this PR still applies to it: it passes on the head of #194 (0 failures in 100 runs). ## How it was tested macOS 26 arm64, Go 1.26.4. | Check | `master` (b5928ef) with the test only | This PR | |---|---|---| | `TestTimerExpiryRace` under `-race`, 100 separate processes | 100 of 100 fail, `DATA RACE` between `timers.go:41` (write) and `timers.go:73` (`IsPending`) | 0 of 100 fail | | `TestTimerExpiryRace` under `-race`, `-count=1000` in one process | | pass | | `TestTimerExpiryRace` without `-race`, `-count=1000` | pass | pass | Separate processes, because the race detector reports a given race only once per process. The rest of the suite on this branch: - `gofmt -l .` is clean, `go build ./...` passes. - `go vet ./...` reports only the copylocks warning at `device/pools_test.go:67`, as on `master` (#157, #192). - `go test ./... -count=1` and `go test -race ./... -count=1` pass except `TestAWGDevicePing`, which fails on `master` as well because of #168 (20 of 20 runs on this machine). With a fix for #168 on top, `go test -race ./... -count=1` passes in full, and `TestAWGDevicePing` passes 10 of 10 under `-race`. - Cross-compiled with `CGO_ENABLED=0` for linux/amd64, arm64, 386, arm; windows/amd64, arm64, 386; darwin/amd64, arm64; freebsd/amd64; openbsd/amd64; and the library packages for android/arm64, arm, amd64 and ios/arm64. `outline/` is left out of these builds because `golang.getoutline.org/sdk/x/smart` needs cgo on unix targets, which is the same on `master`. - In an Xray-core integration that runs amneziawg-go for its WireGuard outbound and inbound, its package tests pass under `-race` with this change and a fix for #168. ## Not tested - Runtime on Linux, Windows, Android or iOS; those targets were only cross-compiled. - The lost `Mod` described above was found by reading the code; I have no test that hits the window between the unlock and the write. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

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

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#203

No description provided.

Rows
Columns