Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

field/error: Handle panic in Error() #867

Merged
merged 4 commits into from
Nov 24, 2020
Merged

field/error: Handle panic in Error() #867

merged 4 commits into from
Nov 24, 2020

Conversation

FMLS
Copy link
Contributor

@FMLS FMLS commented Sep 24, 2020

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))

@CLAassistant
Copy link

CLAassistant commented Sep 24, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Sep 24, 2020

Codecov Report

Merging #867 (421e615) into master (404189c) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #867      +/-   ##
==========================================
- Coverage   98.36%   98.21%   -0.15%     
==========================================
  Files          43       43              
  Lines        2390     1910     -480     
==========================================
- Hits         2351     1876     -475     
+ Misses         32       27       -5     
  Partials        7        7              
Impacted Files Coverage Δ
zapcore/error.go 96.77% <100.00%> (+3.02%) ⬆️
zapcore/field.go 100.00% <100.00%> (ø)
internal/ztest/timeout.go 81.81% <0.00%> (-4.85%) ⬇️
zapcore/write_syncer.go 87.50% <0.00%> (-2.98%) ⬇️
internal/exit/exit.go 90.90% <0.00%> (-2.85%) ⬇️
zapcore/encoder.go 87.09% <0.00%> (-1.14%) ⬇️
zapcore/core.go 93.54% <0.00%> (-1.05%) ⬇️
zapcore/entry.go 93.82% <0.00%> (-0.80%) ⬇️
stacktrace.go 96.00% <0.00%> (-0.56%) ⬇️
global.go 96.66% <0.00%> (-0.48%) ⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 404189c...421e615. Read the comment docs.

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @FMLS

Using reflect check for nil isn't always right, as it's possible to have a nil value that implements the error interface,
https://play.golang.org/p/5T8S35zdGsH

To handle nil in this case, please follow the same logic used by Stringer to detect a nil panic (which uses recover, similar to the standard library).

zapcore/field.go Outdated Show resolved Hide resolved
zapcore/field.go Outdated Show resolved Hide resolved
zapcore/error.go Outdated Show resolved Hide resolved
zapcore/error.go Outdated Show resolved Hide resolved
zapcore/error.go Outdated Show resolved Hide resolved
error.go Outdated Show resolved Hide resolved
@abhinav abhinav changed the title BUGFIX: judge nil pointer stored in error interface field/error: Handle panic in Error() Nov 24, 2020
@abhinav abhinav merged commit 4b2b07c into uber-go:master Nov 24, 2020
cgxxv pushed a commit to cgxxv/zap that referenced this pull request Mar 25, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants