Prevent zap.Object from panicing on nils

[alshopov]

It is possible to call zap.Object with nil or nil interface This will cause panic in zap bringing down the app using it

The standard way of handling this is via nilField - compare all the constructors on pointers. As interfaces are similar to pointers - lets handle this case the same way

Add tests in zapcore for equality on zap.Object(nil) Show ReflectType does not panic on nils
Add test that zap.Object(nil) works

Update benchmarks and use such a field to show performance and allocation are not affected excessively

Refs #1500

[alshopov]

This is a proposed fix for #1500

A demo on the current problem is available in Gp playground: https://go.dev/play/p/yUgOv6aEQ3t

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.59%. Comparing base (a6afd05) to head (e7b227a).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1501   +/-   ##
=======================================
  Coverage   98.59%   98.59%           
=======================================
  Files          53       53           
  Lines        3562     3565    +3     
=======================================
+ Hits         3512     3515    +3     
  Misses         42       42           
  Partials        8        8           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JacobOaks]

Since this only uses nilField for nil interfaces (i.e., when there isn't an actual MarshalLogObject method to call) I think this makes sense, but I'd like to wait for a second pair of eyes as well. Just merged a PR that should fix the lint issue in CI.

[tchung1118]

I think this makes sense as well. Just make sure to rebase before merging to re-run the lint step in CI. Thank you for the contribution!

[alshopov]

@JacobOaks , @tchung1118 - rebase done, all lint checks run and green

[prashantv]

Just a heads up that this only helps when passing in an interface type with a nil type + value, but not in the case of a struct pointer that's nil. I think the latter is likely more common than passing a nil interface.

Repro:

func TestNilObject(t *testing.T) {
        logger := zap.NewExample() // or NewProduction, or NewDevelopment
        defer logger.Sync()

        var nilObj zapcore.ObjectMarshaler
        logger.Info("nil interface works", zap.Object("nilObj", nilObj))

        var u *user
        logger.Info("nil typed value does not", zap.Object("nilUser", u))
}

type user struct {
        name string
}

func (u *user) MarshalLogObject(enc zapcore.ObjectEncoder) error {
        enc.AddString("name", u.name)
        return nil
}

output:

=== RUN   TestNilObject
{"level":"info","msg":"nil interface works","nilObj":null}
--- FAIL: TestNilObject (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference

To handle the latter case, we recover panics and convert those to "":

zap/zapcore/field.go

Lines 214 to 229 in 07077a6

	func encodeStringer(key string, stringer interface{}, enc ObjectEncoder) (retErr error) {
	// 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 it's a nil pointer, just say "<nil>". The likeliest causes are a
	// Stringer that fails to guard against nil or a nil pointer for a
	// value receiver, and in either case, "<nil>" is a nice result.
	if v := reflect.ValueOf(stringer); v.Kind() == reflect.Ptr && v.IsNil() {
	enc.AddString(key, "<nil>")
	return
	}
	retErr = fmt.Errorf("PANIC=%v", err)
	}
	}()

[alshopov]

@prashantv

not in the case of a struct pointer that's nil. I think the latter is likely more common than passing a nil interface.
I get your point but these are different problems and need different solutions.

What I solved is the case of a missing value in the interface.

What you point out is a MarshalLogObject method with a bug. It does nil dereference without check and the code rightfully panics. A possible correct way would be like this: https://go.dev/play/p/bHDT2unMpgs

type user struct {
	name string
}

func (u *user) MarshalLogObject(enc zapcore.ObjectEncoder) error {
	msg := "<nil>"
	if u != nil {
		msg = u.name
	}
	enc.AddString("name", msg)
	return nil
}

You do not need nil-s at all. Here is a much simpler example without using pointers at all: https://go.dev/play/p/6gmHs7HIBlk

type user struct{}

func (u user) MarshalLogObject(enc zapcore.ObjectEncoder) error {
	panic("Catch me if you can")
}

func main() {
	logger := zap.NewExample()
	defer logger.Sync()

	var u user
	logger.Info("proper coded nil object works", zap.Object("nilUser", u))
}

I will propose a fix for this soon.

[prashantv]

What I solved is the case of a missing value in the interface.

To make sure we're using the same terminology, I'm using it as described here:

interface values can be thought of as a tuple of a value and a concrete type

The check added here only works when the type is nil (and when that's the case, the value is also nil).

It does not help if the type is non-nil, but the value is nil, since it will try to make a method call.

What you point out is a MarshalLogObject method with a bug. It does nil dereference without check and the code rightfully panics.

Even if MarshalLogOjbect is on a value type (where it can't do a nil dereference), you can still cause a panic by passing a nil value:
https://go.dev/play/p/YLIJxL91nrq

type user struct{}

func (u user) MarshalLogObject(enc zapcore.ObjectEncoder) error {
	enc.AddString("foo", "bar")
	return nil
}

func main() {
	logger := zap.NewExample()
	defer logger.Sync()

	var u *user
	logger.Info("proper coded nil object works", zap.Object("nilUser", u))
}

The solution used for stringer will also help here, but I wanted to call out that the solution here helps in limited cases, and there's other nil value cases for which #1500 is not solved.

[alshopov]

I think I see what you mean now. The previous example did not show that.
In your last example: since the method set of a pointer to a type includes the methods of a type, a pointer to an ObjectMarshaler is accepted as an argument to zap.Object
When the encoder tries to invoke the method (for example) it results in a nil dereference.
I can try to implement something similar. That would also cover the badly implemented ObjectMarshaler case.