From e48bc3efd66420e704a7e5209426784ab9f7c08a Mon Sep 17 00:00:00 2001 From: Prashant Varanasi Date: Tue, 24 Mar 2020 11:28:21 -0700 Subject: [PATCH 1/3] Fix handling of Time values out of UnixNano rnage 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 | 4 ++++ zapcore/field.go | 7 ++++++- zapcore/field_test.go | 11 +++++++++++ 4 files changed, 29 insertions(+), 1 deletion(-) diff --git a/field.go b/field.go index 83c1ea245..7d65f33e6 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, 0) + _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..73901a37d 100644 --- a/field_test.go +++ b/field_test.go @@ -21,6 +21,7 @@ package zap import ( + "math" "net" "sync" "testing" @@ -107,6 +108,9 @@ 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.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..0ff876654 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,10 @@ func TestFields(t *testing.T) { } func TestEquals(t *testing.T) { + timeOutOfRange := time.Unix(0, math.MaxInt64).Add(time.Nanosecond) + timeOutOfRangeNano := time.Unix(0, timeOutOfRange.UnixNano()) + require.NotEqual(t, timeOutOfRange, timeOutOfRangeNano, "should be different as value is out of UnixNano range") + tests := []struct { a, b Field want bool @@ -198,6 +203,12 @@ func TestEquals(t *testing.T) { b: zap.Time("k", time.Unix(1000, 1000).In(time.FixedZone("TEST", -8))), want: false, }, + { + // Values outside the UnixNano range were encoded incorrectly (#737, #803). + a: zap.Time("k", timeOutOfRange), + b: zap.Time("k", timeOutOfRangeNano), + want: false, + }, { a: zap.Time("k", time.Unix(1000, 1000)), b: zap.Time("k", time.Unix(1000, 2000)), From 06a1362f64c4e9725fd4f736a127b7da1592417c Mon Sep 17 00:00:00 2001 From: Prashant Varanasi Date: Wed, 25 Mar 2020 23:42:57 -0700 Subject: [PATCH 2/3] _minTime should use math.MinInt64, and test for it Co-Authored-By: Abhinav Gupta --- field.go | 2 +- field_test.go | 1 + zapcore/field_test.go | 20 ++++++++++++++------ 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/field.go b/field.go index 7d65f33e6..dd558fc23 100644 --- a/field.go +++ b/field.go @@ -33,7 +33,7 @@ import ( type Field = zapcore.Field var ( - _minTimeInt64 = time.Unix(0, 0) + _minTimeInt64 = time.Unix(0, math.MinInt64) _maxTimeInt64 = time.Unix(0, math.MaxInt64) ) diff --git a/field_test.go b/field_test.go index 73901a37d..cc7f2e564 100644 --- a/field_test.go +++ b/field_test.go @@ -108,6 +108,7 @@ 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))}, diff --git a/zapcore/field_test.go b/zapcore/field_test.go index 0ff876654..95444a8a4 100644 --- a/zapcore/field_test.go +++ b/zapcore/field_test.go @@ -165,9 +165,13 @@ func TestFields(t *testing.T) { } func TestEquals(t *testing.T) { - timeOutOfRange := time.Unix(0, math.MaxInt64).Add(time.Nanosecond) - timeOutOfRangeNano := time.Unix(0, timeOutOfRange.UnixNano()) - require.NotEqual(t, timeOutOfRange, timeOutOfRangeNano, "should be different as value is out of UnixNano range") + // 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.NotEqual(t, timeOutOfRangeHigh, timeOutOfRangeHighNano, "should be different as value is > UnixNano range") + require.NotEqual(t, timeOutOfRangeHigh, timeOutOfRangeHighNano, "should be different as value is < UnixNano range") tests := []struct { a, b Field @@ -204,9 +208,13 @@ func TestEquals(t *testing.T) { want: false, }, { - // Values outside the UnixNano range were encoded incorrectly (#737, #803). - a: zap.Time("k", timeOutOfRange), - b: zap.Time("k", timeOutOfRangeNano), + a: zap.Time("k", timeOutOfRangeLow), + b: zap.Time("k", timeOutOfRangeLowNano), + want: false, + }, + { + a: zap.Time("k", timeOutOfRangeHigh), + b: zap.Time("k", timeOutOfRangeHighNano), want: false, }, { From 0182292fa8fadce408cb8241bd1a2520b8e320e6 Mon Sep 17 00:00:00 2001 From: Prashant Varanasi Date: Thu, 26 Mar 2020 14:18:54 -0700 Subject: [PATCH 3/3] Use `time.Equal` to ensure times are not the same Co-Authored-By: Abhinav Gupta --- zapcore/field_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zapcore/field_test.go b/zapcore/field_test.go index 95444a8a4..a0c9c12b7 100644 --- a/zapcore/field_test.go +++ b/zapcore/field_test.go @@ -170,8 +170,8 @@ func TestEquals(t *testing.T) { timeOutOfRangeLow := time.Unix(0, math.MinInt64).Add(-time.Nanosecond) timeOutOfRangeHighNano := time.Unix(0, timeOutOfRangeHigh.UnixNano()) timeOutOfRangeLowNano := time.Unix(0, timeOutOfRangeLow.UnixNano()) - require.NotEqual(t, timeOutOfRangeHigh, timeOutOfRangeHighNano, "should be different as value is > UnixNano range") - require.NotEqual(t, timeOutOfRangeHigh, timeOutOfRangeHighNano, "should be different as value is < UnixNano range") + 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