From 5b139da83f428788edff2fbb16952551cf877e72 Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Tue, 15 Feb 2022 12:57:24 +0100
Subject: [PATCH] handle panics in MarshalLog, Error, String

The previous fix only covered fmt.Stringer.String in klog, but not klogr.
error.Error and logr.Marshaler.MarshalLog have the same problem. The
replacement string now captures the error, which makes it consistent with
https://github.com/go-logr/logr/pull/130.
---
 internal/serialize/keyvalues.go      | 22 +++++++++++++++++----
 internal/serialize/keyvalues_test.go |  2 +-
 klogr/klogr.go                       |  2 +-
 test/output.go                       | 29 ++++++++++++++++++++++++++++
 4 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/internal/serialize/keyvalues.go b/internal/serialize/keyvalues.go
index 4f583912f..d89731368 100644
--- a/internal/serialize/keyvalues.go
+++ b/internal/serialize/keyvalues.go
@@ -126,11 +126,11 @@ 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, stringerToString(v))
+			writeStringValue(b, true, StringerToString(v))
 		case string:
 			writeStringValue(b, true, v)
 		case error:
-			writeStringValue(b, true, v.Error())
+			writeStringValue(b, true, ErrorToString(v))
 		case []byte:
 			// In https://github.com/kubernetes/klog/pull/237 it was decided
 			// to format byte slices with "%+q". The advantages of that are:
@@ -151,16 +151,30 @@ func KVListFormat(b *bytes.Buffer, keysAndValues ...interface{}) {
 	}
 }
 
-func stringerToString(s fmt.Stringer) (ret string) {
+// StringerToString converts a Stringer to a string,
+// handling panics if they occur.
+func StringerToString(s fmt.Stringer) (ret string) {
 	defer func() {
 		if err := recover(); err != nil {
-			ret = "nil"
+			ret = fmt.Sprintf("<panic: %s>", err)
 		}
 	}()
 	ret = s.String()
 	return
 }
 
+// ErrorToString converts an error to a string,
+// handling panics if they occur.
+func ErrorToString(err error) (ret string) {
+	defer func() {
+		if err := recover(); err != nil {
+			ret = fmt.Sprintf("<panic: %s>", err)
+		}
+	}()
+	ret = err.Error()
+	return
+}
+
 func writeStringValue(b *bytes.Buffer, quote bool, v string) {
 	data := []byte(v)
 	index := bytes.IndexByte(data, '\n')
diff --git a/internal/serialize/keyvalues_test.go b/internal/serialize/keyvalues_test.go
index f7fdaba36..affb887ec 100644
--- a/internal/serialize/keyvalues_test.go
+++ b/internal/serialize/keyvalues_test.go
@@ -137,7 +137,7 @@ No whitespace.`,
 		},
 		{
 			keysValues: []interface{}{"point-1", point{100, 200}, "point-2", emptyPoint},
-			want:       " point-1=\"x=100, y=200\" point-2=\"nil\"",
+			want:       " point-1=\"x=100, y=200\" point-2=\"<panic: value method k8s.io/klog/v2/internal/serialize_test.point.String called using nil *point pointer>\"",
 		},
 	}
 
diff --git a/klogr/klogr.go b/klogr/klogr.go
index 8206c10d4..48cb65815 100644
--- a/klogr/klogr.go
+++ b/klogr/klogr.go
@@ -140,7 +140,7 @@ func (l klogger) Error(err error, msg string, kvList ...interface{}) {
 	msgStr := flatten("msg", msg)
 	var loggableErr interface{}
 	if err != nil {
-		loggableErr = err.Error()
+		loggableErr = serialize.ErrorToString(err)
 	}
 	switch l.format {
 	case FormatSerialize:
diff --git a/test/output.go b/test/output.go
index 0c447a827..6a6cee9ea 100644
--- a/test/output.go
+++ b/test/output.go
@@ -286,6 +286,24 @@ I output.go:<LINE>] "test" firstKey=1 secondKey=3
 			text: "test",
 			err:  errors.New("whoops"),
 			expectedOutput: `E output.go:<LINE>] "test" err="whoops"
+`,
+		},
+		"Error() that panics": {
+			text: "error panic",
+			err:  (*customErrorJSON)(nil),
+			expectedOutput: `E output.go:<LINE>] "error panic" err="<panic: runtime error: invalid memory address or nil pointer dereference>"
+`,
+		},
+		"String() that panics": {
+			text:   "stringer panic",
+			values: []interface{}{"stringer", (*stringer)(nil)},
+			expectedOutput: `I output.go:<LINE>] "stringer panic" stringer="<panic: runtime error: invalid memory address or nil pointer dereference>"
+`,
+		},
+		"MarshalLog that panics": {
+			text:   "marshaler panic",
+			values: []interface{}{"obj", (*klog.ObjectRef)(nil)},
+			expectedOutput: `I output.go:<LINE>] "marshaler panic" obj="<panic: value method k8s.io/klog/v2.ObjectRef.String called using nil *ObjectRef pointer>"
 `,
 		},
 	}
@@ -699,3 +717,14 @@ func (e *customErrorJSON) Error() string {
 func (e *customErrorJSON) MarshalJSON() ([]byte, error) {
 	return json.Marshal(strings.ToUpper(e.s))
 }
+
+type stringer struct {
+	s string
+}
+
+// String crashes when called for nil.
+func (s *stringer) String() string {
+	return s.s
+}
+
+var _ fmt.Stringer = &stringer{}