From 459cf3b8151dcfd8aa971077774eaf0c804119e4 Mon Sep 17 00:00:00 2001
From: chressie <chressie@golang.org>
Date: Mon, 4 Nov 2024 10:58:55 +0100
Subject: [PATCH] glog: check that stderr is valid before using it by default
 (#72)

Windows Services are [spawned without `stdout` and `stderr`](https://learn.microsoft.com/en-us/windows/console/getstdhandle#return-value:~:text=If%20an%20application%20does%20not%20have%20associated%20standard%20handles%2C%20such%20as%20a%20service%20running%20on%20an%20interactive%20desktop%2C%20and%20has%20not%20redirected%20them%2C%20the%20return%20value%20is%20NULL.), so any attempt to use them equates to referencing an invalid file `Handle`. `stderrSink` returns the error, which makes `logsink` trigger the termination of the process. The result is that any call to `log.Error` (or `log.Fatal`) from a Windows Service will always crash the program.

This approach checks that `os.Stderr`'s embedded file descriptor (`Handle`, for Windows) is non-NULL once during `init` to determine its validity and if it fails, then never registers `stderrSink` as a logsink.

Disabling based upon whether `os.Stderr` is NULL was chosen because it's precise, doesn't actually influence the result of any output and doesn't require any syscalls, but still detects the case in which `stderr` is unavailable on Windows.

A rejected approach here would've been to swallow the errors from `stderrSink.Emit`, thus sparing us program termination at the hands of `logsink`. I rejected this approach because it felt like it could result in some subtle behavioral changes. For example, invoking `log.Error` after `os.Stderr.Close()` would no longer exit the program and that felt like a non-trivial change (although I can think of no reason why one would desire this behavior).

cl/680817112 (google-internal)
---
 glog_file.go            |  5 ++++-
 glog_file_nonwindows.go |  7 +++++++
 glog_file_windows.go    | 13 +++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/glog_file.go b/glog_file.go
index 8eb8b08c..2b478ae6 100644
--- a/glog_file.go
+++ b/glog_file.go
@@ -158,7 +158,10 @@ var sinks struct {
 func init() {
 	// Register stderr first: that way if we crash during file-writing at least
 	// the log will have gone somewhere.
-	logsink.TextSinks = append(logsink.TextSinks, &sinks.stderr, &sinks.file)
+	if shouldRegisterStderrSink() {
+		logsink.TextSinks = append(logsink.TextSinks, &sinks.stderr)
+	}
+	logsink.TextSinks = append(logsink.TextSinks, &sinks.file)
 
 	sinks.file.flushChan = make(chan logsink.Severity, 1)
 	go sinks.file.flushDaemon()
diff --git a/glog_file_nonwindows.go b/glog_file_nonwindows.go
index d5cdb793..a0089ba4 100644
--- a/glog_file_nonwindows.go
+++ b/glog_file_nonwindows.go
@@ -4,6 +4,13 @@ package glog
 
 import "os/user"
 
+// shouldRegisterStderrSink determines whether we should register a log sink that writes to stderr.
+// Today, this always returns true on non-Windows platforms, as it specifically checks for a
+// condition that is only present on Windows.
+func shouldRegisterStderrSink() bool {
+	return true
+}
+
 func lookupUser() string {
 	if current, err := user.Current(); err == nil {
 		return current.Username
diff --git a/glog_file_windows.go b/glog_file_windows.go
index a9e4f609..2f032e19 100644
--- a/glog_file_windows.go
+++ b/glog_file_windows.go
@@ -3,9 +3,22 @@
 package glog
 
 import (
+	"os"
 	"syscall"
 )
 
+// shouldRegisterStderrSink determines whether we should register a log sink that writes to stderr.
+// Today, this checks if stderr is "valid", in that it maps to a non-NULL Handle.
+// Windows Services are spawned without Stdout and Stderr, so any attempt to use them equates to
+// referencing an invalid file Handle.
+// os.Stderr's FD is derived from a call to `syscall.GetStdHandle(syscall.STD_ERROR_HANDLE)`.
+// Documentation[1] for the GetStdHandle function indicates the return value may be NULL if the
+// application lacks the standard handle, so consider Stderr valid if its FD is non-NULL.
+// [1]: https://learn.microsoft.com/en-us/windows/console/getstdhandle
+func shouldRegisterStderrSink() bool {
+	return os.Stderr.Fd() != 0
+}
+
 // This follows the logic in the standard library's user.Current() function, except
 // that it leaves out the potentially expensive calls required to look up the user's
 // display name in Active Directory.