From 1c0c73912192a0064b47200d3ceabde23f4647dd Mon Sep 17 00:00:00 2001 From: FMLS Date: Thu, 24 Sep 2020 17:32:47 +0800 Subject: [PATCH 1/4] 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 { From de2b032c0d2ef0e152c35b993ca5b1446d8719b2 Mon Sep 17 00:00:00 2001 From: FMLS Date: Wed, 4 Nov 2020 16:58:42 +0800 Subject: [PATCH 2/4] add a unit test for encode error --- zapcore/field_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/zapcore/field_test.go b/zapcore/field_test.go index 698138cc4..31de0b623 100644 --- a/zapcore/field_test.go +++ b/zapcore/field_test.go @@ -81,11 +81,16 @@ func (o *obj) String() string { } type errObj struct { - errMessage string + kind int + errMsg string } func (eobj *errObj) Error() string { - return eobj.errMessage + if eobj.kind == 1 { + panic("panic in Error() method") + } else { + return eobj.errMsg + } } func TestUnknownFieldType(t *testing.T) { @@ -110,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="}, + {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} From fbb85ad2917a1103efe56245bbb04561f87b3554 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 24 Nov 2020 10:34:01 -0800 Subject: [PATCH 3/4] Apply suggestions from code review --- zapcore/error.go | 6 +++--- zapcore/field.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/zapcore/error.go b/zapcore/error.go index a338f8532..f2a07d786 100644 --- a/zapcore/error.go +++ b/zapcore/error.go @@ -49,7 +49,7 @@ func encodeError(key string, err error, enc ObjectEncoder) (retErr error) { 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 + // error 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, "") @@ -65,7 +65,7 @@ func encodeError(key string, err error, enc ObjectEncoder) (retErr error) { switch e := err.(type) { case errorGroup: - retErr = enc.AddArray(key+"Causes", errArray(e.Errors())) + return enc.AddArray(key+"Causes", errArray(e.Errors())) case fmt.Formatter: verbose := fmt.Sprintf("%+v", e) if verbose != basic { @@ -74,7 +74,7 @@ func encodeError(key string, err error, enc ObjectEncoder) (retErr error) { enc.AddString(key+"Verbose", verbose) } } - return retErr + return nil } type errorGroup interface { diff --git a/zapcore/field.go b/zapcore/field.go index b46449e49..e0105868e 100644 --- a/zapcore/field.go +++ b/zapcore/field.go @@ -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 rerr := recover(); rerr != nil { + if err := recover(); err != 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", rerr) + retErr = fmt.Errorf("PANIC=%v", err) } }() From 421e6152d59aa262a7245517196728462a97f8d7 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 24 Nov 2020 10:35:16 -0800 Subject: [PATCH 4/4] error.go: revert import grouping change --- error.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/error.go b/error.go index db0002d7e..65982a51e 100644 --- a/error.go +++ b/error.go @@ -21,8 +21,9 @@ package zap import ( - "go.uber.org/zap/zapcore" "sync" + + "go.uber.org/zap/zapcore" ) var _errArrayElemPool = sync.Pool{New: func() interface{} {