From 632908a992a7c01f960d8f90fb1f04059f616ee3 Mon Sep 17 00:00:00 2001 From: SAY-5 Date: Thu, 21 May 2026 02:34:55 -0700 Subject: [PATCH] fix: guard nil DefaultLogger.Out in moduleReload teardown 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 #846. --- maddy.go | 21 +++++++-- maddy_test.go | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 3 deletions(-) create mode 100644 maddy_test.go diff --git a/maddy.go b/maddy.go index ba5a95ac..4a1733cb 100644 --- a/maddy.go +++ b/maddy.go @@ -462,6 +462,23 @@ func moduleMain(configPath string) error { return nil } +// closeContainerLogOutput closes c.DefaultLogger.Out if it is set, mirroring +// the nil-safe behaviour of the moduleMain shutdown path. When the +// configuration has no `log` directive, defaultLogOutput returns (nil, nil) +// and c.DefaultLogger.Out is left unset; closing it unconditionally panics. +// errLogger receives any close failure (it must not be the logger of c, +// which is being torn down). +func closeContainerLogOutput(c *container.C, errLogger *log.Logger) { + if c == nil || c.DefaultLogger == nil || c.DefaultLogger.Out == nil { + return + } + if err := c.DefaultLogger.Out.Close(); err != nil { + if errLogger != nil { + errLogger.Error("failed to close old server log", err) + } + } +} + func moduleReload(oldContainer *container.C, configPath string, asyncStopWg *sync.WaitGroup) *container.C { oldContainer.DefaultLogger.Msg("reloading server...") systemdStatus(SDReloading, "Reloading server...") @@ -521,9 +538,7 @@ func moduleReload(oldContainer *container.C, configPath string, asyncStopWg *syn oldContainer.DefaultLogger.Error("moduleStop failed", err) } oldContainer.DefaultLogger.Msg("old server stopped") - if err := oldContainer.DefaultLogger.Out.Close(); err != nil { - newContainer.DefaultLogger.Error("failed to close old server log", err) - } + closeContainerLogOutput(oldContainer, newContainer.DefaultLogger) systemdStatus(SDReloading, "Configuration running.") }() diff --git a/maddy_test.go b/maddy_test.go new file mode 100644 index 00000000..8d0289bd --- /dev/null +++ b/maddy_test.go @@ -0,0 +1,122 @@ +/* +Maddy Mail Server - Composable all-in-one email server. +Copyright © 2019-2020 Max Mazurov , Maddy Mail Server contributors + +This program is free software: you can redistribute it and/or modify +it under the terms of the GNU General Public License as published by +the Free Software Foundation, either version 3 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU General Public License for more details. + +You should have received a copy of the GNU General Public License +along with this program. If not, see . +*/ + +package maddy + +import ( + "errors" + "testing" + "time" + + "github.com/foxcpp/maddy/framework/container" + "github.com/foxcpp/maddy/framework/log" +) + +// trackingOutput is a log.Output that records whether Close was called. +type trackingOutput struct { + closed bool + closeErr error +} + +func (o *trackingOutput) Write(_ time.Time, _ bool, _ string) {} +func (o *trackingOutput) Close() error { + o.closed = true + return o.closeErr +} + +func newErrLogger() (*log.Logger, *trackingOutput) { + out := &trackingOutput{} + return &log.Logger{Out: out}, out +} + +// TestCloseContainerLogOutput_NilOut verifies that closeContainerLogOutput +// does not panic when DefaultLogger.Out is nil, which is the configuration +// produced by defaultLogOutput when the user's maddy.conf has no `log` +// directive. Without the nil guard, SIGUSR2 reload panicked in the teardown +// goroutine of moduleReload and the daemon died (foxcpp/maddy#846). +func TestCloseContainerLogOutput_NilOut(t *testing.T) { + c := &container.C{ + DefaultLogger: &log.Logger{}, // Out is the zero value (nil) + } + errLog, errOut := newErrLogger() + + defer func() { + if r := recover(); r != nil { + t.Fatalf("closeContainerLogOutput panicked on nil Out: %v", r) + } + }() + closeContainerLogOutput(c, errLog) + + if errOut.closed { + t.Fatalf("error logger output should not be closed by the helper") + } +} + +// TestCloseContainerLogOutput_NonNil verifies that a non-nil Out is closed +// exactly once, mirroring the previous unconditional behaviour on the +// happy path. +func TestCloseContainerLogOutput_NonNil(t *testing.T) { + out := &trackingOutput{} + c := &container.C{ + DefaultLogger: &log.Logger{Out: out}, + } + errLog, _ := newErrLogger() + + closeContainerLogOutput(c, errLog) + if !out.closed { + t.Fatalf("expected old container log output to be closed") + } +} + +// TestCloseContainerLogOutput_CloseError verifies that an error returned +// from the underlying Close is reported through errLogger and does not +// propagate as a panic. +func TestCloseContainerLogOutput_CloseError(t *testing.T) { + out := &trackingOutput{closeErr: errors.New("disk full")} + c := &container.C{ + DefaultLogger: &log.Logger{Out: out}, + } + errLog, _ := newErrLogger() + + defer func() { + if r := recover(); r != nil { + t.Fatalf("closeContainerLogOutput panicked: %v", r) + } + }() + closeContainerLogOutput(c, errLog) + if !out.closed { + t.Fatalf("expected Close to be invoked even when it returns an error") + } +} + +// TestCloseContainerLogOutput_NilContainer verifies the helper tolerates a +// nil container / nil DefaultLogger, so callers do not need to add extra +// nil checks of their own. +func TestCloseContainerLogOutput_NilContainer(t *testing.T) { + errLog, _ := newErrLogger() + + defer func() { + if r := recover(); r != nil { + t.Fatalf("closeContainerLogOutput panicked on nil container: %v", r) + } + }() + closeContainerLogOutput(nil, errLog) + + c := &container.C{} // DefaultLogger is nil + closeContainerLogOutput(c, errLog) +}