From d00360119d1bc06208a134b2546039476e217b1b Mon Sep 17 00:00:00 2001
From: LiuYang <kfliuyang@gmail.com>
Date: Wed, 25 Nov 2020 03:15:17 +0800
Subject: [PATCH] field/error: Handle panic in Error() (#867)

We shouldn't panic if the `Error()` method of an `error` panics.

```
type T struct{ msg string }

func (t *T) Error() string { return t.msg }

// The following panics.
var n *T = nil
log.Info("panic", zap.Error(n))
```

Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
---
 zapcore/error.go      | 19 ++++++++++++++++++-
 zapcore/field.go      |  2 +-
 zapcore/field_test.go | 15 +++++++++++++++
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/zapcore/error.go b/zapcore/error.go
index 9ba2272c3..f2a07d786 100644
--- a/zapcore/error.go
+++ b/zapcore/error.go
@@ -22,6 +22,7 @@ package zapcore
 
 import (
 	"fmt"
+	"reflect"
 	"sync"
 )
 
@@ -42,7 +43,23 @@ import (
 //      ...
 //    ],
 //  }
-func encodeError(key string, err error, enc ObjectEncoder) error {
+func encodeError(key string, err error, enc ObjectEncoder) (retErr error) {
+	// Try to capture panics (from nil references or otherwise) when calling
+	// the Error() method
+	defer func() {
+		if rerr := recover(); rerr != nil {
+			// If it's a nil pointer, just say "<nil>". The likeliest causes are a
+			// error that fails to guard against nil or a nil pointer for a
+			// value receiver, and in either case, "<nil>" is a nice result.
+			if v := reflect.ValueOf(err); v.Kind() == reflect.Ptr && v.IsNil() {
+				enc.AddString(key, "<nil>")
+				return
+			}
+
+			retErr = fmt.Errorf("PANIC=%v", rerr)
+		}
+	}()
+
 	basic := err.Error()
 	enc.AddString(key, basic)
 
diff --git a/zapcore/field.go b/zapcore/field.go
index 7e255d63e..e0105868e 100644
--- a/zapcore/field.go
+++ b/zapcore/field.go
@@ -167,7 +167,7 @@ func (f Field) AddTo(enc ObjectEncoder) {
 	case StringerType:
 		err = encodeStringer(f.Key, f.Interface, enc)
 	case ErrorType:
-		encodeError(f.Key, f.Interface.(error), enc)
+		err = encodeError(f.Key, f.Interface.(error), enc)
 	case SkipType:
 		break
 	default:
diff --git a/zapcore/field_test.go b/zapcore/field_test.go
index 44aeee62e..31de0b623 100644
--- a/zapcore/field_test.go
+++ b/zapcore/field_test.go
@@ -80,6 +80,19 @@ func (o *obj) String() string {
 	return "obj"
 }
 
+type errObj struct {
+	kind   int
+	errMsg string
+}
+
+func (eobj *errObj) Error() string {
+	if eobj.kind == 1 {
+		panic("panic in Error() method")
+	} else {
+		return eobj.errMsg
+	}
+}
+
 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.")
@@ -102,6 +115,7 @@ func TestFieldAddingError(t *testing.T) {
 		{t: StringerType, iface: &obj{1}, want: empty, err: "PANIC=panic with string"},
 		{t: StringerType, iface: &obj{2}, want: empty, err: "PANIC=panic with error"},
 		{t: StringerType, iface: &obj{3}, want: empty, err: "PANIC=<nil>"},
+		{t: ErrorType, iface: &errObj{kind: 1}, want: empty, err: "PANIC=panic in Error() method"},
 	}
 	for _, tt := range tests {
 		f := Field{Key: "k", Interface: tt.iface, Type: tt.t}
@@ -150,6 +164,7 @@ func TestFields(t *testing.T) {
 		{t: SkipType, want: interface{}(nil)},
 		{t: StringerType, iface: (*url.URL)(nil), want: "<nil>"},
 		{t: StringerType, iface: (*users)(nil), want: "<nil>"},
+		{t: ErrorType, iface: (*errObj)(nil), want: "<nil>"},
 	}
 
 	for _, tt := range tests {