Skip to content

Commit

Permalink
Fix handling of Time values out of UnixNano range (#804)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
prashantv authored Mar 27, 2020
1 parent f994215 commit ec2dc45
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 1 deletion.
8 changes: 8 additions & 0 deletions field.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()}
}

Expand Down
5 changes: 5 additions & 0 deletions field_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
package zap

import (
"math"
"net"
"sync"
"testing"
Expand Down Expand Up @@ -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)},
Expand Down
7 changes: 6 additions & 1 deletion zapcore/field.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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:
Expand Down
19 changes: 19 additions & 0 deletions zapcore/field_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)),
Expand Down

0 comments on commit ec2dc45

Please sign in to comment.