From b3ae5d9e00a58775a162d08816e7e47902e398e2 Mon Sep 17 00:00:00 2001 From: Prashant Varanasi Date: Thu, 26 Mar 2020 21:34:00 -0700 Subject: [PATCH] Fix handling of Time values out of UnixNano range (#804) Fixes #737, #803. Time values are encoded by storing the UnixNano representation, but this fails when the value of UnixNano is out of int64 range, see docs: https://golang.org/pkg/time/#Time.UnixNano > The result is undefined if the Unix time in nanoseconds cannot be represented by an int64 (a date before the year 1678 or after 2262) Fix this by storing values outside of UnixNano range as-is rather than using UnixNano with a new Field type. --- field.go | 8 ++++++++ field_test.go | 5 +++++ zapcore/field.go | 7 ++++++- zapcore/field_test.go | 19 +++++++++++++++++++ 4 files changed, 38 insertions(+), 1 deletion(-) diff --git a/field.go b/field.go index 83c1ea245..dd558fc23 100644 --- a/field.go +++ b/field.go @@ -32,6 +32,11 @@ import ( // improves the navigability of this package's API documentation. type Field = zapcore.Field +var ( + _minTimeInt64 = time.Unix(0, math.MinInt64) + _maxTimeInt64 = time.Unix(0, math.MaxInt64) +) + // Skip constructs a no-op field, which is often useful when handling invalid // inputs in other Field constructors. func Skip() Field { @@ -339,6 +344,9 @@ func Stringer(key string, val fmt.Stringer) Field { // Time constructs a Field with the given key and value. The encoder // controls how the time is serialized. func Time(key string, val time.Time) Field { + if val.Before(_minTimeInt64) || val.After(_maxTimeInt64) { + return Field{Key: key, Type: zapcore.TimeFullType, Interface: val} + } return Field{Key: key, Type: zapcore.TimeType, Integer: val.UnixNano(), Interface: val.Location()} } diff --git a/field_test.go b/field_test.go index d25c964fb..cc7f2e564 100644 --- a/field_test.go +++ b/field_test.go @@ -21,6 +21,7 @@ package zap import ( + "math" "net" "sync" "testing" @@ -107,6 +108,10 @@ func TestFieldConstructors(t *testing.T) { {"String", Field{Key: "k", Type: zapcore.StringType, String: "foo"}, String("k", "foo")}, {"Time", Field{Key: "k", Type: zapcore.TimeType, Integer: 0, Interface: time.UTC}, Time("k", time.Unix(0, 0).In(time.UTC))}, {"Time", Field{Key: "k", Type: zapcore.TimeType, Integer: 1000, Interface: time.UTC}, Time("k", time.Unix(0, 1000).In(time.UTC))}, + {"Time", Field{Key: "k", Type: zapcore.TimeType, Integer: math.MinInt64, Interface: time.UTC}, Time("k", time.Unix(0, math.MinInt64).In(time.UTC))}, + {"Time", Field{Key: "k", Type: zapcore.TimeType, Integer: math.MaxInt64, Interface: time.UTC}, Time("k", time.Unix(0, math.MaxInt64).In(time.UTC))}, + {"Time", Field{Key: "k", Type: zapcore.TimeFullType, Interface: time.Time{}}, Time("k", time.Time{})}, + {"Time", Field{Key: "k", Type: zapcore.TimeFullType, Interface: time.Unix(math.MaxInt64, 0)}, Time("k", time.Unix(math.MaxInt64, 0))}, {"Uint", Field{Key: "k", Type: zapcore.Uint64Type, Integer: 1}, Uint("k", 1)}, {"Uint64", Field{Key: "k", Type: zapcore.Uint64Type, Integer: 1}, Uint64("k", 1)}, {"Uint32", Field{Key: "k", Type: zapcore.Uint32Type, Integer: 1}, Uint32("k", 1)}, diff --git a/zapcore/field.go b/zapcore/field.go index ae772e4a1..6e05f831f 100644 --- a/zapcore/field.go +++ b/zapcore/field.go @@ -65,8 +65,11 @@ const ( Int8Type // StringType indicates that the field carries a string. StringType - // TimeType indicates that the field carries a time.Time. + // TimeType indicates that the field carries a time.Time that is + // representable by a UnixNano() stored as an int64. TimeType + // TimeFullType indicates that the field carries a time.Time stored as-is. + TimeFullType // Uint64Type indicates that the field carries a uint64. Uint64Type // Uint32Type indicates that the field carries a uint32. @@ -145,6 +148,8 @@ func (f Field) AddTo(enc ObjectEncoder) { // Fall back to UTC if location is nil. enc.AddTime(f.Key, time.Unix(0, f.Integer)) } + case TimeFullType: + enc.AddTime(f.Key, f.Interface.(time.Time)) case Uint64Type: enc.AddUint64(f.Key, uint64(f.Integer)) case Uint32Type: diff --git a/zapcore/field_test.go b/zapcore/field_test.go index 8ec045fd7..a0c9c12b7 100644 --- a/zapcore/field_test.go +++ b/zapcore/field_test.go @@ -29,6 +29,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/zap" . "go.uber.org/zap/zapcore" ) @@ -164,6 +165,14 @@ func TestFields(t *testing.T) { } func TestEquals(t *testing.T) { + // Values outside the UnixNano range were encoded incorrectly (#737, #803). + timeOutOfRangeHigh := time.Unix(0, math.MaxInt64).Add(time.Nanosecond) + timeOutOfRangeLow := time.Unix(0, math.MinInt64).Add(-time.Nanosecond) + timeOutOfRangeHighNano := time.Unix(0, timeOutOfRangeHigh.UnixNano()) + timeOutOfRangeLowNano := time.Unix(0, timeOutOfRangeLow.UnixNano()) + require.False(t, timeOutOfRangeHigh.Equal(timeOutOfRangeHighNano), "should be different as value is > UnixNano range") + require.False(t, timeOutOfRangeHigh.Equal(timeOutOfRangeHighNano), "should be different as value is < UnixNano range") + tests := []struct { a, b Field want bool @@ -198,6 +207,16 @@ func TestEquals(t *testing.T) { b: zap.Time("k", time.Unix(1000, 1000).In(time.FixedZone("TEST", -8))), want: false, }, + { + a: zap.Time("k", timeOutOfRangeLow), + b: zap.Time("k", timeOutOfRangeLowNano), + want: false, + }, + { + a: zap.Time("k", timeOutOfRangeHigh), + b: zap.Time("k", timeOutOfRangeHighNano), + want: false, + }, { a: zap.Time("k", time.Unix(1000, 1000)), b: zap.Time("k", time.Unix(1000, 2000)),