fix(simpleradio): use atomic.Bool for secureCoalitionRadios#672
Merged
Conversation
Owner
|
Thank you for the patch! LGTM after the checks pass. |
The TCP receiver goroutine writes c.secureCoalitionRadios from
updateServerSettings while the UDP receiver goroutine reads it from
the voice-packet handling path, with no synchronization — a data race
the Go race detector flags and an unlucky interleaving could let a
voice packet skip the coalition filter while the flag is flipping.
Switch the field to sync/atomic.Bool and route the write/read through
Store/Load. The tri-state log message ('enabling secure coalition
radios') still fires only on transitions, via a Load() check before
Store(true).
Refs dharmab/skyeye issue 661.
Signed-off-by: SAY-5 <say.apm35@gmail.com>
dharmab
force-pushed
the
fix/secure-coalition-radios-race-661
branch
from
efa7194 to
1c2f5f8
Compare
Owner
|
It turns out CodeQL scanning just gets stuck sometimes for no reason https://github.com/orgs/community/discussions/159026 I've never actually taken any actionable insight from the CodeQL scanner anyway, so I'm just going to disable it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #661.
Problem
secureCoalitionRadioswas a plainboolwritten from the TCP receiver goroutine inupdateServerSettings(pkg/simpleradio/message.go:77-91) and read from the UDP receiver goroutine (pkg/simpleradio/receive.go:107) with no synchronization. The Go race detector flagged it, and an unlucky interleaving could let a voice packet skip the coalition filter while the flag was flipping.Fix
Switch the field to
sync/atomic.Booland route the write/read throughStore/Load. The one-shot "enabling secure coalition radios" transition log still fires only when the flag actually flips, via aLoad()check beforeStore(true).Test
gofmtclean.go test -racelocally fails to link because the opus/opusfile system packages aren't installed on my machine, but the race was surfaced in CI on the linked issue and this is the minimal correctness fix.