Repository: kubernetes/klog (go)

Issue:
segfault for nil fmt.Stringer
/kind bug

**What steps did you take and what happened:**

Unit tests in Kubernetes segfault (https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/107103/pull-kubernetes-unit/1471877384482328576/build-log.txt):

```
testing.tRunner.func1.2({0x517fd60, 0x8aa1f60})
	/usr/local/go/src/testing/testing.go:1209 +0x36c
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1212 +0x3b6
panic({0x517fd60, 0x8aa1f60})
	/usr/local/go/src/runtime/panic.go:1047 +0x266
k8s.io/kubernetes/pkg/kubelet/container.(*RuntimeCondition).String(0x0)
	/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/container/runtime.go:528 +0x59
k8s.io/kubernetes/vendor/k8s.io/klog/v2.kvListFormat(0xc0000251f0, {0xc000c08620, 0x2, 0x5925698})
	/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:850 +0x453
```

**What did you expect to happen:**

We probably need to emulate what `fmt.Sprintf("%q", <nil stringer>)` did - whatever that is.


Pull request description:
Recover from nil pointers when logging
we used to rely on `fmt.Sprintf` which was handling the nil pointers for us. With the latest changes we avoid using `fmt.Sprintf` for speed/performance, So let's use a `recover` to survive the `panic`.

Signed-off-by: Davanum Srinivas <davanum@gmail.com>

<!--  Thanks for sending a pull request!  Here are some tips for you:
1. If this is your first time, read our contributor guidelines https://git.k8s.io/community/contributors/guide/pull-requests.md#the-pull-request-submit-process and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
3. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
4. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
5. If this PR changes image versions, please title this PR "Bump <image name> from x.x.x to y.y.y."
-->

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #278

**Special notes for your reviewer**:

_Please confirm that if this PR changes any image versions, then that's the sole change this PR makes._

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
fixes a problem handling nil pointers when logging
```

Patch:
diff --git a/klog.go b/klog.go
index 6b842d8..dacdf91 100644
--- a/klog.go
+++ b/klog.go
@@ -847,7 +847,7 @@ func kvListFormat(b *bytes.Buffer, keysAndValues ...interface{}) {
 		// (https://github.com/kubernetes/kubernetes/pull/106594#issuecomment-975526235).
 		switch v := v.(type) {
 		case fmt.Stringer:
-			writeStringValue(b, true, v.String())
+			writeStringValue(b, true, stringerToString(v))
 		case string:
 			writeStringValue(b, true, v)
 		case error:
@@ -872,6 +872,16 @@ func kvListFormat(b *bytes.Buffer, keysAndValues ...interface{}) {
 	}
 }
 
+func stringerToString(s fmt.Stringer) (ret string) {
+	defer func() {
+		if err := recover(); err != nil {
+			ret = "nil"
+		}
+	}()
+	ret = s.String()
+	return
+}
+
 func writeStringValue(b *bytes.Buffer, quote bool, v string) {
 	data := []byte(v)
 	index := bytes.IndexByte(data, '\n')
diff --git a/klog_file.go b/klog_file.go
index 1025d64..de830d9 100644
--- a/klog_file.go
+++ b/klog_file.go
@@ -22,7 +22,9 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"os/user"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"sync"
 	"time"
@@ -55,6 +57,38 @@ func init() {
 	}
 }
 
+func getUserName() string {
+	userNameOnce.Do(func() {
+		// On Windows, the Go 'user' package requires netapi32.dll.
+		// This affects Windows Nano Server:
+		//   https://github.com/golang/go/issues/21867
+		// Fallback to using environment variables.
+		if runtime.GOOS == "windows" {
+			u := os.Getenv("USERNAME")
+			if len(u) == 0 {
+				return
+			}
+			// Sanitize the USERNAME since it may contain filepath separators.
+			u = strings.Replace(u, `\`, "_", -1)
+
+			// user.Current().Username normally produces something like 'USERDOMAIN\USERNAME'
+			d := os.Getenv("USERDOMAIN")
+			if len(d) != 0 {
+				userName = d + "_" + u
+			} else {
+				userName = u
+			}
+		} else {
+			current, err := user.Current()
+			if err == nil {
+				userName = current.Username
+			}
+		}
+	})
+
+	return userName
+}
+
 // shortHostname returns its argument, truncating at the first period.
 // For instance, given "www.google.com" it returns "www".
 func shortHostname(hostname string) string {
diff --git a/klog_file_others.go b/klog_file_others.go
deleted file mode 100644
index aa46726..0000000
--- a/klog_file_others.go
+++ /dev/null
@@ -1,19 +0,0 @@
-//go:build !windows
-// +build !windows
-
-package klog
-
-import (
-	"os/user"
-)
-
-func getUserName() string {
-	userNameOnce.Do(func() {
-		current, err := user.Current()
-		if err == nil {
-			userName = current.Username
-		}
-	})
-
-	return userName
-}
diff --git a/klog_file_windows.go b/klog_file_windows.go
deleted file mode 100644
index 2517f9c..0000000
--- a/klog_file_windows.go
+++ /dev/null
@@ -1,34 +0,0 @@
-//go:build windows
-// +build windows
-
-package klog
-
-import (
-	"os"
-	"strings"
-)
-
-func getUserName() string {
-	userNameOnce.Do(func() {
-		// On Windows, the Go 'user' package requires netapi32.dll.
-		// This affects Windows Nano Server:
-		//   https://github.com/golang/go/issues/21867
-		// Fallback to using environment variables.
-		u := os.Getenv("USERNAME")
-		if len(u) == 0 {
-			return
-		}
-		// Sanitize the USERNAME since it may contain filepath separators.
-		u = strings.Replace(u, `\`, "_", -1)
-
-		// user.Current().Username normally produces something like 'USERDOMAIN\USERNAME'
-		d := os.Getenv("USERDOMAIN")
-		if len(d) != 0 {
-			userName = d + "_" + u
-		} else {
-			userName = u
-		}
-	})
-
-	return userName
-}


Test patch:
diff --git a/klog_test.go b/klog_test.go
index a89968a..a371845 100644
--- a/klog_test.go
+++ b/klog_test.go
@@ -1063,8 +1063,20 @@ func TestErrorS(t *testing.T) {
 	}
 }
 
+// point conforms to fmt.Stringer interface as it implements the String() method
+type point struct {
+	x int
+	y int
+}
+
+// we now have a value receiver
+func (p point) String() string {
+	return fmt.Sprintf("x=%d, y=%d", p.x, p.y)
+}
+
 // Test that kvListFormat works as advertised.
 func TestKvListFormat(t *testing.T) {
+	var emptyPoint *point
 	var testKVList = []struct {
 		keysValues []interface{}
 		want       string
@@ -1159,6 +1171,10 @@ No whitespace.`,
 			})},
 			want: " pods=[kube-system/kube-dns mi-conf]",
 		},
+		{
+			keysValues: []interface{}{"point-1", point{100, 200}, "point-2", emptyPoint},
+			want:       " point-1=\"x=100, y=200\" point-2=\"nil\"",
+		},
 	}
 
 	for _, d := range testKVList {