Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider migrating off anymap, it contains undefined behavior #306

Closed
erickt opened this issue May 7, 2021 · 1 comment · Fixed by #307
Closed

Consider migrating off anymap, it contains undefined behavior #306

erickt opened this issue May 7, 2021 · 1 comment · Fixed by #307
Milestone

Comments

@erickt
Copy link
Contributor

erickt commented May 7, 2021

It appears that the anymap crate contains some undefined behavior that has been fixed in the repository, but was never published to crates.io. Unfortunately it appears the project has since been abandoned (chris-morgan/anymap#37, rustsec/advisory-db#906).

I'd recommend migrating off the project, especially since there still may be an opportunity to make api breaking changes before 5.0.0 is released. Unfortunately there isn't an officially recommended solution. There's https://github.com/azriel91/anymap2, which is a fork that includes the fix, but I'd reconsider a more straightforward approach, where the event type uses serde-value to communicate arbitrary values.

struct Event {
    ...

    // Use an option-box to make it a single pointer sized
    pub attrs: Option<Box<EventAttrs>>,
}

struct EventAttrs {
    pub tracker: Tracker,
    pub info: Info,
    pub flag: Flag,
    pub extra: HashMap<String, serde_value::Value>,
}

Or if you want to still support arbitrary types, you could implement your own AnyMap, since TypeId is hashable.

@0xpr03 0xpr03 added this to the 5.0.0 milestone May 8, 2021
@erickt
Copy link
Contributor Author

erickt commented May 10, 2021

How important is it that the attrs field support arbitrary types? It looks like the type internally only stores strings and integer-like types into the field. This field is unfortunately a public property, so it'd be non-trivial to discover if downstream users are stuffing in other items into the attrs field.

erickt added a commit to erickt/notify that referenced this issue May 10, 2021
`anymap` has some undefined behavior, and appears to no longer be maintained.
This patch replaces anymap with an opaque `Attributes` type, which contains a
number of accessor methods to get and set values. Under the covers, this type
is simply a box of a struct of option types. However, since it's opaque the
underlying type could be swapped out with an alternative type that can be
extended to support more attributes without breaking users.

Closes notify-rs#306
erickt added a commit to erickt/notify that referenced this issue May 10, 2021
`anymap` has some undefined behavior, and appears to no longer be maintained.
This patch replaces anymap with an opaque `Attributes` type, which contains a
number of accessor methods to get and set values. Under the covers, this type
is simply a box of a struct of option types. However, since it's opaque the
underlying type could be swapped out with an alternative type that can be
extended to support more attributes without breaking users.

Closes notify-rs#306
@erickt erickt mentioned this issue May 10, 2021
erickt added a commit to erickt/notify that referenced this issue May 10, 2021
`anymap` has some undefined behavior, and appears to no longer be maintained.
This patch replaces anymap with an opaque `EventAttributes` type, which
contains a number of accessor methods to get and set values. Under the
covers, this type is simply a box of a struct of option types. However,
since it's opaque the underlying type could be swapped out with an
alternative type that can be extended to support more attributes without
breaking users.

Closes notify-rs#306
erickt added a commit to erickt/notify that referenced this issue May 10, 2021
`anymap` has some undefined behavior, and appears to no longer be maintained.
This patch replaces anymap with an opaque `EventAttributes` type, which
contains a number of accessor methods to get and set values. Under the
covers, this type is simply a box of a struct of option types. However,
since it's opaque the underlying type could be swapped out with an
alternative type that can be extended to support more attributes without
breaking users.

Closes notify-rs#306
erickt added a commit to erickt/notify that referenced this issue May 11, 2021
`anymap` has some undefined behavior, and appears to no longer be maintained.
This patch replaces anymap with an opaque `EventAttributes` type, which
contains a number of accessor methods to get and set values. Under the
covers, this type is simply a box of a struct of option types. However,
since it's opaque the underlying type could be swapped out with an
alternative type that can be extended to support more attributes without
breaking users.

Closes notify-rs#306
erickt added a commit to erickt/notify that referenced this issue May 11, 2021
0xpr03 pushed a commit that referenced this issue May 12, 2021
`anymap` has some undefined behavior, and appears to no longer be maintained.
This patch replaces anymap with an opaque `EventAttributes` type, which
contains a number of accessor methods to get and set values. Under the
covers, this type is simply a box of a struct of option types. However,
since it's opaque the underlying type could be swapped out with an
alternative type that can be extended to support more attributes without
breaking users.

Closes #306
0xpr03 pushed a commit that referenced this issue May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants