Repository: fxamacker/cbor
Pull request #503 (2024-03-05 23:29:36)

Decouple time.Time parsing from empty interface behavior.
<!--
Thank you for your interest in contributing to fxamacker/cbor!
-->

### Description


<!-- For code contributions, please complete all the items below this line. -->
<!-- For documentation-only contributions, please delete everything below this line. -->

#### PR Was Proposed and Welcomed in Currently Open Issue

- [x] This PR was proposed and welcomed by maintainer(s) in issue #501
- [x] Closes or Updates Issue #501

#### Checklist (for code PR only, ignore for docs PR)

- [x] Include unit tests that cover the new code
- [x] Pass all unit tests 
- [x] Pass all 18 ci linters (golint, gosec, staticcheck, etc.)
- [x] Sign each commit with your real name and email.  
      Last line of each commit message should be in this format:  
      Signed-off-by: Firstname Lastname <firstname.lastname@example.com>
- [x] Certify the Developer's Certificate of Origin 1.1
      (see next section).

#### Certify the Developer's Certificate of Origin 1.1

- [x] By marking this item as completed, I certify 
      the Developer Certificate of Origin 1.1.

```
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
660 York Street, Suite 102,
San Francisco, CA 94110 USA

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
```



benluddy: @fxamacker Thanks for the discussion in https://github.com/fxamacker/cbor/pull/503#discussion_r1513768915! I've added tests for those edge cases and resolved them according to your suggested behaviors. PTAL!

Patch:
diff --git a/decode.go b/decode.go
index e31ff7e..c2b5e00 100644
--- a/decode.go
+++ b/decode.go
@@ -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.
 //
 // To unmarshal CBOR null (0xf6) and undefined (0xf7) values into a
 // slice/map/pointer, Unmarshal sets Go value to nil.  Because null is often
@@ -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.
 	TimeTag DecTagMode
 
 	// MaxNestedLevels specifies the max nested levels allowed for any combination of CBOR array, maps, and tags.
@@ -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 {
@@ -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)
@@ -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()
+		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()}
 	}
 }
 
@@ -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 {
@@ -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)


Test patch:
diff --git a/decode_test.go b/decode_test.go
index 5fcea78..54da82f 100644
--- a/decode_test.go
+++ b/decode_test.go
@@ -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"),
@@ -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) {
@@ -3675,6 +3689,7 @@ func TestDecodeTimeWithTag(t *testing.T) {
 func TestDecodeTimeError(t *testing.T) {
 	testCases := []struct {
 		name         string
+		opts         DecOptions
 		data         []byte
 		wantErrorMsg string
 	}{
@@ -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",
+		},
+		{
+			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)
@@ -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)",
 		},
 		{
 			name:          "Tag 1 with string content",
@@ -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"),