Skip to content

Commit

Permalink
Fix handling of Time values out of UnixNano range (uber-go#804)
Browse files Browse the repository at this point in the history
Fixes uber-go#737, uber-go#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 and uvfive committed May 21, 2020
1 parent 6484585 commit b3ae5d9
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 1 deletion.
8 changes: 8 additions & 0 deletions field.go
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
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
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
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 b3ae5d9

Please sign in to comment.