[types] make type tag nesting limited #526
Conversation
| fn make_type_tag_struct(type_param: TypeTag) -> TypeTag { | ||
| TypeTag::Struct(StructTag { | ||
| address: AccountAddress::ONE, | ||
| module: Identifier::from_utf8(("a".as_bytes()).to_vec()).unwrap(), |
There was a problem hiding this comment.
just for future reference and I can only imagine your "care factor" here, but if you want to save your beautiful fingers a bit more you can do Identifier::new("a").unwrap().
I know, I know....
| #[serde(rename = "struct", alias = "Struct")] | ||
| Struct(StructTag), | ||
| } | ||
|
|
||
| impl From<&TypeTag> for SafeTypeTag { | ||
| fn from(type_tag: &TypeTag) -> Self { | ||
| match &type_tag { | ||
| TypeTag::Bool => SafeTypeTag::Bool, | ||
| TypeTag::U8 => SafeTypeTag::U8, | ||
| TypeTag::U64 => SafeTypeTag::U64, | ||
| TypeTag::U128 => SafeTypeTag::U128, | ||
| TypeTag::Address => SafeTypeTag::Address, | ||
| TypeTag::Signer => SafeTypeTag::Signer, | ||
| TypeTag::Vector(value) => SafeTypeTag::Vector(value.clone()), | ||
| TypeTag::Struct(value) => SafeTypeTag::Struct(value.clone()), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Serialize for TypeTag { | ||
| fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error> | ||
| where | ||
| S: Serializer, | ||
| { | ||
| match &self { | ||
| TypeTag::Vector(type_tag) => { | ||
| validate_type_tag_recursion(vec![(**type_tag).clone()].as_slice(), 1) | ||
| } | ||
| TypeTag::Struct(value) => validate_type_tag_recursion(&value.type_params, 1), | ||
| _ => Ok(()), | ||
| } | ||
| .map_err(serde::ser::Error::custom)?; | ||
|
|
||
| serializer.serialize_newtype_struct("TypeTag", &SafeTypeTag::from(self)) | ||
| } | ||
| } |
There was a problem hiding this comment.
so let me understand this and see if this is what you intended.
Here we check that the top level struct is properly nested (up to max) and then we go on to serialize SafeTypeTag which contains a StructTag and we reenter this function for n-1 levels recursively for all nested level, which are obviously correct because we already checked the top level.
That seems a bit too much?
I understand the alternatives may be a bit on the annoying side of life (though not sure, are they?) but asking to see if that was the intention or not.
Of course given the check we reenter at most sum(1..8) (or sum(1.. MAX_TYPE_TAG_NESTING )) so not a big deal... let me know if you want this in anyway
There was a problem hiding this comment.
So the issue here is that even if I build out a proper custom serializer, I will inevitably be left playing a very miserable game of unwrap:
TypeTag::Vector(ref value) => serializer.serialize_newtype_variant("TypeTag", 6, "vector", &value)
which if value is a typetag as well, then it will just end up trying to serialize it within this method again
same with struct tag containing a typetag
I've made a complete wrapper around it, so that we only go into the recursion three times, once to count, once to convert, and once to serialize. I'd prefer not to do further optimizations at this point, because we still have deserialization. We could theoretically remove one of those by doing serialization time conversions.
|
@dariorussi I've updated the PR to be a bit more optimal. |
There's limited value and a lot of wasted resources by leaving this limited to serde's limits. Recursion is dangerous.
dariorussi
left a comment
dariorussi
left a comment
There was a problem hiding this comment.
fwiw I would have been fine with the other one as well given how small is the number of recursion and the importance of stopping crazy recursion on crazy nested types.
I just wanted to make sure that was the intent.
Love and cheese brother
Hahaha, next time, next time |
3 participants
There's limited value and a lot of wasted resources by leaving this limited to serde's limits. Recursion is dangerous.
Motivation
(Write your motivation for proposed changes here.)
Have you read the Contributing Guidelines on pull requests?
(Write your answer here.)
Test Plan
(Share your test plan here. If you changed code, please provide us with clear instructions for verifying that your changes work.)