near / borsh-rs

Rust implementation of Binary Object Representation Serializer for Hashing
https://borsh.io/
Apache License 2.0
287 stars 62 forks source link

Restriction on Serializing Zero-Sized Types Affects Marker Component Usage #259

Closed cybersoulK closed 8 months ago

cybersoulK commented 8 months ago
Err(Custom { kind: InvalidData, error: "Collections of zero-sized types are not allowed due to deny-of-service concerns on deserialization." })

The error detects a ECS component in my game (struct Dynamic) that works as a marker. i have many, and they are very useful components.

This change makes it unusable for me... :(

https://github.com/near/borsh-rs/blob/903601b26a1fc11ead79a62c45b77faeedf7e6a1/borsh/src/error.rs#L3

dj8yfo commented 8 months ago

@cybersoulK , we've decided that using zero-sized types as keys in HashSet/HashMap is impractical, as there cannot be more that one instance of the type (and, transitively, more than one key in collection of this type).

Please share a minimal excerpt from your code to illustrate how this case is useful for you. Ideally, if you shared it as a compilable repo with a previous version of borsh.

cybersoulK commented 8 months ago

honestly i am having a hard time replicating it manually.

I thought it was

struct Marker;
enum Enum {
    Marker(Marker)
    SomethingElse(f32, ...)
}
Vec<Enum>

but i am able to run that successfuly, is there a way to print the exact type that's causing the issue?

cybersoulK commented 8 months ago

yep, i cannot see any Hashmap Key, or Vec Value that is an empty struct. i looked 10 times at the data

cybersoulK commented 8 months ago

Ok, i got it:

#[Component]
pub struct Cooldowns(pub HashMap<Skill, Interval>);

#[Component]
pub enum Skill {
    Dash,
}

where right now, Skill is only one variant.

I feel relieved that my assumption of the markers was wrong. But still, why ban an enum from being a key, if it only has one variant?

dj8yfo commented 8 months ago

@cybersoulK , this is probably due to an optimization from compiler, where it makes single-variant enum zero-size. https://github.com/rust-lang/rust/issues/37649 . Can likely be helped with adding #[repr(u8)] attribute to the definition or just adding another variant.

cybersoulK commented 8 months ago

Thank you ❤️, #[repr(u8)] should be good enough. If there was anything can could be done is Borsh Derive to correct it automatically, would be nice.

cybersoulK commented 8 months ago

oh, congrats coming a long way with borsh 1.2!