fix: guard nil DefaultLogger.Out in moduleReload teardown (#846)#849
Open
SAY-5 wants to merge 1 commit into
Open
fix: guard nil DefaultLogger.Out in moduleReload teardown (#846)#849SAY-5 wants to merge 1 commit into
SAY-5 wants to merge 1 commit into
Conversation
When the configuration has no top-level 'log' directive, defaultLogOutput returns (nil, nil) and the container's DefaultLogger.Out is left unset. The async teardown goroutine in moduleReload called Out.Close() unconditionally, so 'systemctl reload maddy' (SIGUSR2) panicked and the daemon exited with status=2/INVALIDARGUMENT, dropping the mail service. Mirror the nil-check used by the synchronous shutdown path in moduleMain by extracting the close into a small helper and calling it from the reload teardown goroutine. Add unit tests covering the nil-Out, non-nil, Close-error, and nil-container code paths. Fixes foxcpp#846.
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
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 #846.
systemctl reload maddy(SIGUSR2) crashes the daemon with a nil pointer dereference in the async teardown goroutine ofmoduleReload. The new server has already bound listeners, but the panicking goroutine takes the whole process down with it, leaving the host with no mail service running untilsystemctl restart maddyis issued.Root cause: when the configuration has no top-level
logdirective,defaultLogOutputreturns(nil, nil)andc.DefaultLogger.Outis left unset. ThemoduleMainshutdown path atmaddy.go:452already nil-checks before callingClose(); the reload teardown goroutine added in 5841e95 (log: Refactor to define proper loggers tree) did not, so the close atmaddy.go:520panics.Fix: factor the close into a small
closeContainerLogOutputhelper that mirrors the existing nil-safe shutdown pattern, and call it from the reload teardown goroutine.Reproducer (boots a container with no
logdirective and runs the sameOut.Close()call that the teardown goroutine made):Tests:
TestCloseContainerLogOutput_NilOut— exercises the bug condition; before this change the equivalent inlineOut.Close()panicked.TestCloseContainerLogOutput_NonNil— happy path is unchanged.TestCloseContainerLogOutput_CloseError— Close errors are still surfaced through the new container's logger.TestCloseContainerLogOutput_NilContainer— defensive coverage for nil container / nil logger.go test -race .andgofmt -l .are clean.