[PR #36] fix: relay race condition in WireGuard session management #35

New Issue

GiteaMirror · 2025-11-19T07:03:47-06:00

GiteaMirror commented

2025-11-19 07:03:47 -06:00

📋 Pull Request Information

Original PR: https://github.com/fosrl/gerbil/pull/36
Author: @LaurenceJJones
Created: 11/13/2025
Status: 🔄 Open

Base: main ← Head: fix-wg-session-race-condition

📝 Commits (3)

ee27bf3 Fix race condition in WireGuard session management
a3f9a89 Refactor WireGuard session locking and remove unused methods
e282715 Merge branch 'main' into fix-wg-session-race-condition

📊 Changes

1 file changed (+41 additions, -14 deletions)

View changed files

📝 relay/relay.go (+41 -14)

📄 Description

Community Contribution License Agreement

By creating this pull request, I grant the project maintainers an unlimited,
perpetual license to use, modify, and redistribute these contributions under any terms they
choose, including both the AGPLv3 and the Fossorial Commercial license terms. I
represent that I have the right to grant this license for all contributed content.

Description

The race condition existed because while sync.Map is thread-safe for map operations (Load, Store, Delete, Range), it does not provide thread-safety for the data stored within it. When WireGuardSession structs were stored as pointers in the sync.Map, multiple goroutines could:

Retrieve the same session pointer from the map concurrently
Access and modify the session's fields (particularly LastSeen) without synchronization
Cause data races when one goroutine reads LastSeen while another updates it

This fix adds a sync.RWMutex to each WireGuardSession struct to protect concurrent access to its fields. All field access now goes through thread-safe methods that properly acquire/release the mutex.

Changes:

Added sync.RWMutex to WireGuardSession struct
Added thread-safe accessor methods (GetLastSeen, GetDestAddr, etc.)
Updated all session field accesses to use thread-safe methods
Removed redundant Store call after updating LastSeen (pointer update is atomic in Go, but field access within pointer was not)

How to test?

Understanding the Race Condition

The race condition occurs in these scenarios:

Cleanup goroutine reads LastSeen: The cleanupIdleSessions() function periodically reads session.LastSeen to check if sessions should be removed.
Packet handler updates LastSeen: The handleWireGuardPacket() function updates session.LastSeen = time.Now() when processing transport data packets.
Both happen concurrently: When both operations occur simultaneously on the same session pointer, the race detector detects unsynchronized access to the LastSeen field.

Notes

An easier fix could be here but I didnt want to change the type against the sync.map in case a pointer was chosen for a reason, in short instead of storing a pointer, storing the struct itself and when updating the any values just replace the key everytime (which you was doing when updating last seen, but since it was a pointer that was unnecessary). May cause some extra garbage collection but in this case cause the struct is tiny would be not noticeable.

let me know if you rather have atomic updates against the sync.map instead of locks on the struct.

_{🔄 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/fosrl/gerbil/pull/36 **Author:** [@LaurenceJJones](https://github.com/LaurenceJJones) **Created:** 11/13/2025 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix-wg-session-race-condition` --- ### 📝 Commits (3) - [`ee27bf3`](https://github.com/fosrl/gerbil/commit/ee27bf3153e4054d75f3d39ae7d94f0c697857ce) Fix race condition in WireGuard session management - [`a3f9a89`](https://github.com/fosrl/gerbil/commit/a3f9a89079eb6b5babb6bb32c14936d3fb9c3799) Refactor WireGuard session locking and remove unused methods - [`e282715`](https://github.com/fosrl/gerbil/commit/e282715251e0ca0abe44e671593b6e6bd6769d25) Merge branch 'main' into fix-wg-session-race-condition ### 📊 Changes **1 file changed** (+41 additions, -14 deletions) <details> <summary>View changed files</summary> 📝 `relay/relay.go` (+41 -14) </details> ### 📄 Description ## Community Contribution License Agreement By creating this pull request, I grant the project maintainers an unlimited, perpetual license to use, modify, and redistribute these contributions under any terms they choose, including both the AGPLv3 and the Fossorial Commercial license terms. I represent that I have the right to grant this license for all contributed content. ## Description The race condition existed because while sync.Map is thread-safe for map operations (Load, Store, Delete, Range), it does not provide thread-safety for the data stored within it. When WireGuardSession structs were stored as pointers in the sync.Map, multiple goroutines could: 1. Retrieve the same session pointer from the map concurrently 2. Access and modify the session's fields (particularly LastSeen) without synchronization 3. Cause data races when one goroutine reads LastSeen while another updates it This fix adds a sync.RWMutex to each WireGuardSession struct to protect concurrent access to its fields. All field access now goes through thread-safe methods that properly acquire/release the mutex. Changes: - Added sync.RWMutex to WireGuardSession struct - Added thread-safe accessor methods (GetLastSeen, GetDestAddr, etc.) - Updated all session field accesses to use thread-safe methods - Removed redundant Store call after updating LastSeen (pointer update is atomic in Go, but field access within pointer was not) ## How to test? ### Understanding the Race Condition The race condition occurs in these scenarios: 1. **Cleanup goroutine reads `LastSeen`**: The `cleanupIdleSessions()` function periodically reads `session.LastSeen` to check if sessions should be removed. 2. **Packet handler updates `LastSeen`**: The `handleWireGuardPacket()` function updates `session.LastSeen = time.Now()` when processing transport data packets. 3. **Both happen concurrently**: When both operations occur simultaneously on the same session pointer, the race detector detects unsynchronized access to the `LastSeen` field. ## Notes An easier fix could be here but I didnt want to change the type against the sync.map in case a pointer was chosen for a reason, _in short_ instead of storing a pointer, storing the struct itself and when updating the any values just replace the key everytime (which you was doing when updating last seen, but since it was a pointer that was unnecessary). May cause some extra garbage collection but in this case cause the struct is tiny would be not noticeable. let me know if you rather have atomic updates against the sync.map instead of locks on the struct. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

GiteaMirror added the pull-request label 2025-11-19 07:03:47 -06:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/gerbil#35