Commit f3615b5
committed
Fix: Unsynchronized staging matcher access races during reload
=== Classification
- Type: race condition
- Severity: high
- Confidence: certain
=== Affected Locations
- `dnscrypt-proxy/plugin_cloak.go:167`
- `dnscrypt-proxy/plugin_cloak.go:191`
- `dnscrypt-proxy/plugin_cloak.go:198`
=== Summary
`PluginCloak.stagingMatcher` was accessed from reload paths without consistent locking. `PrepareReload()` created and populated a staged matcher without holding the mutex, `ApplyReload()` consumed it under lock, and `CancelReload()` cleared it without lock. Concurrent reload, apply, or cancel operations on the same instance could therefore race and corrupt the staged-to-active handoff.
=== Provenance
The finding was reproduced from a verified report derived from Swival Security Scanner (https://swival.dev) output and confirmed locally with the Go race detector.
=== Preconditions
- Concurrent reload operations on one `PluginCloak` instance
=== Proof
- `PrepareReload()` assigned `plugin.stagingMatcher = NewPatternMatcher()` and filled it without synchronization in `dnscrypt-proxy/plugin_cloak.go:167`.
- `ApplyReload()` read and swapped `stagingMatcher` while holding `plugin.Lock()` in `dnscrypt-proxy/plugin_cloak.go:191`.
- `CancelReload()` wrote `plugin.stagingMatcher = nil` without locking in `dnscrypt-proxy/plugin_cloak.go:198`.
- A temporary reproducer concurrently invoked `plugin.Reload()` on one `PluginCloak`.
- Running `go test -race ./dnscrypt-proxy -run TestPluginCloakConcurrentReloadRace -count=1` reported:
- write/write race in `PrepareReload()`
- read/write race involving `ApplyReload()` and staged field clearing
- Observed impact matched the report: one goroutine could apply another goroutine's staged matcher or see `nil`, causing lost updates or spurious `no staged configuration to apply` failures.
=== Why This Is A Real Bug
This is not a theoretical race. The field participates directly in configuration state transfer between reload phases, and the race detector reported conflicting accesses under a realistic concurrent reload scenario. Because the staged matcher can be overwritten or cleared mid-handoff, reload behavior becomes non-atomic and can apply the wrong configuration or fail unexpectedly.
=== Fix Requirement
Guard all reads and writes of `stagingMatcher` with the same mutex.
=== Patch Rationale
The patch serializes every `stagingMatcher` access through `PluginCloak`'s mutex so staging, apply, and cancel all observe a consistent reload state. This preserves the intended staged-to-active handoff and removes the conflicting unsynchronized accesses identified by the reproducer.
=== Residual Risk
None
=== Patch
- Patched in `006-unsynchronized-staging-matcher-access-races-during-reload.patch`
- The patch wraps `stagingMatcher` mutation and consumption in `dnscrypt-proxy/plugin_cloak.go` with the existing mutex, aligning `PrepareReload()`, `ApplyReload()`, and `CancelReload()` on one synchronization mechanism.1 parent 2d7227c commit f3615b5
1 file changed
+11
-6
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
175 | 175 | | |
176 | 176 | | |
177 | 177 | | |
178 | | - | |
179 | | - | |
| 178 | + | |
180 | 179 | | |
181 | 180 | | |
182 | | - | |
| 181 | + | |
183 | 182 | | |
184 | 183 | | |
185 | 184 | | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
186 | 189 | | |
187 | 190 | | |
188 | 191 | | |
189 | 192 | | |
190 | 193 | | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
191 | 197 | | |
192 | 198 | | |
193 | 199 | | |
194 | 200 | | |
195 | | - | |
196 | | - | |
197 | 201 | | |
198 | 202 | | |
199 | | - | |
200 | 203 | | |
201 | 204 | | |
202 | 205 | | |
203 | 206 | | |
204 | 207 | | |
205 | 208 | | |
206 | 209 | | |
| 210 | + | |
207 | 211 | | |
| 212 | + | |
208 | 213 | | |
209 | 214 | | |
210 | 215 | | |
| |||
0 commit comments