Commit 5b078bb
committed
Fix: Reload staging map races with apply/cancel
=== Classification
Race condition in reload state handling. Severity: medium. Confidence: certain.
=== Affected Locations
- `dnscrypt-proxy/plugin_forward.go:205`
- `dnscrypt-proxy/plugin_forward.go:228`
- `dnscrypt-proxy/plugin_forward.go:235`
- `dnscrypt-proxy/plugin_forward.go:241`
- `dnscrypt-proxy/plugin_forward.go:242`
=== Summary
`PluginForward` stores staged reload data in `plugin.stagingMap` without consistently holding `rwLock`. `PrepareReload()` writes the staged map unlocked, `ApplyReload()` reads and nil-checks it before locking, and `CancelReload()` clears it unlocked. Concurrent reload sequences can therefore race on the shared slice header, causing spurious `no staged configuration to apply` failures or applying another reload's staged rules.
=== Provenance
Verified from a reproduced finding derived from Swival Security Scanner output at https://swival.dev, then confirmed with a local concurrent reload reproducer and Go race detector evidence.
=== Preconditions
Concurrent `PrepareReload`/`ApplyReload` or `PrepareReload`/`CancelReload` calls against the same `PluginForward` instance.
=== Proof
A temporary concurrent test ran two `plugin.Reload()` loops against one `PluginForward`.
`go test -race` reported concrete races on `plugin.stagingMap`, including:
- write/write at `dnscrypt-proxy/plugin_forward.go:228`
- write/read between `dnscrypt-proxy/plugin_forward.go:242` and `dnscrypt-proxy/plugin_forward.go:235`
- write/read between `dnscrypt-proxy/plugin_forward.go:228` and `dnscrypt-proxy/plugin_forward.go:241`
A non-race execution of the same reproducer also triggered real reload failures after sufficient iterations, including spurious `no staged configuration to apply` errors and cross-application of another reload's staged rules.
=== Why This Is A Real Bug
This is not a detector-only issue. The raced value is the live reload handoff state that decides whether a staged configuration exists and which rules become active. Unsynchronized access to the slice header permits lost, stale, or mismatched staging state across overlapping reloads, directly producing incorrect runtime behavior during reachable reload paths such as file-watcher and SIGHUP-triggered reloads.
=== Fix Requirement
Guard every read and write of `plugin.stagingMap`, including existence checks and nil assignment, with the same mutex used for reload state.
=== Patch Rationale
The patch serializes all `stagingMap` access under `rwLock` in `PrepareReload()`, `ApplyReload()`, and `CancelReload()`. Moving the nil check inside the lock ensures `ApplyReload()` observes a consistent staged value, and locking the clear/write paths prevents concurrent reload operations from racing on the shared slice header.
=== Residual Risk
None
=== Patch
Patch file: `013-reload-staging-map-races-with-apply-cancel.patch`1 parent 974b431 commit 5b078bb
1 file changed
+7
-3
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
225 | 225 | | |
226 | 226 | | |
227 | 227 | | |
| 228 | + | |
228 | 229 | | |
| 230 | + | |
229 | 231 | | |
230 | 232 | | |
231 | 233 | | |
232 | 234 | | |
233 | 235 | | |
234 | 236 | | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
235 | 240 | | |
236 | 241 | | |
237 | 242 | | |
238 | 243 | | |
239 | | - | |
240 | | - | |
241 | 244 | | |
242 | 245 | | |
243 | | - | |
244 | 246 | | |
245 | 247 | | |
246 | 248 | | |
247 | 249 | | |
248 | 250 | | |
249 | 251 | | |
250 | 252 | | |
| 253 | + | |
251 | 254 | | |
| 255 | + | |
252 | 256 | | |
253 | 257 | | |
254 | 258 | | |
| |||
0 commit comments