Skip to content
This repository was archived by the owner on Aug 2, 2022. It is now read-only.

fix bugs in abi_serializer variant to binary #5680

Merged
merged 1 commit into from
Sep 14, 2018

Conversation

arhag
Copy link
Contributor

@arhag arhag commented Sep 14, 2018

Changes to abi_serializer:

  • Throw if trying to serialize a variant type other than an array or an object into a struct type. This includes the change by @taokayan from PR throw exception when serializing struct with incompatiable type 5629 #5663. This bug fix should solve cases (such as the one in abi_json_to_bin for Action newaccount Returns Incorrect Value #5629) where in incorrect JSON is not rejected and invalid binary is generated.
  • When converting a JSON array into a struct, the code now enforces that there are enough elements in the array to convert into all of the required fields of the struct. This prevents other ways of generating unexpected binary output when the abi_serializer succeeds in default constructing the omitted fields. Furthermore, an inappropriate if (va.size() > 0) check was removed from abi_serializer::_variant_to_binary since it caused a bug in which passing in an empty JSON array as the input for a struct causes the serialization of that struct to be entirely skipped.
  • Finally, an unnecessary call to _variant_to_binary prior to throwing a pack_exception was removed. This happened when a required field of a struct was missing from the JSON. In the best case, the unnecessary call meant extra work was done before throwing the appropriate error. In the worst case, the serialization would fail with a misleading error message.

Three new unit tests were added to test the cases related to the above bug fixes.

Also, the abi_tests/abi_recursive_structs unit test needed to be modified because it was relying on buggy behavior (which is now fixed in this PR) to generate the binary that it then feeds into the binary_to_variant call that would infinitely recurse (assuming no recursion limits or deadlines).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants