Skip to content

fix: relay race condition in WireGuard session management#36

Merged
oschwartz10612 merged 3 commits intofosrl:mainfrom
LaurenceJJones:fix-wg-session-race-condition
Dec 6, 2025
Merged

fix: relay race condition in WireGuard session management#36
oschwartz10612 merged 3 commits intofosrl:mainfrom
LaurenceJJones:fix-wg-session-race-condition

Conversation

@LaurenceJJones
Copy link
Contributor

@LaurenceJJones LaurenceJJones commented Nov 13, 2025

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.

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.)
- Added atomic CheckAndUpdateIfMatch method for efficient check-and-update
- 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)
- Remove unused methods: UpdateLastSeen, GetSenderIndex, MatchesSenderIndex
  (replaced by simpler direct usage in Range callbacks)
- Simplify session access pattern: check GetSenderIndex in Range callback,
  then call GetDestAddr and UpdateLastSeen when match found
- Optimize UpdateLastSeen usage: only use for existing sessions already
  in sync.Map; use direct assignment in struct literals for new sessions
  (safe since no concurrent access during creation)

This simplifies the code while maintaining thread-safety for concurrent
access to existing sessions.
@oschwartz10612 oschwartz10612 merged commit 4cb2fde into fosrl:main Dec 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants