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

Add Config validation to build stage to prevent runtime panics #777

Closed
xeoncross opened this issue Jan 22, 2020 · 2 comments · Fixed by #791
Closed

Add Config validation to build stage to prevent runtime panics #777

xeoncross opened this issue Jan 22, 2020 · 2 comments · Fixed by #791

Comments

@xeoncross
Copy link

xeoncross commented Jan 22, 2020

When creating a custom zap.Config there seems to be only minimal validation of the configuration that results in runtime panics if certain properties are missing. This is dangerous as all required configuration properties should be checked at the build stage.

There is probably a reason this has not been done, but opening an issue nonetheless.

Example

	cfg := zap.Config{
		Encoding:    "json",
		OutputPaths: []string{"stderr"},
		EncoderConfig: zapcore.EncoderConfig{
			MessageKey: "msg",
		},
	}

	logger, err := cfg.Build()
	if err != nil {
		panic(err)
	}

	logger.Sugar().Errorw("Problem here", "foo", "bar") // Panic

This results in the following panic:


panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x12696c6]

goroutine 1 [running]:
go.uber.org/atomic.(*Int32).Load(...)
	/Users/owner/go/pkg/mod/go.uber.org/atomic@v1.5.1/atomic.go:41
go.uber.org/zap.AtomicLevel.Level(...)
	/Users/owner/go/pkg/mod/go.uber.org/zap@v1.13.0/level.go:97
go.uber.org/zap.AtomicLevel.Enabled(0x0, 0x10b5c02, 0xc000098c30)
	/Users/owner/go/pkg/mod/go.uber.org/zap@v1.13.0/level.go:92 +0x6
go.uber.org/zap.(*SugaredLogger).log(0xc000055f00, 0xc000010002, 0x1312a3e, 0xc, 0x0, 0x0, 0x0, 0xc000055f30, 0x2, 0x2)
	/Users/owner/go/pkg/mod/go.uber.org/zap@v1.13.0/sugar.go:221 +0x1aa
go.uber.org/zap.(*SugaredLogger).Errorw(...)
	/Users/owner/go/pkg/mod/go.uber.org/zap@v1.13.0/sugar.go:191
main.main()
	/Users/owner/zaplog/zaplog.go:22 +0x27b
exit status 2

The issue here is we are missing a "Level"

Level:            zap.NewAtomicLevelAt(zapcore.DebugLevel),

However, there are other issues as well. Suppose you specify the "TimeKey", but not the "EncodeTime".

	cfg := zap.Config{
		Level:       zap.NewAtomicLevelAt(zapcore.DebugLevel),
		Encoding:    "json",
		OutputPaths: []string{"stderr"},
		EncoderConfig: zapcore.EncoderConfig{
			MessageKey: "msg",
			TimeKey:    "ts",
		},
	}

This configuration will build without error, but you again will be faced with runtime error: invalid memory address or nil pointer dereference on use.

@YashishDua
Copy link
Contributor

Seems like this can be taken up! @mway Should I go ahead or this needs prior design discussion?

@abhinav
Copy link
Collaborator

abhinav commented Feb 13, 2020

@YashishDua Yes, please! If you'd like to take a stab at it, go for it. Thank you!
The validation should probably occur in Config.Build.

abhinav added a commit that referenced this issue Mar 4, 2020
This adds validation for Level and EncoderConfig.EncoderTime when
building a logger from a zap.Config.

Resolves #777

Co-authored-by: Abhinav Gupta <abg@uber.com>
cgxxv pushed a commit to cgxxv/zap that referenced this issue Mar 25, 2022
This adds validation for Level and EncoderConfig.EncoderTime when
building a logger from a zap.Config.

Resolves uber-go#777

Co-authored-by: Abhinav Gupta <abg@uber.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants