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

benluddy · 2024-03-05T23:29:36Z

Description

PR Was Proposed and Welcomed in Currently Open Issue

This PR was proposed and welcomed by maintainer(s) in issue Bug: decoding CBOR byte string into time.Time is allowed if decoding option DefaultByteStringType is set to reflect.TypeOf("") #501
Closes or Updates Issue Bug: decoding CBOR byte string into time.Time is allowed if decoding option DefaultByteStringType is set to reflect.TypeOf("") #501

Checklist (for code PR only, ignore for docs PR)

Include unit tests that cover the new code
Pass all unit tests
Pass all 18 ci linters (golint, gosec, staticcheck, etc.)
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
Certify the Developer's Certificate of Origin 1.1
(see next section).

Certify the Developer's Certificate of Origin 1.1

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 · 2024-03-05T23:31:49Z

decode_test.go

@@ -3759,7 +3773,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)",


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.

benluddy · 2024-03-05T23:32:36Z

decode_test.go

+				DefaultByteStringType: reflect.TypeOf(""),
+			},
+			data:         hexDecode("54323031332d30332d32315432303a30343a30305a"),
+			wantErrorMsg: "cbor: cannot unmarshal byte string into Go value of type time.Time",


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

fxamacker

Thanks for opening this PR! Looks great!

I left a comment about the need to handle decoding CBOR null and CBOR undefined to time.Time{}.

fxamacker · 2024-03-06T03:17:47Z

decode.go

+			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{}, &UnmarshalTypeError{
+					CBORType: t.String(),
+					GoType:   typeTime.String(),
+					errorMsg: fmt.Sprintf("-18446744073709551616 overflows Go's int64"),
+				}
+			}


Thanks for preventing overflow 👍

decode.go

benluddy · 2024-03-11T17:10:31Z

@fxamacker Thanks for the discussion in #503 (comment)! I've added tests for those edge cases and resolved them according to your suggested behaviors. PTAL!

fxamacker

Thanks for updating this PR and adding detailed documentation!

I left a comment related to skipping data before returning from parseToTime().

decode.go

fxamacker · 2024-03-13T00:13:18Z

decode.go

+// and fractional seconds since January 1, 1970 UTC. As a special case, Infinite
+// and NaN float values decode to time.Time's zero value.


Thank you for adding the comment about special case! 👍

fxamacker · 2024-03-13T00:15:16Z

decode.go

+	// 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.


Thanks so much for improving the documentation! 👍

Due to an implementation detail, options controlling decodes into empty interface values could in some cases affect the behavior of decoding into time.Time. That is fixed. This patch also clarifies the documentation and reconciles a couple edge-case behaviors: 1. In TimeTag mode DecTagIgnored, decoding a null or undefined simple value enclosed in a tag other than 0 or 1 to time.Time is now a no-op. This is the same as the existing behavior when decoding an untagged null or undefined. 2. The content type enclosed by tags 0 and 1 were not being validated if enclosed within an unrecognized tag. This has been fixed. Signed-off-by: Ben Luddy <bluddy@redhat.com>

fxamacker

Looks great! Thanks so much for contributing this PR! 👍

benluddy commented Mar 5, 2024

View reviewed changes

This was referenced Mar 6, 2024

feature: Allow decoding a byte string into time.Time value #497

Closed

feature: Add TimeTagToAny option to produce RFC3339 when decoding time into interface{} #496

Closed

fxamacker reviewed Mar 6, 2024

View reviewed changes

fxamacker added the bugfix label Mar 7, 2024

benluddy force-pushed the decode-into-time-interface-option-side-effects branch 2 times, most recently from 42e1c7c to c6e1f39 Compare March 11, 2024 17:05

benluddy requested a review from fxamacker March 11, 2024 17:07

fxamacker reviewed Mar 13, 2024

View reviewed changes

benluddy force-pushed the decode-into-time-interface-option-side-effects branch from c6e1f39 to 865ac96 Compare March 13, 2024 13:05

fxamacker approved these changes Mar 14, 2024

View reviewed changes

fxamacker merged commit a35fe34 into fxamacker:master Mar 14, 2024
17 checks passed

fxamacker mentioned this pull request Mar 14, 2024

Bug: decoding CBOR byte string into time.Time is allowed if decoding option DefaultByteStringType is set to reflect.TypeOf("") #501

Closed

fxamacker added this to the v2.7.0 milestone Apr 22, 2024

benluddy mentioned this pull request Apr 24, 2024

Add a decoding option to allow decoding byte string into time.Time. #524

Merged

8 tasks

benluddy mentioned this pull request May 30, 2024

Using ByteSliceMode (unreleased new feature) can encode invalid CBOR data #545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

benluddy commented Mar 5, 2024 •

edited

Loading

benluddy Mar 5, 2024

benluddy Mar 5, 2024

fxamacker left a comment •

edited

Loading

fxamacker Mar 6, 2024

benluddy commented Mar 11, 2024

fxamacker left a comment

fxamacker Mar 13, 2024

fxamacker Mar 13, 2024

fxamacker left a comment

		// and fractional seconds since January 1, 1970 UTC. As a special case, Infinite
		// and NaN float values decode to time.Time's zero value.

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

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

Conversation

benluddy commented Mar 5, 2024 • edited Loading

Description

PR Was Proposed and Welcomed in Currently Open Issue

Checklist (for code PR only, ignore for docs PR)

Certify the Developer's Certificate of Origin 1.1

benluddy Mar 5, 2024

Choose a reason for hiding this comment

benluddy Mar 5, 2024

Choose a reason for hiding this comment

fxamacker left a comment • edited Loading

Choose a reason for hiding this comment

fxamacker Mar 6, 2024

Choose a reason for hiding this comment

benluddy commented Mar 11, 2024

fxamacker left a comment

Choose a reason for hiding this comment

fxamacker Mar 13, 2024

Choose a reason for hiding this comment

fxamacker Mar 13, 2024

Choose a reason for hiding this comment

fxamacker left a comment

Choose a reason for hiding this comment

benluddy commented Mar 5, 2024 •

edited

Loading

fxamacker left a comment •

edited

Loading