Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Decouple time.Time parsing from empty interface behavior. #503

Merged
merged 1 commit into from
Mar 14, 2024
Merged

Decouple time.Time parsing from empty interface behavior. #503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 107 additions & 49 deletions decode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ import (
// To unmarshal a CBOR text string into a time.Time value, Unmarshal parses text
// string formatted in RFC3339. To unmarshal a CBOR integer/float into a
// time.Time value, Unmarshal creates an unix time with integer/float as seconds
// and fractional seconds since January 1, 1970 UTC.
// and fractional seconds since January 1, 1970 UTC. As a special case, Infinite
// and NaN float values decode to time.Time's zero value.
Comment on lines +90 to +91
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding the comment about special case! 👍

//
// To unmarshal CBOR null (0xf6) and undefined (0xf7) values into a
// slice/map/pointer, Unmarshal sets Go value to nil. Because null is often
Expand Down Expand Up @@ -476,8 +477,28 @@ type DecOptions struct {
// DupMapKey specifies whether to enforce duplicate map key.
DupMapKey DupMapKeyMode

// TimeTag specifies whether to check validity of time.Time (e.g. valid tag number and tag content type).
// For now, valid tag number means 0 or 1 as specified in RFC 7049 if the Go type is time.Time.
// TimeTag specifies whether or not untagged data items, or tags other
// than tag 0 and tag 1, can be decoded to time.Time. If tag 0 or tag 1
// appears in an input, the type of its content is always validated as
// specified in RFC 8949. That behavior is not controlled by this
// option. The behavior of the supported modes are:
//
// DecTagIgnored (default): Untagged text strings and text strings
// enclosed in tags other than 0 and 1 are decoded as though enclosed
// in tag 0. Untagged unsigned integers, negative integers, and
// floating-point numbers (or those enclosed in tags other than 0 and
// 1) are decoded as though enclosed in tag 1. Decoding a tag other
// than 0 or 1 enclosing simple values null or undefined into a
// time.Time does not modify the destination value.
//
// DecTagOptional: Untagged text strings are decoded as though
// enclosed in tag 0. Untagged unsigned integers, negative integers,
// and floating-point numbers are decoded as though enclosed in tag
// 1. Tags other than 0 and 1 will produce an error on attempts to
// decode them into a time.Time.
//
// DecTagRequired: Only tags 0 and 1 can be decoded to time.Time. Any
// other input will produce an error.
Comment on lines +480 to +501
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for improving the documentation! 👍

TimeTag DecTagMode

// MaxNestedLevels specifies the max nested levels allowed for any combination of CBOR array, maps, and tags.
Expand Down Expand Up @@ -1024,15 +1045,15 @@ func (d *decoder) parseToValue(v reflect.Value, tInfo *typeInfo) error { //nolin
}

// Check validity of supported built-in tags.
if d.nextCBORType() == cborTypeTag {
off := d.off
off := d.off
for d.nextCBORType() == cborTypeTag {
_, _, tagNum := d.getHead()
if err := validBuiltinTag(tagNum, d.data[d.off]); err != nil {
d.skip()
return err
}
d.off = off
}
d.off = off

if tInfo.spclType != specialTypeNone {
switch tInfo.spclType {
Expand All @@ -1050,11 +1071,13 @@ func (d *decoder) parseToValue(v reflect.Value, tInfo *typeInfo) error { //nolin
d.skip()
return nil
}
tm, err := d.parseToTime()
tm, ok, err := d.parseToTime()
if err != nil {
return err
}
v.Set(reflect.ValueOf(tm))
if ok {
v.Set(reflect.ValueOf(tm))
}
return nil
case specialTypeUnmarshalerIface:
return d.parseToUnmarshaler(v)
Expand Down Expand Up @@ -1239,64 +1262,98 @@ func (d *decoder) parseToTag(v reflect.Value) error {
return nil
}

func (d *decoder) parseToTime() (tm time.Time, err error) {
t := d.nextCBORType()

// parseToTime decodes the current data item as a time.Time. The bool return value is false if and
// only if the destination value should remain unmodified.
func (d *decoder) parseToTime() (time.Time, bool, error) {
// Verify that tag number or absence of tag number is acceptable to specified timeTag.
if t == cborTypeTag {
if t := d.nextCBORType(); t == cborTypeTag {
if d.dm.timeTag == DecTagIgnored {
// Skip tag number
// Skip all enclosing tags
for t == cborTypeTag {
d.getHead()
t = d.nextCBORType()
}
if d.nextCBORNil() {
d.skip()
return time.Time{}, false, nil
}
} else {
// Read tag number
_, _, tagNum := d.getHead()
if tagNum != 0 && tagNum != 1 {
d.skip()
err = errors.New("cbor: wrong tag number for time.Time, got " + strconv.Itoa(int(tagNum)) + ", expect 0 or 1")
return
d.skip() // skip tag content
return time.Time{}, false, errors.New("cbor: wrong tag number for time.Time, got " + strconv.Itoa(int(tagNum)) + ", expect 0 or 1")
}
}
} else {
if d.dm.timeTag == DecTagRequired {
d.skip()
err = &UnmarshalTypeError{CBORType: t.String(), GoType: typeTime.String(), errorMsg: "expect CBOR tag value"}
return
return time.Time{}, false, &UnmarshalTypeError{CBORType: t.String(), GoType: typeTime.String(), errorMsg: "expect CBOR tag value"}
}
}

var content interface{}
content, err = d.parse(false)
if err != nil {
return
}

switch c := content.(type) {
case nil:
return
case uint64:
return time.Unix(int64(c), 0), nil
case int64:
return time.Unix(c, 0), nil
case float64:
if math.IsNaN(c) || math.IsInf(c, 0) {
return
}
f1, f2 := math.Modf(c)
return time.Unix(int64(f1), int64(f2*1e9)), nil
case string:
tm, err = time.Parse(time.RFC3339, c)
switch t := d.nextCBORType(); t {
case cborTypeTextString:
s, err := d.parseTextString()
if err != nil {
tm = time.Time{}
err = errors.New("cbor: cannot set " + c + " for time.Time: " + err.Error())
return
return time.Time{}, false, err
}
return
t, err := time.Parse(time.RFC3339, string(s))
if err != nil {
return time.Time{}, false, errors.New("cbor: cannot set " + string(s) + " for time.Time: " + err.Error())
}
return t, true, nil
case cborTypePositiveInt:
_, _, val := d.getHead()
if val > math.MaxInt64 {
return time.Time{}, false, &UnmarshalTypeError{
CBORType: t.String(),
GoType: typeTime.String(),
errorMsg: fmt.Sprintf("%d overflows Go's int64", val),
}
}
return time.Unix(int64(val), 0), true, nil
case cborTypeNegativeInt:
_, _, val := d.getHead()
if val > math.MaxInt64 {
if val == math.MaxUint64 {
// Maximum absolute value representable by negative integer is 2^64,
// not 2^64-1, so it overflows uint64.
return time.Time{}, false, &UnmarshalTypeError{
CBORType: t.String(),
GoType: typeTime.String(),
errorMsg: "-18446744073709551616 overflows Go's int64",
}
}
return time.Time{}, false, &UnmarshalTypeError{
CBORType: t.String(),
GoType: typeTime.String(),
errorMsg: fmt.Sprintf("-%d overflows Go's int64", val+1),
}
}
return time.Unix(int64(-1)^int64(val), 0), true, nil
case cborTypePrimitives:
_, ai, val := d.getHead()
benluddy marked this conversation as resolved.
Show resolved Hide resolved
var f float64
switch ai {
case 25:
f = float64(float16.Frombits(uint16(val)).Float32())
case 26:
f = float64(math.Float32frombits(uint32(val)))
case 27:
f = math.Float64frombits(val)
default:
return time.Time{}, false, &UnmarshalTypeError{CBORType: t.String(), GoType: typeTime.String()}
}

if math.IsNaN(f) || math.IsInf(f, 0) {
// https://www.rfc-editor.org/rfc/rfc8949.html#section-3.4.2-6
return time.Time{}, true, nil
}
seconds, fractional := math.Modf(f)
return time.Unix(int64(seconds), int64(fractional*1e9)), true, nil
default:
err = &UnmarshalTypeError{CBORType: t.String(), GoType: typeTime.String()}
return
return time.Time{}, false, &UnmarshalTypeError{CBORType: t.String(), GoType: typeTime.String()}
}
}

Expand Down Expand Up @@ -1336,15 +1393,15 @@ func (d *decoder) parse(skipSelfDescribedTag bool) (interface{}, error) { //noli
}

// Check validity of supported built-in tags.
if d.nextCBORType() == cborTypeTag {
off := d.off
off := d.off
for d.nextCBORType() == cborTypeTag {
_, _, tagNum := d.getHead()
if err := validBuiltinTag(tagNum, d.data[d.off]); err != nil {
d.skip()
return nil, err
}
d.off = off
}
d.off = off

t := d.nextCBORType()
switch t {
Expand Down Expand Up @@ -1445,7 +1502,8 @@ func (d *decoder) parse(skipSelfDescribedTag bool) (interface{}, error) { //noli
switch tagNum {
case 0, 1:
d.off = tagOff
return d.parseToTime()
tm, _, err := d.parseToTime()
return tm, err
case 2:
b, _ := d.parseByteString()
bi := new(big.Int).SetBytes(b)
Expand Down
45 changes: 39 additions & 6 deletions decode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3547,13 +3547,27 @@ func TestMapKeyNil(t *testing.T) {
}

func TestDecodeTime(t *testing.T) {
unmodified := time.Now()

testCases := []struct {
name string
cborRFC3339Time []byte
cborUnixTime []byte
wantTime time.Time
}{
// Decoding CBOR null/defined to time.Time is no-op. See TestUnmarshalNil.
// Decoding untagged CBOR null/defined to time.Time is no-op. See TestUnmarshalNil.
{
name: "null within unrecognized tag", // no-op in DecTagIgnored
cborRFC3339Time: hexDecode("dadeadbeeff6"),
cborUnixTime: hexDecode("dadeadbeeff6"),
wantTime: unmodified,
},
{
name: "undefined within unrecognized tag", // no-op in DecTagIgnored
cborRFC3339Time: hexDecode("dadeadbeeff7"),
cborUnixTime: hexDecode("dadeadbeeff7"),
wantTime: unmodified,
},
{
name: "NaN",
cborRFC3339Time: hexDecode("f97e00"),
Expand Down Expand Up @@ -3599,13 +3613,13 @@ func TestDecodeTime(t *testing.T) {
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
tm := time.Now()
tm := unmodified
if err := Unmarshal(tc.cborRFC3339Time, &tm); err != nil {
t.Errorf("Unmarshal(0x%x) returned error %v", tc.cborRFC3339Time, err)
} else if !tc.wantTime.Equal(tm) {
t.Errorf("Unmarshal(0x%x) = %v (%T), want %v (%T)", tc.cborRFC3339Time, tm, tm, tc.wantTime, tc.wantTime)
}
tm = time.Now()
tm = unmodified
if err := Unmarshal(tc.cborUnixTime, &tm); err != nil {
t.Errorf("Unmarshal(0x%x) returned error %v", tc.cborUnixTime, err)
} else if !tc.wantTime.Equal(tm) {
Expand Down Expand Up @@ -3675,6 +3689,7 @@ func TestDecodeTimeWithTag(t *testing.T) {
func TestDecodeTimeError(t *testing.T) {
testCases := []struct {
name string
opts DecOptions
data []byte
wantErrorMsg string
}{
Expand Down Expand Up @@ -3703,11 +3718,29 @@ func TestDecodeTimeError(t *testing.T) {
data: hexDecode("3bffffffffffffffff"),
wantErrorMsg: "cbor: cannot unmarshal negative integer into Go value of type time.Time",
},
{
name: "untagged byte string content cannot be decoded into time.Time with DefaultByteStringType string",
opts: DecOptions{
TimeTag: DecTagOptional,
DefaultByteStringType: reflect.TypeOf(""),
},
data: hexDecode("54323031332d30332d32315432303a30343a30305a"),
wantErrorMsg: "cbor: cannot unmarshal byte string into Go value of type time.Time",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reproduces #501 (on master, a nil error is returned instead).

},
{
name: "time tag is validated when enclosed in unrecognized tag",
data: hexDecode("dadeadbeefc001"),
wantErrorMsg: "cbor: tag number 0 must be followed by text string, got positive integer",
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
dm, err := tc.opts.DecMode()
if err != nil {
t.Fatal(err)
}
tm := time.Now()
if err := Unmarshal(tc.data, &tm); err == nil {
if err := dm.Unmarshal(tc.data, &tm); err == nil {
t.Errorf("Unmarshal(0x%x) didn't return an error, want error msg %q", tc.data, tc.wantErrorMsg)
} else if !strings.Contains(err.Error(), tc.wantErrorMsg) {
t.Errorf("Unmarshal(0x%x) returned error %q, want %q", tc.data, err.Error(), tc.wantErrorMsg)
Expand Down Expand Up @@ -3759,7 +3792,7 @@ func TestDecodeInvalidTagTime(t *testing.T) {
name: "Tag 1 with negative integer overflow",
data: hexDecode("c13bffffffffffffffff"),
decodeToTypes: []reflect.Type{typeIntf, typeTime},
wantErrorMsg: "cbor: cannot unmarshal tag into Go value of type time.Time",
wantErrorMsg: "cbor: cannot unmarshal negative integer into Go value of type time.Time (-18446744073709551616 overflows Go's int64)",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that overflow was causing parse to return math.Big in these cases, so the previous error came from the default case of the type switch in parseToTime.

},
{
name: "Tag 1 with string content",
Expand Down Expand Up @@ -3912,7 +3945,7 @@ func TestDecodeTimeStreaming(t *testing.T) {
},
{
data: hexDecode("c13bffffffffffffffff"),
wantErrorMsg: "cbor: cannot unmarshal tag into Go value of type time.Time",
wantErrorMsg: "cbor: cannot unmarshal negative integer into Go value of type time.Time (-18446744073709551616 overflows Go's int64)",
},
{
data: hexDecode("c11a514b67b0"),
Expand Down
Loading