Change os.Stderr reassign for Windows service

dcantah · dcantah · commit 0578d20c5db1 · 2022-08-02T05:50:31.000-07:00
Previously we were reassigning os.Stderr to the panic.log file we create when getting asked to run Containerd as a Windows service. The panic.log file was used as a means to easily collect panic stacks as Windows services don't have regular standard IO, and the usual recommendation is to either write to the event log or just to a file in the case of running as a service. One place where this panic.log flow was biting us was with shim logging, which is forwarded from the shim and copied to os.Stderr directly which was causing shim logs to get forwarded to this panic.log file instead of just panics. We expose an additional `--log-file` flag if you ask to run a windows service which is the main way you'd get Containerd logs, and with this change all of the shim logging which would today end up in panic.log will now also go to this log file. Signed-off-by: Daniel Canter <dcanter@microsoft.com> (cherry picked from commit ee0f2e9) Signed-off-by: Daniel Canter <dcanter@microsoft.com>
diff --git a/cmd/containerd/command/service_windows.go b/cmd/containerd/command/service_windows.go
@@ -18,7 +18,6 @@ package command
 
 import (
 	"fmt"
-	"io"
 	"log"
 	"os"
 	"path/filepath"
@@ -214,7 +213,6 @@ func unregisterService() error {
 // to handle (un-)registering against Windows Service Control Manager (SCM).
 // It returns an indication to stop on successful SCM operation, and an error.
 func registerUnregisterService(root string) (bool, error) {
-
 	if unregisterServiceFlag {
 		if registerServiceFlag {
 			return true, fmt.Errorf("--register-service and --unregister-service cannot be used together: %w", errdefs.ErrInvalidArgument)
@@ -248,22 +246,57 @@ func registerUnregisterService(root string) (bool, error) {
 			return true, err
 		}
 
-		logOutput := io.Discard
+		// The usual advice for Windows services is to either write to a log file or to the windows event
+		// log, the former of which we've exposed here via a --log-file flag. We additionally write panic
+		// stacks to a panic.log file to diagnose crashes. Below details the two different outcomes if
+		// --log-file is specified or not:
+		//
+		// --log-file is *not* specified.
+		// -------------------------------
+		// -logrus, the stdlibs logging package and os.Stderr output will go to
+		// NUL (Windows' /dev/null equivalent).
+		// -Panics will write their stack trace to the panic.log file.
+		// -Writing to the handle returned from GetStdHandle(STD_ERROR_HANDLE) will write
+		// to the panic.log file as the underlying handle itself has been redirected.
+		//
+		// --log-file *is* specified
+		// -------------------------------
+		// -Logging to logrus, the stdlibs logging package or directly to
+		// os.Stderr will all go to the log file specified.
+		// -Panics will write their stack trace to the panic.log file.
+		// -Writing to the handle returned from GetStdHandle(STD_ERROR_HANDLE) will write
+		// to the panic.log file as the underlying handle itself has been redirected.
+		var f *os.File
 		if logFileFlag != "" {
-			f, err := os.OpenFile(logFileFlag, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
+			f, err = os.OpenFile(logFileFlag, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
 			if err != nil {
 				return true, fmt.Errorf("open log file %q: %w", logFileFlag, err)
 			}
-			logOutput = f
+		} else {
+			// Windows services start with NULL stdio handles, and thus os.Stderr and friends will be
+			// backed by an os.File with a NULL handle. This means writes to os.Stderr will fail, which
+			// isn't a huge issue as we want output to be discarded if the user doesn't ask for the log
+			// file. However, writes succeeding but just going to the ether is a much better construct
+			// so use devnull instead of relying on writes failing. We use devnull instead of io.Discard
+			// as os.Stderr is an os.File and can't be assigned to io.Discard.
+			f, err = os.OpenFile(os.DevNull, os.O_WRONLY, 0)
+			if err != nil {
+				return true, err
+			}
 		}
-		logrus.SetOutput(logOutput)
+		// Reassign os.Stderr to the log file or NUL. Shim logs are copied to os.Stderr
+		// directly so this ensures those will end up in the log file as well if specified.
+		os.Stderr = f
+		// Assign the stdlibs log package in case of any miscellaneous uses by
+		// dependencies.
+		log.SetOutput(f)
+		logrus.SetOutput(f)
 	}
 	return false, nil
 }
 
 // launchService is the entry point for running the daemon under SCM.
 func launchService(s *server.Server, done chan struct{}) error {
-
 	if !runServiceFlag {
 		return nil
 	}
@@ -359,12 +392,6 @@ func initPanicFile(path string) error {
 		return err
 	}
 
-	// Reset os.Stderr to the panic file (so fmt.Fprintf(os.Stderr,...) actually gets redirected)
-	os.Stderr = os.NewFile(panicFile.Fd(), "/dev/stderr")
-
-	// Force threads that panic to write to stderr (the panicFile handle now), otherwise it will go into the ether
-	log.SetOutput(os.Stderr)
-
 	return nil
 }
 
