fix: panic caused by nil Writer provided to Logger

log/logger.go

@@ -58,20 +58,44 @@ type Logger struct {
 
 var pid = os.Getpid()
 
-// NewLogger creates a new Logger.
-func NewLogger(name string, level int, target int, logDir string) *Logger {
-	var logger Logger
+// NewLoggerE creates a new Logger and surfaces any errors encountered during
+// the process. The returned logger is guaranteed to be safe to use when a
+// non-nil error is returned, but may have undesired behavior. Callers should
+// treat the logger as nil under error conditions unless necessary for
+// backwards compatibility reasons.
+func NewLoggerE(name string, level, target int, logDir string) (*Logger, error) {
+	logger := &Logger{
+		l:            log.New(io.Discard, logPrefix, log.LstdFlags),
+		name:         name,
+		level:        level,
+		directory:    logDir,
+		maxFileSize:  maxLogFileSize,
+		maxFileCount: maxLogFileCount,
+		mutex:        &sync.Mutex{},
+	}
 
-	logger.l = log.New(nil, logPrefix, log.LstdFlags)
-	logger.name = name
-	logger.level = level
-	logger.directory = logDir
-	logger.SetTarget(target)
-	logger.maxFileSize = maxLogFileSize
-	logger.maxFileCount = maxLogFileCount
-	logger.mutex = &sync.Mutex{}
+	err := logger.SetTarget(target)
+	if err != nil {
+		// we *do* want to return the logger here for backwards compatibility
+		return logger, fmt.Errorf("setting log target: %w", err)
+	}
+	return logger, nil
+}
+
+// NewLogger creates a new Logger.
+//
+// Deprecated: use NewLoggerE instead
+func NewLogger(name string, level, target int, logDir string) *Logger {
+	logger, err := NewLoggerE(name, level, target, logDir)
+	if err != nil {
+		// ideally this would be returned to the caller, but this API is depended
+		// on by unknown parties. Given at this point we have an unusable (but
+		// safe) logger, we log to stderr with the standard library in hopes that
+		// an operator will see the error and be able to take corrective action
+		log.Println("error initializing logger: err:", err)
+	}
 
-	return &logger
+	return logger
 }
 
 // SetName sets the log name.

log/logger_test.go

@@ -6,6 +6,7 @@ package log
 import (
 	"fmt"
 	"os"
+	"path"
 	"strings"
 	"testing"
 )
@@ -14,6 +15,33 @@ const (
 	logName = "test"
 )
 
+func TestNewLoggerError(t *testing.T) {
+	// we expect an error from NewLoggerE in the event that we provide an
+	// unwriteable directory
+
+	// this test needs a guaranteed empty directory, so we create a temporary one
+	// and ensure that it gets destroyed afterward.
+	targetDir, err := os.MkdirTemp("", "acn")
+	if err != nil {
+		t.Fatal("unable to create temporary directory: err:", err)
+	}
+
+	t.Cleanup(func() {
+		// This removal could produce an error, but since it's a temporary
+		// directory anyway, this is a best-effort cleanup
+		os.Remove(targetDir)
+	})
+
+	// if we just use the targetDir, NewLoggerE will create the file and it will
+	// work. We need a non-existent directory *within* the tempdir
+	fullPath := path.Join(targetDir, "definitelyDoesNotExist")
+
+	_, err = NewLoggerE(logName, LevelInfo, TargetLogfile, fullPath)
+	if err == nil {
+		t.Error("expected an error but did not receive one")
+	}
+}
+
 // Tests that the log file rotates when size limit is reached.
 func TestLogFileRotatesWhenSizeLimitIsReached(t *testing.T) {
 	logDirectory := "" // This sets the current location for logs