From 1c0c73912192a0064b47200d3ceabde23f4647dd Mon Sep 17 00:00:00 2001 From: FMLS Date: Thu, 24 Sep 2020 17:32:47 +0800 Subject: [PATCH] BUGFIX: judge nil pointer stored in error interface --- error.go | 3 +-- zapcore/error.go | 23 ++++++++++++++++++++--- zapcore/field.go | 6 +++--- zapcore/field_test.go | 9 +++++++++ 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/error.go b/error.go index 65982a51e..db0002d7e 100644 --- a/error.go +++ b/error.go @@ -21,9 +21,8 @@ package zap import ( - "sync" - "go.uber.org/zap/zapcore" + "sync" ) var _errArrayElemPool = sync.Pool{New: func() interface{} { diff --git a/zapcore/error.go b/zapcore/error.go index 9ba2272c3..a338f8532 100644 --- a/zapcore/error.go +++ b/zapcore/error.go @@ -22,6 +22,7 @@ package zapcore import ( "fmt" + "reflect" "sync" ) @@ -42,13 +43,29 @@ 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 "". The likeliest causes are a + // Stringer that fails to guard against nil or a nil pointer for a + // value receiver, and in either case, "" is a nice result. + if v := reflect.ValueOf(err); v.Kind() == reflect.Ptr && v.IsNil() { + enc.AddString(key, "") + return + } + + retErr = fmt.Errorf("PANIC=%v", rerr) + } + }() + basic := err.Error() enc.AddString(key, basic) switch e := err.(type) { case errorGroup: - return enc.AddArray(key+"Causes", errArray(e.Errors())) + retErr = enc.AddArray(key+"Causes", errArray(e.Errors())) case fmt.Formatter: verbose := fmt.Sprintf("%+v", e) if verbose != basic { @@ -57,7 +74,7 @@ func encodeError(key string, err error, enc ObjectEncoder) error { enc.AddString(key+"Verbose", verbose) } } - return nil + return retErr } type errorGroup interface { diff --git a/zapcore/field.go b/zapcore/field.go index 7e255d63e..b46449e49 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: @@ -209,7 +209,7 @@ func encodeStringer(key string, stringer interface{}, enc ObjectEncoder) (retErr // Try to capture panics (from nil references or otherwise) when calling // the String() method, similar to https://golang.org/src/fmt/print.go#L540 defer func() { - if err := recover(); err != nil { + if rerr := recover(); rerr != nil { // If it's a nil pointer, just say "". The likeliest causes are a // Stringer that fails to guard against nil or a nil pointer for a // value receiver, and in either case, "" is a nice result. @@ -218,7 +218,7 @@ func encodeStringer(key string, stringer interface{}, enc ObjectEncoder) (retErr return } - retErr = fmt.Errorf("PANIC=%v", err) + retErr = fmt.Errorf("PANIC=%v", rerr) } }() diff --git a/zapcore/field_test.go b/zapcore/field_test.go index 44aeee62e..698138cc4 100644 --- a/zapcore/field_test.go +++ b/zapcore/field_test.go @@ -80,6 +80,14 @@ func (o *obj) String() string { return "obj" } +type errObj struct { + errMessage string +} + +func (eobj *errObj) Error() string { + return eobj.errMessage +} + 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.") @@ -150,6 +158,7 @@ func TestFields(t *testing.T) { {t: SkipType, want: interface{}(nil)}, {t: StringerType, iface: (*url.URL)(nil), want: ""}, {t: StringerType, iface: (*users)(nil), want: ""}, + {t: ErrorType, iface: (*errObj)(nil), want: ""}, } for _, tt := range tests {