Add decode option to allow rejecting inputs that contain certain simple values. #481

benluddy · 2024-01-30T18:55:04Z

Description

Proof of concept implementation of #477.

PR Was Proposed and Welcomed in Currently Open Issue

This PR was proposed and welcomed by maintainer(s) in issue feature: Configurable handling of unrecognized simple values #477
Closes or Updates Issue feature: Configurable handling of unrecognized simple values #477

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.

fxamacker · 2024-02-20T03:02:06Z

@benluddy Thanks for this POC and discussion in issue #477. I focused on big picture since this has draft status.

If my understanding is correct, use cases are:

no custom handling of simple values (default and matches current behavior)
custom handling of all simple values
custom handling of some simple values, with default handling for all other simple values

The first two use cases are handled nicely in this POC since decoder uses either defaultSimpleValues or received SimpleValues.

However, the third use case seems error-prone because user needs to figure out how to trigger default behavior and apply it for all simple values which don't require custom handling. Maybe it can be more user friendly. Thoughts?

benluddy · 2024-02-27T17:15:37Z

However, the third use case seems error-prone because user needs to figure out how to trigger default behavior and apply it for all simple values which don't require custom handling. Maybe it can be more user friendly. Thoughts?

To be clear, this would support users who want to do something like reject the undefined simple value instead of treating it as nil, but otherwise preserve the existing treatment of all other simple values?

I will take a look at that. Maybe this sort of usage would be clearer if the option field had an opaque type, with some exported functionality to allow users to construct option values that effectively specify default "overrides" for only a few simple values.

benluddy · 2024-03-11T21:47:00Z

@fxamacker based on your feedback, I changed the option from a sparse map to a pointer to an opaque, immutable SimpleValueRegistry type. Again, if the option is nil, the existing behavior is retained. Users can construct a custom SimpleValueRegistry with NewSimpleValueRegistry:

no custom handling of simple values (default and matches current behavior)

SimpleValues: nil,

or

SimpleValues: NewSimpleValueRegistry(WithDefaultSimpleValueAnalogs),

custom handling of all simple values

SimpleValues: NewSimpleValueRegistry(
  // any simple value not registered will be rejected
  // this example will only recognize false and true
  WithSimpleValueAnalog(20, false),
  WithSimpleValueAnalog(21, true),
),

custom handling of some simple values, with default handling for all other simple values

SimpleValues: NewSimpleValueRegistry(
  WithDefaultSimpleValueAnalogs,
  WithNoSimpleValueAnalog(23), // reject undefined
  WithSimpleValueAnalog(200, some.Thing),
),

fxamacker · 2024-03-18T01:25:41Z

@benluddy Thanks for updating this PR! I'll take a closer look this week ~~(hopefully tomorrow)~~. 🙏

fxamacker

@benluddy Thanks for updating this PR! Looks great!

I left a few comments, e.g. need to forbid mutable SimpleValue analogs, provide an additional way to create SimpleValue registry (to reduce user mistakes), etc.

nit: Not sure, but would names be more obvious if "Analog" was replaced by "Substitute" or "Sub"? E.g. "SimpeValueAnalog" -> "SimpleValueSubstitute"

fxamacker · 2024-03-24T00:00:13Z

decode.go

+// SimpleValueRegistry is an immutable mapping from CBOR simple value number (0...23 and 32...255)
+// to Go analog value.
+type SimpleValueRegistry struct {
+	analogs [256]*interface{}


fxamacker · 2024-03-24T00:15:00Z

decode.go

+		if sv >= 24 && sv <= 31 {
+			return fmt.Errorf("cbor: cannot set analog for reserved simple value %d", sv)
+		}
+		r.analogs[sv] = &analog


We should probably deny mutable analogs. If analog is of type *int, the integer value can be modified after SimpleValueRegistry is created.

In addition to forbidding pointers, we could also forbid containers able to hold pointers. This allows us to avoid some code bloat (e.g. checking if any element/field or nested element/field is a pointer).

For simplicity, maybe we can require analogs to essentially be one of these:

nil

bool

string

integers

floats

Thoughts?

I'm not confident that the library should forbid this. As you implied, it's not trivial to pin down without making the limitations very strict.

If limited to only a few built-in types, it becomes hard for users to distinguish between a simple value and a different value with an equivalent Go representation. If the limitation is relaxed to be based on kind rather than type, then users could define a named type with one of the supported underlying types, and use an analog of that type to distinguish the simple value.

However, I think that would eventually bite users who register analogs that they don't entirely control. If a perfect analog value is provided by a library, users may be forced to perform a second pass over their decoded object to convert from a user-defined sentinel value to the analog that they want to appear in the final object. Alternatively, a library-provided value may change during a dependency update in such a way that it no longer meets our restrictions on acceptable analog values.

I don't have a use case for this presently, but I lean toward leaving users responsible for recognizing that their *int analog is the pointer itself and not the referenced value. Can you imagine this causing thorny issues down the line within this library? Map key equivalence is the main area that comes to my mind.

If limited to only a few built-in types, it becomes hard for users to distinguish between a simple value and a different value with an equivalent Go representation. If the limitation is relaxed to be based on kind rather than type, then users could define a named type with one of the supported underlying types, and use an analog of that type to distinguish the simple value.

I agree. We can restrict based on kind rather than type.

However, I think that would eventually bite users who register analogs that they don't entirely control. If a perfect analog value is provided by a library, users may be forced to perform a second pass over their decoded object to convert from a user-defined sentinel value to the analog that they want to appear in the final object. Alternatively, a library-provided value may change during a dependency update in such a way that it no longer meets our restrictions on acceptable analog values.

Maybe we can also allow users to register Unmarshaler to handle simple values, as supported for tags.

So user can:

perform decoding operation via UnmarshalCBOR for registered simple value if any, or

decode simple value to registered analog

This would give user the flexibility to handle more complex analog (pointers and containers) and also let user assume the responsibility for handling edge cases related to that.

Thoughts?

I don't have a use case for this presently, but I lean toward leaving users responsible for recognizing that their *int analog is the pointer itself and not the referenced value. Can you imagine this causing thorny issues down the line within this library? Map key equivalence is the main area that comes to my mind.

My main concern is potential concurrency bugs introduced by users if we allow pointers and containers that can hold pointers. Time spent troubleshooting bugs in 3rd party apps or libraries takes time away from improving this codec.

Maybe we can forbid pointers and containers in this PR and open a separate PR to add support for registering Unmarshaler for simple values, which would allow the user to implement support pointers, containers, etc.

decode.go

fxamacker · 2024-03-24T00:40:28Z

decode.go

+				return &UnmarshalTypeError{
+					CBORType: t.String(),
+					GoType:   v.Type().String(),
+					errorMsg: "simple value " + strconv.FormatInt(int64(val), 10) + " is not recognized",


Maybe return a new type of error (e.g. UnregisteredSimpleValueError) instead of reusing UnmarshalTypeError because:

UnmarshalTypeError is returned when CBOR data can't be decoded to specified Go type

The simple value error only happens when given simple value is explicitly not registered by users.

Yes, agreed. WDYT about a more general UnsupportedValueError that is returned when we recognize a valid input but are configured not to accept it? This sort of thing is mentioned in https://www.rfc-editor.org/rfc/rfc8949.html#section-5-3:

CBOR-based protocols MUST specify how their decoders handle invalid and other unexpected data. CBOR-based protocols MAY specify that they treat arbitrary valid data as unexpected. Encoders for CBOR-based protocols MUST produce only valid items, that is, the protocol cannot be designed to make use of invalid items. An encoder can be capable of encoding as many or as few types of values as is required by the protocol in which it is used; a decoder can be capable of understanding as many or as few types of values as is required by the protocols in which it is used.

I have a similar use case for rejecting inputs containing bignum tags, and I don't think applications would get much utility from having distinct error types for different inputs that the library supports but their application protocol does not.

decode.go

benluddy · 2024-03-26T17:04:01Z

Thanks for the thorough review @fxamacker, I think I've made changes to address all except one of the inline comments. I held off on changing #481 (comment) for the moment. If you feel strongly about it then I'll go ahead and implement the change you suggested (that decision is not critical to any use case I have).

nit: Not sure, but would names be more obvious if "Analog" was replaced by "Substitute" or "Sub"? E.g. "SimpleValueAnalog" -> "SimpleValueSubstitute"

For me "analog" is a closer fit because there is a 1:1 correspondence between a simple value and a unique Go value. "Substitute" might imply that there is a natural representation of any given simple value that the substitute is replacing, but only a few of the simple values (true, false, and maybe null) have obvious Go representations. I'll defer to your judgment on this as well, please let me know if you would prefer to see the terminology changed.

fxamacker

Thanks @benluddy for updating this PR! It looks great!

For me "analog" is a closer fit because there is a 1:1 correspondence between a simple value and a unique Go value. "Substitute" might imply that there is a natural representation of any given simple value that the substitute is replacing, but only a few of the simple values (true, false, and maybe null) have obvious Go representations. I'll defer to your judgment on this as well, please let me know if you would prefer to see the terminology changed.

Thanks for describing the reasons for using the term "analog", it makes sense, lets keep your preferred terminology! 👍

I held off on changing #481 (comment) for the moment. If you feel strongly about it then I'll go ahead and implement the change you suggested (that decision is not critical to any use case I have).

I prefer being more restrictive in built-in decoding functions (forbidding containers and pointers as analogs to SimpleValue) and allowing users to implement support for containers, pointers, etc. by registering Unmarshaler for simple values if they need it.

By forbidding containers and pointers as analogs for simple values:

We avoid making it easier for users to introduce concurrency bugs. This helps me limit time spent troubleshooting concurrency bugs in 3rd-party apps and libraries when users suggest the bug is in this codec.
We avoid premature code bloat from checking nested elements of containers, etc.
We can open separate PR to allow users to register Unmarshaler for simple values so users can implement support for all kinds of types including containers and pointers.

Thoughts?

fxamacker · 2024-03-31T19:07:24Z

decode.go

+// Creates a new SimpleValueRegistry. The registry state is initialized by executing the provided
+// functions in order against an empty registry. Any simple value without a registered analog will
+// produce an UnacceptableDataItemError if encountered in the input while unmarshaling.


nit: maybe add function name to comment.

Suggested change

// Creates a new SimpleValueRegistry. The registry state is initialized by executing the provided

// functions in order against an empty registry. Any simple value without a registered analog will

// produce an UnacceptableDataItemError if encountered in the input while unmarshaling.

// NewSimpleValueRegistry creates a new SimpleValueRegistry. The registry state is initialized by executing the provided

// functions in order against an empty registry. Any simple value without a registered analog will

// produce an UnacceptableDataItemError if encountered in the input while unmarshaling.

fxamacker · 2024-03-31T19:07:43Z

decode.go

+// Creates a new SimpleValueRegistry. The registry state is initialized by executing the provided
+// functions in order against a registry that is pre-populated with the library defaults.


nit: maybe add function name to comment.

Suggested change

// Creates a new SimpleValueRegistry. The registry state is initialized by executing the provided

// functions in order against a registry that is pre-populated with the library defaults.

// NewSimpleValueRegistryFromDefaults creates a new SimpleValueRegistry. The registry state is initialized by executing the provided

// functions in order against a registry that is pre-populated with the library defaults.

decode.go

fxamacker · 2024-03-31T23:57:00Z

decode.go

+		if sv >= 24 && sv <= 31 {
+			return fmt.Errorf("cbor: cannot set analog for reserved simple value %d", sv)
+		}
+		r.analogs[sv] = &analog


If limited to only a few built-in types, it becomes hard for users to distinguish between a simple value and a different value with an equivalent Go representation. If the limitation is relaxed to be based on kind rather than type, then users could define a named type with one of the supported underlying types, and use an analog of that type to distinguish the simple value.

I agree. We can restrict based on kind rather than type.

However, I think that would eventually bite users who register analogs that they don't entirely control. If a perfect analog value is provided by a library, users may be forced to perform a second pass over their decoded object to convert from a user-defined sentinel value to the analog that they want to appear in the final object. Alternatively, a library-provided value may change during a dependency update in such a way that it no longer meets our restrictions on acceptable analog values.

Maybe we can also allow users to register Unmarshaler to handle simple values, as supported for tags.

So user can:

perform decoding operation via UnmarshalCBOR for registered simple value if any, or

decode simple value to registered analog

This would give user the flexibility to handle more complex analog (pointers and containers) and also let user assume the responsibility for handling edge cases related to that.

Thoughts?

I don't have a use case for this presently, but I lean toward leaving users responsible for recognizing that their *int analog is the pointer itself and not the referenced value. Can you imagine this causing thorny issues down the line within this library? Map key equivalence is the main area that comes to my mind.

My main concern is potential concurrency bugs introduced by users if we allow pointers and containers that can hold pointers. Time spent troubleshooting bugs in 3rd party apps or libraries takes time away from improving this codec.

Maybe we can forbid pointers and containers in this PR and open a separate PR to add support for registering Unmarshaler for simple values, which would allow the user to implement support pointers, containers, etc.

benluddy · 2024-04-01T14:19:19Z

Maybe we can forbid pointers and containers in this PR and open a separate PR to add support for registering Unmarshaler for simple values, which would allow the user to implement support pointers, containers, etc.

I like that direction.

If support is eventually added for user-provided simple value unmarshalers, and we also implement limited support for certain simple value analog types today, there will ultimately be several ways to configure the same behaviors. My use cases don't require user-provided analogs at all, only the ability to reject simple values other than true, false, and null. Would you accept a more limited option for today that allows users to configure only which simple values their protocol rejects, without touching how accepted simple values are decoded? Given the discussion on this PR, I don't want to be too hasty to commit to a decision on how users provide their own analogs, but I think a simple value allowlist option and new error type would be uncontroversial.

fxamacker · 2024-04-02T01:42:09Z

Would you accept a more limited option for today that allows users to configure only which simple values their protocol rejects, without touching how accepted simple values are decoded? Given the discussion on this PR, I don't want to be too hasty to commit to a decision on how users provide their own analogs, but I think a simple value allowlist option and new error type would be uncontroversial.

@benluddy Yes, that sounds good to me! 👍

benluddy · 2024-04-02T16:45:29Z

Thanks for the great discussion on this feature @fxamacker!

I updated the PR so that users can reject arbitrary simple values but cannot change how any simple value unmarshals to a Go value. The option's API surface has roughly the same shape (only smaller) and should allow for the registration of custom simple value unmarshalers in the future. Please take another look when you get the chance.

Applications can use the decode option SimpleValues to override the default unmarshal behavior on a per-simple-value basis. The option is open to future extension, and currently supports the choice between the default backwards-compatible behavior (true as true, false as false, null as nil, and undefined as nil, and cbor.SimpleValue(N) for each currently-unregistered but well-formed simple value number N) and outright rejection with the new error type UnacceptableDataItemError. Signed-off-by: Ben Luddy <bluddy@redhat.com>

fxamacker

@benluddy Nice! 👍 Thanks for updating this PR and great discussions!

benluddy mentioned this pull request Jan 30, 2024

feature: Configurable handling of unrecognized simple values #477

Closed

benluddy force-pushed the simplevalue-govalues branch 3 times, most recently from 2208a23 to 2f65e3b Compare March 11, 2024 21:34

benluddy marked this pull request as ready for review March 11, 2024 21:46

fxamacker reviewed Mar 24, 2024

View reviewed changes

benluddy force-pushed the simplevalue-govalues branch from 2f65e3b to 58cd838 Compare March 26, 2024 16:48

benluddy requested a review from fxamacker March 26, 2024 17:04

fxamacker reviewed Apr 1, 2024

View reviewed changes

benluddy force-pushed the simplevalue-govalues branch from 58cd838 to c263dcd Compare April 2, 2024 16:29

benluddy changed the title ~~Option to set arbitrary simple value to Go value mappings.~~ Add decode option to allow rejecting inputs containing certain simple values. Apr 2, 2024

benluddy changed the title ~~Add decode option to allow rejecting inputs containing certain simple values.~~ Add decode option to allow rejecting inputs that contain certain simple values. Apr 2, 2024

benluddy force-pushed the simplevalue-govalues branch from c263dcd to 8639f7c Compare April 2, 2024 16:33

benluddy requested a review from fxamacker April 2, 2024 16:37

benluddy force-pushed the simplevalue-govalues branch from 8639f7c to 28078a7 Compare April 2, 2024 18:52

fxamacker approved these changes Apr 7, 2024

View reviewed changes

fxamacker merged commit 9f099e8 into fxamacker:master Apr 7, 2024
17 checks passed

fxamacker added the enhancement New feature or request label Apr 22, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add decode option to allow rejecting inputs that contain certain simple values. #481

Add decode option to allow rejecting inputs that contain certain simple values. #481

benluddy commented Jan 30, 2024 •

edited

Loading

fxamacker commented Feb 20, 2024

benluddy commented Feb 27, 2024

benluddy commented Mar 11, 2024

fxamacker commented Mar 18, 2024 •

edited

Loading

fxamacker left a comment

fxamacker Mar 24, 2024

fxamacker Mar 24, 2024

benluddy Mar 25, 2024

fxamacker Mar 31, 2024

fxamacker Mar 24, 2024

benluddy Mar 26, 2024

benluddy commented Mar 26, 2024

fxamacker left a comment

fxamacker Mar 31, 2024

fxamacker Mar 31, 2024

fxamacker Mar 31, 2024

benluddy commented Apr 1, 2024

fxamacker commented Apr 2, 2024

benluddy commented Apr 2, 2024

fxamacker left a comment

		// Creates a new SimpleValueRegistry. The registry state is initialized by executing the provided
		// functions in order against a registry that is pre-populated with the library defaults.

Add decode option to allow rejecting inputs that contain certain simple values. #481

Add decode option to allow rejecting inputs that contain certain simple values. #481

Conversation

benluddy commented Jan 30, 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

fxamacker commented Feb 20, 2024

benluddy commented Feb 27, 2024

benluddy commented Mar 11, 2024

fxamacker commented Mar 18, 2024 • edited Loading

fxamacker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benluddy commented Mar 26, 2024

fxamacker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benluddy commented Apr 1, 2024

fxamacker commented Apr 2, 2024

benluddy commented Apr 2, 2024

fxamacker left a comment

Choose a reason for hiding this comment

benluddy commented Jan 30, 2024 •

edited

Loading

fxamacker commented Mar 18, 2024 •

edited

Loading