Skip to content

Commit 459cf3b

Browse files
authored
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)
1 parent 9730314 commit 459cf3b

File tree

3 files changed

+24
-1
lines changed

3 files changed

+24
-1
lines changed

glog_file.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,10 @@ var sinks struct {
158158
func init() {
159159
// Register stderr first: that way if we crash during file-writing at least
160160
// the log will have gone somewhere.
161-
logsink.TextSinks = append(logsink.TextSinks, &sinks.stderr, &sinks.file)
161+
if shouldRegisterStderrSink() {
162+
logsink.TextSinks = append(logsink.TextSinks, &sinks.stderr)
163+
}
164+
logsink.TextSinks = append(logsink.TextSinks, &sinks.file)
162165

163166
sinks.file.flushChan = make(chan logsink.Severity, 1)
164167
go sinks.file.flushDaemon()

glog_file_nonwindows.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,13 @@ package glog
44

55
import "os/user"
66

7+
// shouldRegisterStderrSink determines whether we should register a log sink that writes to stderr.
8+
// Today, this always returns true on non-Windows platforms, as it specifically checks for a
9+
// condition that is only present on Windows.
10+
func shouldRegisterStderrSink() bool {
11+
return true
12+
}
13+
714
func lookupUser() string {
815
if current, err := user.Current(); err == nil {
916
return current.Username

glog_file_windows.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,22 @@
33
package glog
44

55
import (
6+
"os"
67
"syscall"
78
)
89

10+
// shouldRegisterStderrSink determines whether we should register a log sink that writes to stderr.
11+
// Today, this checks if stderr is "valid", in that it maps to a non-NULL Handle.
12+
// Windows Services are spawned without Stdout and Stderr, so any attempt to use them equates to
13+
// referencing an invalid file Handle.
14+
// os.Stderr's FD is derived from a call to `syscall.GetStdHandle(syscall.STD_ERROR_HANDLE)`.
15+
// Documentation[1] for the GetStdHandle function indicates the return value may be NULL if the
16+
// application lacks the standard handle, so consider Stderr valid if its FD is non-NULL.
17+
// [1]: https://learn.microsoft.com/en-us/windows/console/getstdhandle
18+
func shouldRegisterStderrSink() bool {
19+
return os.Stderr.Fd() != 0
20+
}
21+
922
// This follows the logic in the standard library's user.Current() function, except
1023
// that it leaves out the potentially expensive calls required to look up the user's
1124
// display name in Active Directory.

0 commit comments

Comments
 (0)