[types] limit parsing of nested TypeTag #528

davidiw · 2022-10-02T20:10:02Z

Limit parsing of nested type tags in the type parser

language/move-core/types/src/parser.rs

language/move-core/types/src/language_storage.rs

davidiw

addressing other comments...

language/move-core/types/src/parser.rs

igor-aptos

looks good to me, don't have approval rights for this repo :)

igor-aptos · 2022-10-02T23:03:20Z

language/move-core/types/src/language_storage.rs

+#[cfg(test)]
+const MAX_DE_TYPE_TAG_NESTING: u8 = MAX_TYPE_TAG_NESTING - 1;


to confirm, this is there just so we can check that both serialization and deserialization fails, correct?

wanna add a comment for that?

dariorussi · 2022-10-02T23:33:58Z

quick question: when do you plan/want this to get in?
this is really something I would love @vgao1996 to review.
That can happen after too but I really think he has the best context to evaluate this.
In any case review coming soon....

dariorussi

it looks good to me but @vgao1996 please review as you have a chance tomorrow

wrwg

That's an awesome hack and re-engineering of parts of serde! However, I'm bit concerned about the complexity here (and the possibility of new bugs). Lets discuss offline.

wrwg · 2022-10-03T00:35:07Z

language/move-core/types/src/language_storage.rs

 }

 #[derive(Serialize)]
 enum SafeTypeTag<'a> {


Suggest to call this TypeTagForSerialize. There is nothing safe about it besides how you use it.

wrwg · 2022-10-03T00:37:43Z

language/move-core/types/src/language_storage.rs

    }
 }

+// u8 -> depth or nesting level


Are you trying to say that the u8 is the depth? Then please do so, the comment is cryptic otherwise

wrwg · 2022-10-03T01:24:03Z

language/move-core/types/src/language_storage.rs

+// u8 -> depth or nesting level
+struct TypeTagVisitor(u8);
+
+use serde::de::{


Could you please ensure to format before sending? I was spending a while wondering where your Visitor and other types come from, as I did not see them imported on the top of the file (where expected).

wrwg · 2022-10-03T01:28:57Z

language/move-core/types/src/language_storage.rs

+    self, DeserializeSeed, Deserializer, EnumAccess, MapAccess, SeqAccess, VariantAccess, Visitor,
+};
+
+impl<'de> Visitor<'de> for TypeTagVisitor {


I would be nice to see some commenting here, like

// Manual implementation of what serde would derive automatically, but with an injected bound. We are using this // here as currently no configurable type depth is supported by serde. See [link if there is something to see].

Or whatever makes sense. Otherwise, someone seeing this code here would just declare us as insane.

Also as you seem to have reverse engineered Serde, it would be nice you give some hints so others don't need to do the same as far as this is possible.

this is how I keep my job...

wrwg · 2022-10-03T01:31:01Z

language/move-core/types/src/language_storage.rs

+    where
+        A: EnumAccess<'de>,
+    {
+        let (variant_type, variant) = data.variant::<DeTypeTag>()?;


How do we get a match here of DeTypeTag vs the actual type? By position in the variant list of enum? If so it would be helpful if that has been documented with a short sentence at DeTypeTag.

wrwg · 2022-10-03T01:33:49Z

language/move-core/types/src/language_storage.rs

+                Ok(TypeTag::Signer)
+            }
+            DeTypeTag::Vector => Ok(TypeTag::Vector(Box::new(
+                variant.newtype_variant_seed(NestedTypeTag(self.0))?,


Why not just make self.0 be self.nest or something.

I hate extra code :)

wrwg · 2022-10-03T01:36:01Z

language/move-core/types/src/language_storage.rs

+    where
+        D: Deserializer<'de>,
+    {
+        if self.0 >= MAX_DE_TYPE_TAG_NESTING {


Perhaps move the condition and the error generation into some check_nest(self.0)? as it is used more than once.

It is used by two different objects, but I can see the point

wrwg · 2022-10-03T01:36:41Z

language/move-core/types/src/language_storage.rs

+            )));
+        }
+
+        deserializer.deserialize_seq(VecTypeTagVisitor(self.0))


self.0 + 1?

If this was actually omitted (and after you already had a review), consider what this means for in-depth defense. Those also have bugs.

at this point, we haven't really picked up a new type tag. the type tag will be incremented for each actual TypeTag we visit.

in other words... comment please

wrwg · 2022-10-03T01:37:51Z

language/move-core/types/src/language_storage.rs

+    }
+}
+
+const TYPE_TAG_VARIANTS: &[&str] = &["bool", "u8", "u64", "u128", "signer", "vector", "struct"];


Move the const decls to the front? This was used up somewhere already IIRC.

wrwg · 2022-10-03T01:38:31Z

language/move-core/types/src/language_storage.rs

+// u8 -> depth or nesting level
+struct NestedStructTag(u8);
+
+impl<'de> DeserializeSeed<'de> for NestedStructTag {


Move this closer to the struct stuff?

I suspect one could bikeshed a lot about the organization of this file. I might argue putting the public types at the top is more readable. I could also see an argument about relational placement.

wrwg · 2022-10-03T05:19:11Z

I've a potential alternative (simpler) solution. It involves thread locals. I hope I can send for comparison before EOD.

davidiw · 2022-10-03T05:23:19Z

I'd like to move fast here. If we can come up with a generalizable scheme, I'm all game. I do think that this is a rather safe PR and something that would definitely be beneficial upstream. It is a mess that serde doesn't support limited nesting at a struct level.

This is an alternative version of what move-language#528 and the previous for limitation of nesting provides. This approach installs custom serializers at the recursion points of the `TypeTag` enum. Those serializer use a thread local to track the nesting of the type tag, both for serialization and deserialization. The usage of a thread local is safe and a common technique to provide extra state for implementations which miss it (e.g. serde). A given thread has a single execution stack and this will always count the depth counter consistently up and down. Because Rust has no exceptions are other exit points, this cannot be corrupted. Possibly, thread locals may cause inefficiency. However, we expect TypeTags to be small (a few constructors, limited nestings). The technique used for type tags can be generalized for any other recursive type if needed. Eventually, we hope serde will support per-type nesting, and possibly we should talk with the author if we can add it ourselves. This uses and passes the tests from move-language#528.

Cherry picked from move-language#529. This is an alternative version of what move-language#528 and the previous for limitation of nesting provides, having a significant lower footprint. This approach installs custom serializers at the recursion points of the `TypeTag` enum. Those serializer use a thread local to track the nesting of the type tag, both for serialization and deserialization. The usage of a thread local is safe and a common technique to provide extra state for implementations which miss it (e.g. serde). A given thread has a single execution stack and this will always count the depth counter consistently up and down. Because Rust has no exceptions are other exit points, this cannot be corrupted. Possibly, thread locals may cause inefficiency. However, we expect TypeTags to be small (a few constructors, limited nestings). The technique used for type tags can be generalized for any other recursive type if needed. Eventually, we hope serde will support per-type nesting, and possibly we should talk with the author if we can add it ourselves. This uses and passes the tests from move-language#528.

Cherry picked from #529. This is an alternative version of what #528 and the previous for limitation of nesting provides, having a significant lower footprint. This approach installs custom serializers at the recursion points of the `TypeTag` enum. Those serializer use a thread local to track the nesting of the type tag, both for serialization and deserialization. The usage of a thread local is safe and a common technique to provide extra state for implementations which miss it (e.g. serde). A given thread has a single execution stack and this will always count the depth counter consistently up and down. Because Rust has no exceptions are other exit points, this cannot be corrupted. Possibly, thread locals may cause inefficiency. However, we expect TypeTags to be small (a few constructors, limited nestings). The technique used for type tags can be generalized for any other recursive type if needed. Eventually, we hope serde will support per-type nesting, and possibly we should talk with the author if we can add it ourselves. This uses and passes the tests from #528.

There's limited value and a lot of wasted resources by leaving this limited to serde's limits. Recursion is dangerous.

Cherry picked from move-language#529. This is an alternative version of what move-language#528 and the previous for limitation of nesting provides, having a significant lower footprint. This approach installs custom serializers at the recursion points of the `TypeTag` enum. Those serializer use a thread local to track the nesting of the type tag, both for serialization and deserialization. The usage of a thread local is safe and a common technique to provide extra state for implementations which miss it (e.g. serde). A given thread has a single execution stack and this will always count the depth counter consistently up and down. Because Rust has no exceptions are other exit points, this cannot be corrupted. Possibly, thread locals may cause inefficiency. However, we expect TypeTags to be small (a few constructors, limited nestings). The technique used for type tags can be generalized for any other recursive type if needed. Eventually, we hope serde will support per-type nesting, and possibly we should talk with the author if we can add it ourselves. This uses and passes the tests from move-language#528.

* [types] make type tag nesting limited There's limited value and a lot of wasted resources by leaving this limited to serde's limits. Recursion is dangerous. * [types] limit nesting of types via state (#532) Cherry picked from #529. This is an alternative version of what #528 and the previous for limitation of nesting provides, having a significant lower footprint. This approach installs custom serializers at the recursion points of the `TypeTag` enum. Those serializer use a thread local to track the nesting of the type tag, both for serialization and deserialization. The usage of a thread local is safe and a common technique to provide extra state for implementations which miss it (e.g. serde). A given thread has a single execution stack and this will always count the depth counter consistently up and down. Because Rust has no exceptions are other exit points, this cannot be corrupted. Possibly, thread locals may cause inefficiency. However, we expect TypeTags to be small (a few constructors, limited nestings). The technique used for type tags can be generalized for any other recursive type if needed. Eventually, we hope serde will support per-type nesting, and possibly we should talk with the author if we can add it ourselves. This uses and passes the tests from #528. * [types] limit parsing of nested TypeTag There's limited value and a lot of wasted resources by leaving this limited to serde's limits. Recursion is dangerous. Co-authored-by: David Wolinsky <isaac.wolinsky@gmail.com>

* [types] make type tag nesting limited There's limited value and a lot of wasted resources by leaving this limited to serde's limits. Recursion is dangerous. * [types] limit nesting of types via state (move-language#532) Cherry picked from move-language#529. This is an alternative version of what move-language#528 and the previous for limitation of nesting provides, having a significant lower footprint. This approach installs custom serializers at the recursion points of the `TypeTag` enum. Those serializer use a thread local to track the nesting of the type tag, both for serialization and deserialization. The usage of a thread local is safe and a common technique to provide extra state for implementations which miss it (e.g. serde). A given thread has a single execution stack and this will always count the depth counter consistently up and down. Because Rust has no exceptions are other exit points, this cannot be corrupted. Possibly, thread locals may cause inefficiency. However, we expect TypeTags to be small (a few constructors, limited nestings). The technique used for type tags can be generalized for any other recursive type if needed. Eventually, we hope serde will support per-type nesting, and possibly we should talk with the author if we can add it ourselves. This uses and passes the tests from move-language#528. * [types] limit parsing of nested TypeTag There's limited value and a lot of wasted resources by leaving this limited to serde's limits. Recursion is dangerous. Co-authored-by: David Wolinsky <isaac.wolinsky@gmail.com>

* [types] make type tag nesting limited There's limited value and a lot of wasted resources by leaving this limited to serde's limits. Recursion is dangerous. * [types] limit nesting of types via state (#532) Cherry picked from #529. This is an alternative version of what #528 and the previous for limitation of nesting provides, having a significant lower footprint. This approach installs custom serializers at the recursion points of the `TypeTag` enum. Those serializer use a thread local to track the nesting of the type tag, both for serialization and deserialization. The usage of a thread local is safe and a common technique to provide extra state for implementations which miss it (e.g. serde). A given thread has a single execution stack and this will always count the depth counter consistently up and down. Because Rust has no exceptions are other exit points, this cannot be corrupted. Possibly, thread locals may cause inefficiency. However, we expect TypeTags to be small (a few constructors, limited nestings). The technique used for type tags can be generalized for any other recursive type if needed. Eventually, we hope serde will support per-type nesting, and possibly we should talk with the author if we can add it ourselves. This uses and passes the tests from #528. * [types] limit parsing of nested TypeTag There's limited value and a lot of wasted resources by leaving this limited to serde's limits. Recursion is dangerous. Co-authored-by: David Wolinsky <isaac.wolinsky@gmail.com> Co-authored-by: David Wolinsky <isaac.wolinsky@gmail.com>

davidiw requested review from dariorussi, vgao1996 and wrwg as code owners October 2, 2022 20:10

gregnazario reviewed Oct 2, 2022

View reviewed changes

davidiw commented Oct 2, 2022

View reviewed changes

language/move-core/types/src/parser.rs Outdated Show resolved Hide resolved

language/move-core/types/src/parser.rs Show resolved Hide resolved

davidiw force-pushed the aptos branch from d0192fd to c8fa55c Compare October 2, 2022 21:11

gregnazario approved these changes Oct 2, 2022

View reviewed changes

igor-aptos reviewed Oct 2, 2022

View reviewed changes

dariorussi approved these changes Oct 3, 2022

View reviewed changes

wrwg reviewed Oct 3, 2022

View reviewed changes

wrwg mentioned this pull request Oct 3, 2022

[types] limit nesting of types via state #529

Closed

wrwg mentioned this pull request Oct 3, 2022

[types] limit nesting of types via state #532

Merged

davidiw force-pushed the aptos branch from c8fa55c to ffc3e7b Compare October 3, 2022 22:38

davidiw changed the title ~~[types] make type tag nesting limited in deserialize~~ [types] limit parsing of nested TypeTag Oct 3, 2022

davidiw enabled auto-merge (rebase) October 3, 2022 22:40

[types] limit parsing of nested TypeTag

c6ea826

There's limited value and a lot of wasted resources by leaving this limited to serde's limits. Recursion is dangerous.

davidiw force-pushed the aptos branch from ffc3e7b to c6ea826 Compare October 3, 2022 23:33

wrwg approved these changes Oct 3, 2022

View reviewed changes

davidiw merged commit d873870 into move-language:aptos Oct 3, 2022

wrwg mentioned this pull request Jan 3, 2023

Cherry picks TypeTag bounds from old aptos branch (#791) #793

Merged

		#[cfg(test)]
		const MAX_DE_TYPE_TAG_NESTING: u8 = MAX_TYPE_TAG_NESTING - 1;

[types] limit parsing of nested TypeTag #528

[types] limit parsing of nested TypeTag #528

Uh oh!

Conversation

davidiw commented Oct 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidiw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

igor-aptos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dariorussi commented Oct 2, 2022

Uh oh!

dariorussi left a comment

Choose a reason for hiding this comment

Uh oh!

wrwg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidiw Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wrwg commented Oct 3, 2022

Uh oh!

davidiw commented Oct 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidiw commented Oct 2, 2022 •

edited

Loading

davidiw Oct 3, 2022 •

edited

Loading