fix(syslog): don't close the shared acquisition output channel#4553
fix(syslog): don't close the shared acquisition output channel#4553alxrxs wants to merge 1 commit into
Conversation
|
@alxrxs: There are no 'kind' label on this PR. You need a 'kind' label to generate the release automatically.
DetailsI am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository. |
|
@alxrxs: There are no area labels on this PR. You can add as many areas as you see fit.
DetailsI am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository. |
|
/kind fix |
The syslog datasource's Stream() does `defer close(out)` on the pipeline
output channel. That channel is shared: StartAcquisition() passes the same
`out` to every configured datasource, and it is owned by the acquisition
orchestrator, which never closes it (shutdown is driven by context
cancellation, then drainChan). syslog is the only datasource that closes it.
When syslog and another datasource run together, syslog's close races with
the other datasource still sending. This is reproducible with the AppSec
datasource: on shutdown/reload, an in-flight out-of-band request in
appsec_runner's handleOutBandInterrupt does `r.outChan <- evt` after syslog
has closed the channel, panicking with "send on closed channel" (seen in
production on 1.7.8 under a syslog+appsec config).
Remove the close. No consumer relies on it: runParse ranges via a
`select { case <-ctx.Done(); case ev := <-input }` loop and drainChan is a
non-blocking best-effort drain, so both terminate on context cancellation
regardless. This aligns syslog with every other datasource (file, journalctl,
docker, ...), none of which close the shared channel. The "never close out"
contract is documented on the RestartableStreamer and Tailer interfaces, and
a regression test asserts Stream() leaves the channel open after teardown.
alxrxs
force-pushed
the
fix/syslog-close-shared-output-channel
branch
from
fc3bf43 to
dc023a2
Compare
1 participant
What
The syslog datasource is the only acquisition module that closes the shared pipeline output channel. Removing that
close(out)fixes asend on closed channelpanic that crashes the log processor on shutdown/reload when syslog runs alongside another datasource.The bug
StartAcquisition()passes the sameout chan pipeline.Eventto every configured datasource, and the acquisition orchestrator owns it — shutdown is driven by context cancellation (thendrainChan), and the orchestrator never closes the channel. Every datasource (file,journalctl,docker, …) treatsoutas send-only and leaves it open.syslogis the exception — inpkg/acquisition/modules/syslog/run.go:When syslog and another datasource are configured together, syslog's
close(out)on shutdown races with the other datasource still sending onout, and Go panics.This reliably reproduces with the AppSec datasource, which sends events on the shared channel from its asynchronous request handling (
appsec_runner.go, both the in-band and out-of-band paths, e.g.r.outChan <- evt). On asystemctl restart/reload while a request is still in flight, syslog closes the channel first and AppSec panics.Production stack trace (v1.7.8, a
syslog+appsecconfig)The process is
Restart=on-failure'd back up, but the AppSec listener can come back degraded (high CPU, climbing memory, WAF requests timing out) until a subsequent clean restart, so an operator-facing symptom is "AppSec intermittently stops responding after a restart."The fix
Remove the
close(out)from syslog. No consumer relies on it:runParsereads viaselect { case <-ctx.Done(): return; case ev := <-input: }— it terminates on context cancellation, not on channel close.drainChanis a non-blocking best-effort drain that handles both open and closed channels. (Its own comment already notes "We should close the chan on the writer side rather than this" — there is no single writer to do so, which is exactly why no datasource should.)This aligns syslog with every other datasource. I also documented the "never close
out" contract on both theRestartableStreamerandTailerinterfaces so it doesn't regress.Alternatives considered
recover()in AppSec's send path — papers over the symptom at one call site while leaving the channel-ownership violation (and the same latent race for any future multi-source combo) in place.sync.Once/ orchestrator-side close — the orchestrator has no single writer to coordinate a safe close, and it already shuts the pipeline down cleanly via context cancellation +drainChan, so a close is simply unnecessary.Closing a channel you don't own (with other senders live) is a textbook Go anti-pattern; "don't close what you don't own" is the minimal, root-cause fix.
Testing
TestStreamDoesNotCloseOutput(pkg/acquisition/modules/syslog/syslog_test.go): startsStream(), cancels the context to tear it down, and asserts the shared output channel is never observed closed. It passes on this change and fails on the pre-fix code (I verified both).go build ./...andgo vet ./pkg/acquisition/...pass; the new code isgofmt-clean.