Skip to content

Commit

Permalink
glog: check that stderr is valid before using it by default (#72)
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
chressie authored Nov 4, 2024
1 parent 9730314 commit 459cf3b
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 1 deletion.
5 changes: 4 additions & 1 deletion glog_file.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
7 changes: 7 additions & 0 deletions glog_file_nonwindows.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions glog_file_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 459cf3b

Please sign in to comment.