From feb9f7c6226c1f910a1394befe2fd35e0507284c Mon Sep 17 00:00:00 2001
From: Joa Ebert <jebert@make.tv>
Date: Wed, 30 Jan 2019 13:07:27 +0100
Subject: [PATCH] Don't panic when a Stringer field value is nil

This commit ensures that zap.Stringer won't cause a panic when the
given field value is nil or a typed nil[1]. In fact any panic
caused by the Stringer implementation would be recovered and
printed. For panic("foo") for a field k we would see kError=foo
within the log. A panic with an error would result in the error
message in kError. Other kinds of panic values are simply printed
as "<unknown>" since trying to convert the value itself could
cause a panic again which would trigger the panic within recover
panic.

[1]: https://golang.org/doc/faq#nil_error
---
 zapcore/field.go      | 17 +++++++++++++++
 zapcore/field_test.go | 48 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/zapcore/field.go b/zapcore/field.go
index 6a5e33e2f..1c571695e 100644
--- a/zapcore/field.go
+++ b/zapcore/field.go
@@ -160,6 +160,23 @@ func (f Field) AddTo(enc ObjectEncoder) {
 	case NamespaceType:
 		enc.OpenNamespace(f.Key)
 	case StringerType:
+		defer func() {
+			if r := recover(); r != nil {
+				var msg string
+
+				switch v := r.(type) {
+				case error:
+					msg = v.Error()
+				case string:
+					msg = v
+				default:
+					msg = "<unknown>"
+				}
+
+				enc.AddString(fmt.Sprintf("%sError", f.Key), msg)
+			}
+		}()
+
 		enc.AddString(f.Key, f.Interface.(fmt.Stringer).String())
 	case ErrorType:
 		encodeError(f.Key, f.Interface.(error), enc)
diff --git a/zapcore/field_test.go b/zapcore/field_test.go
index 9a5fe0189..d0b8b8d97 100644
--- a/zapcore/field_test.go
+++ b/zapcore/field_test.go
@@ -24,13 +24,12 @@ import (
 	"errors"
 	"fmt"
 	"math"
+	"net/url"
 	"testing"
 	"time"
 
-	"go.uber.org/zap"
-
 	"github.com/stretchr/testify/assert"
-
+	"go.uber.org/zap"
 	. "go.uber.org/zap/zapcore"
 )
 
@@ -58,6 +57,27 @@ func (u users) MarshalLogArray(enc ArrayEncoder) error {
 	return nil
 }
 
+type obj struct {
+	kind int
+}
+
+func (o *obj) String() string {
+	if o == nil {
+		return "nil obj"
+	}
+
+	if o.kind == 1 {
+		panic("panic with string")
+	} else if o.kind == 2 {
+		panic(errors.New("panic with error"))
+	} else if o.kind == 3 {
+		// recursive - causes a panic when trying to print the panic value
+		panic(o)
+	}
+
+	return "obj"
+}
+
 func TestUnknownFieldType(t *testing.T) {
 	unknown := Field{Key: "k", String: "foo"}
 	assert.Equal(t, UnknownType, unknown.Type, "Expected zero value of FieldType to be UnknownType.")
@@ -67,19 +87,27 @@ func TestUnknownFieldType(t *testing.T) {
 }
 
 func TestFieldAddingError(t *testing.T) {
+	var empty interface{}
 	tests := []struct {
-		t    FieldType
-		want interface{}
+		t     FieldType
+		iface interface{}
+		want  interface{}
+		err   string
 	}{
-		{ArrayMarshalerType, []interface{}{}},
-		{ObjectMarshalerType, map[string]interface{}{}},
+		{t: ArrayMarshalerType, iface: users(-1), want: []interface{}{}, err: "too few users"},
+		{t: ObjectMarshalerType, iface: users(-1), want: map[string]interface{}{}, err: "too few users"},
+		{t: StringerType, iface: obj{}, want: empty, err: "interface conversion: zapcore_test.obj is not fmt.Stringer: missing method String"},
+		{t: StringerType, iface: &obj{1}, want: empty, err: "panic with string"},
+		{t: StringerType, iface: &obj{2}, want: empty, err: "panic with error"},
+		{t: StringerType, iface: &obj{3}, want: empty, err: "<unknown>"},
+		{t: StringerType, iface: (*url.URL)(nil), want: empty, err: "runtime error: invalid memory address or nil pointer dereference"},
 	}
 	for _, tt := range tests {
-		f := Field{Key: "k", Interface: users(-1), Type: tt.t}
+		f := Field{Key: "k", Interface: tt.iface, Type: tt.t}
 		enc := NewMapObjectEncoder()
 		assert.NotPanics(t, func() { f.AddTo(enc) }, "Unexpected panic when adding fields returns an error.")
 		assert.Equal(t, tt.want, enc.Fields["k"], "On error, expected zero value in field.Key.")
-		assert.Equal(t, "too few users", enc.Fields["kError"], "Expected error message in log context.")
+		assert.Equal(t, tt.err, enc.Fields["kError"], "Expected error message in log context.")
 	}
 }
 
@@ -116,6 +144,8 @@ func TestFields(t *testing.T) {
 		{t: ReflectType, iface: users(2), want: users(2)},
 		{t: NamespaceType, want: map[string]interface{}{}},
 		{t: StringerType, iface: users(2), want: "2 users"},
+		{t: StringerType, iface: &obj{}, want: "obj"},
+		{t: StringerType, iface: (*obj)(nil), want: "nil obj"},
 		{t: SkipType, want: interface{}(nil)},
 	}