[WIP] [Help wanted] Adapt to upcoming changes in Abomonation #11
Conversation
| @@ -14,38 +14,51 @@ fn derive_abomonation(mut s: synstructure::Structure) -> proc_macro2::TokenStrea | |||
| }); | |||
|
|
|||
| let entomb = s.each(|bi| quote! { | |||
| ::abomonation::Abomonation::entomb(#bi, _write)?; | |||
| ::abomonation::Entomb::entomb(#bi, _write)?; | |||
There was a problem hiding this comment.
Abomonation API change from TimelyDataflow/abomonation#28: The Abomonation trait is now API sugar for two separate traits, Entomb and Exhume, via a blanket impl which ensures that everything which implements those two traits implements Abomonation. This was done to allow deserialization of types which contain references, which requires adding a lifetime parameter to the deserialization trait (as in serde).
The semantics of Exhume<'de>'s lifetime parameter is that if T: Exhume<'de>, it means that it is okay to deserialize an &'de T from &'de mut [u8] bytes. This is not always the case for references, for example you don't want to allow people to deserialize &'static Something from stack-allocated bytes.
src/lib.rs
Outdated
| }); | ||
|
|
||
| s.bind_with(|_| synstructure::BindStyle::RefMut); | ||
|
|
||
| let exhume = s.each(|bi| quote! { | ||
| let temp = bytes; | ||
| bytes = ::abomonation::Abomonation::exhume(#bi, temp)?; | ||
| bytes = ::abomonation::Exhume::exhume(From::from(#bi), bytes)?; |
There was a problem hiding this comment.
Abomonation API change from TimelyDataflow/abomonation#22: exhume() now expects NonNull<Self> instead of &mut self. That is because exhume() receives an invalid Self as input (it has dangling pointers/references), and creating an &mut to invalid data is UB. Raw pointers should be used whenever possible in those situations.
| }); | ||
|
|
||
| s.bound_impl(quote!(abomonation::Abomonation), quote! { | ||
| #[inline] unsafe fn entomb<W: ::std::io::Write>(&self, _write: &mut W) -> ::std::io::Result<()> { |
There was a problem hiding this comment.
I removed the inline directives since I established in TimelyDataflow/abomonation#24 that they are actually very rarely needed. It's probably better to let the compiler's optimizer do its job if it does it reasonably well.
| extern crate abomonation; | ||
| extern crate std; | ||
|
|
||
| gen unsafe impl abomonation::Entomb for @Self { |
There was a problem hiding this comment.
Abomonation API change from TimelyDataflow/abomonation#25: implementing Abomonation traits is now unsafe, since an incorrect implementation (e.g. forgetting to patch a pointer) can lead to memory unsafety.
| ) -> Option<&'de mut [u8]> { | ||
| // FIXME: This (briefly) constructs an &mut _ to invalid data | ||
| // (via "ref mut"), which is UB. The proposed &raw mut | ||
| // operator would allow avoiding this. |
There was a problem hiding this comment.
As the FIXME says, this operation cannot be implemented without UB in Rust, for now, but there is a new language feature on the way that should fix that.
| Some(bytes) | ||
|
|
||
| gen unsafe impl<'de> abomonation::Exhume<'de> for @Self | ||
| where Self: 'de, |
There was a problem hiding this comment.
synstructure help wanted here to fix a memory safety bug: assuming that Self is Generic<'a, 'b, 'c>, I would like to enforce that abomonation::Exhume<'de> is only implemented for Generic<'de, 'de, 'de>.
This where clase does not fully do what I want, because it allows Self to be e.g. &'static Something when 'de is shorter-lived than 'static, which is wrong and can lead to memory unsafety through deserialization of a fake &'static T that points into short-lived stack-allocated bytes followed by deallocation of said bytes.
A 'de: 'self bound (where 'self is the lifetime of Self) is not enough either, because abomonation::decode() will ultimately generate a &'de Self, and therefore Self must live for at least 'de.
Since Self must live for at least 'de and at most 'de, it must live for exactly 'de, i.e. Generic<'de, 'de, 'de> is what I want here. But I don't know how to do it.
HadrienG2
force-pushed
the
next-abomonation
branch
from
340d87f
to
3e0c125
Compare
HadrienG2
changed the title
| } | ||
| } | ||
| } | ||
| alignment.push(quote!(align)); |
There was a problem hiding this comment.
Abomonation API change from TimelyDataflow/abomonation#33: after this PR, abomonation will provide full support for correctly aligning data in memory... which requires from support from the Abomonation trait implementors.
| @@ -26,6 +27,7 @@ mod tests { | |||
| assert_eq!(bytes.len(), measure(&record)); | |||
|
|
|||
| // decode from binary data | |||
| let mut bytes = AlignedBytes::<Struct>::new(&mut bytes); | |||
There was a problem hiding this comment.
decode() now expects its input data to be properly aligned, and this is a minimally invasive way to prepare said data.
WIP because 1/depends on a development branch of abomonation and 2/still has a memory-safety-critical bug that I need your help to solve
So, I've been proposing a bunch of changes to abomonation recently. If accepted, they will cause quite a bit of API breakage. This PR matches my understanding of how
abomonation_derivewill need to change in order to adapt to the new API.