Skip to content

Improve code quality by removing a potential nil panic #1499

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chengpeng-wang
Copy link

Dear developers,

In the Logger.check function, the result of log.core.Check(ent, nil) is assigned to ce and later used in a switch block that may call methods on ce (e.g., ce.After(...)) without ensuring it is non-nil. This can lead to a nil pointer dereference panic if the implementation of Check returns nil.

Although the current code checks whether ce != nil holds at line 361 (Link), the check occurs too late. I think the original purpose of introducing the !willWrite check should be to avoid the nil pointer dereference panic, but it was mistakenly placed in the wrong location in the program.

We have provided a patch for your reference. With this patch, the code would be more reliable than the original one. We hope our effort helps improve your code quality.

Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

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

This is by design.
There's no risk of nil panic here as ce.After is nil safe.
It creates a ce if there wasn't one there before.

zap/zapcore/entry.go

Lines 301 to 305 in 6d48253

func (ce *CheckedEntry) After(ent Entry, hook CheckWriteHook) *CheckedEntry {
if ce == nil {
ce = getCheckedEntry()
ce.Entry = ent
}

This PR would change the behavior, returning nil instead of non-nil in these cases, dropping the panic/fatal/dpanic post-logging behaviors.

There are plenty of unit tests that catch this. Please run go test, and you'll see that this change breaks those behaviors.

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.

2 participants