Specify storage limits, enforce when encoding, fix bignum encoding #370
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tead of registration
…ld version The encoding of negative bignums is specified in https://tools.ietf.org/html/rfc7049#section-2.4.2: "For tag value 3, the value of the bignum is -1 - n." Negative bignums were encoded incorrectly in version < 2, as just -n. Fix this by adjusting by one.
jordanschalm
approved these changes
Sep 29, 2020
zhangchiqing
approved these changes
Sep 29, 2020
| var encodedDictionaryValueEntriesFieldKey = cborFieldKey( | ||
| encodedDictionaryValue{}, | ||
| "Entries", | ||
| // NOTE: NEVER change, only add/increment; ensure uint64 |
There was a problem hiding this comment.
Suggested change
| // NOTE: NEVER change, only add/increment; ensure uint64 | |
| // NOTE: NEVER change, only add/increment; ensure uint64 | |
| // otherwise, we will have problem decoding those values which | |
| // were encoded as a different type. |
There was a problem hiding this comment.
This isn't about type confusion, but about confusion of the fields in a container
| Entries: entries, | ||
| return cbor.Tag{ | ||
| Number: cborTagDictionaryValue, | ||
| Content: cborMap{ |
There was a problem hiding this comment.
Do we still need this comment
There was a problem hiding this comment.
Not sure what you mean. You already suggested extending the comment above, but it's not gone.
Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
turbolent
commented
Sep 29, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #351
The CBOR library's decoder has limits on the maximum number of items in an array, the maximum number of elements in a map, and the maximum nesting level of containers. Increase these limits, as they are a bit on the low side and users have run into this limit before.
Also explicitly pin / specify the limits explicitly instead of relying on the defaults of the library (which might change).
The CBOR library's encoder does not enforce those limits, they are only used when decoding. To ensure Cadence is not encoding data that can not be decoded due to limits, validate the encoding result. This required exporting the validation function in the CBOR library: I opened an issue and a PR. This requires temporarily using a fork which has this change. I hope to switch back to the original as soon as the patch has landed upstream.
I initially planned to validate the encoded data by traversing the prepared data structure which is encoded, by keeping a depth counter. For that reason I refactored the dedicated struct types to just use
cbor.Tag. In addition, this made the key fields actual constants (instead of relying on reflection to read struct field tags).However, when implementing this, I found myself unsure if it would implement the same limit checks as the CBOR library's decoder, and eventually decided against this approach, and simply checking the encoded data for validity (which checks the decoding limits). I think this approach is simpler, as it doesn't complicate the encoding code, and doesn't potentially get out of sync with the CBOR library's decoding limit checks.
I think it's still worth keeping the
cbor.Tagrefactor, as I find it to be an improvement.Switching to the fork updated the library, which revealed a bug in the encoding of negative bignums.
The CBOR library originally didn't support encoding CBOR positive and negative bignums (tags 2 and 3), so I had implemented this according to the RFC.
The encoding of negative bignums is specified in http://tools.ietf.org/html/rfc7049#section-2.4.2:
"For tag value 3, the value of the bignum is -1 - n.".
However, I made a mistake and didn't adjust by one. This is really only a problem if the ledger data is read by another implementation which assumes the data is RFC-compliant CBOR. The data is correctly decoded though (as proven by tests), so this is not a problem for Flow / Cadence users.
Still, we should produce correct CBOR data. I therefore bumped the Cadence encoding version from 1 to 2 and added support for decoding data that was written in the old, incorrect format.