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

[WIP] [Help wanted] Adapt to upcoming changes in Abomonation #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,4 @@ quote = "1.0"
synstructure = "0.12"

[dev-dependencies]
abomonation = "0.7"
abomonation = { git = "https://github.com/HadrienG2/abomonation.git", branch = "alignment" }
88 changes: 64 additions & 24 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
#![recursion_limit="128"]

use synstructure::decl_derive;
use quote::quote;
use std::collections::HashSet;
use synstructure::decl_derive;

decl_derive!([Abomonation, attributes(unsafe_abomonate_ignore)] => derive_abomonation);

Expand All @@ -14,38 +15,77 @@ 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)?;
Copy link
Author

@HadrienG2 HadrienG2 Oct 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});

let extent = s.each(|bi| quote! {
sum += ::abomonation::Abomonation::extent(#bi);
});
// T::alignment() is the max of mem::align_of<T> and the U::alignment()s of
// every U type used as a struct member or inside of an enum variant (which
// includes the alignment of recursively abomonated data)
//
// Unfortunately, we cannot use synstructure's nice `fold()` convenience
// here because it's based on generating match arms for a `self` value,
// whereas here we're trying to implement an inherent type method without
// having such a `self` handy.
//
// We can, however, use Structure::variants() and VariantInfo::bindings()
// to enumerate all the types which _would appear_ in such match arms'
// inner bindings.
//
let mut alignment = vec![
quote!(let mut align = ::std::mem::align_of::<Self>();)
];
let mut probed_types = HashSet::new();
for variant_info in s.variants() {
for binding_info in variant_info.bindings() {
let binding_type = &binding_info.ast().ty;
// Do not query a type's alignment() multiple times
if probed_types.insert(binding_type) {
alignment.push(
quote!(align = align.max(<#binding_type as ::abomonation::Entomb>::alignment());)
);
}
}
}
alignment.push(quote!(align));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


s.bind_with(|_| synstructure::BindStyle::RefMut);

let exhume = s.each(|bi| quote! {
let temp = bytes;
bytes = ::abomonation::Abomonation::exhume(#bi, temp)?;
::abomonation::Exhume::exhume(From::from(#bi), reader)?;
});

s.bound_impl(quote!(abomonation::Abomonation), quote! {
#[inline] unsafe fn entomb<W: ::std::io::Write>(&self, _write: &mut W) -> ::std::io::Result<()> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

match *self { #entomb }
Ok(())
}
#[allow(unused_mut)]
#[inline] fn extent(&self) -> usize {
let mut sum = 0;
match *self { #extent }
sum
s.gen_impl(quote! {
extern crate abomonation;
extern crate std;

gen unsafe impl abomonation::Entomb for @Self {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

unsafe fn entomb<W: ::std::io::Write>(
&self,
_write: &mut ::abomonation::align::AlignedWriter<W>
) -> ::std::io::Result<()> {
match *self { #entomb }
Ok(())
}

fn alignment() -> usize {
#(#alignment)*
}
}
#[allow(unused_mut)]
#[inline] unsafe fn exhume<'a,'b>(
&'a mut self,
mut bytes: &'b mut [u8]
) -> Option<&'b mut [u8]> {
match *self { #exhume }
Some(bytes)

gen unsafe impl<'de> abomonation::Exhume<'de> for @Self
where Self: 'de,
Copy link
Author

@HadrienG2 HadrienG2 Oct 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
#[allow(unused_mut)]
unsafe fn exhume(
self_: std::ptr::NonNull<Self>,
reader: &mut ::abomonation::align::AlignedReader<'de>
) -> Option<&'de mut Self> {
// FIXME: This (briefly) constructs an &mut _ to invalid data
// (via "ref mut"), which is UB. The proposed &raw mut
// operator would allow avoiding this.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

match *self_.as_ptr() { #exhume }
Some(&mut *self_.as_ptr())
}
}
})
}
32 changes: 31 additions & 1 deletion tests/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ extern crate abomonation_derive;
#[cfg(test)]
mod tests {
use abomonation::*;
use abomonation::align::AlignedBytes;

#[derive(Eq, PartialEq, Abomonation)]
pub struct Struct {
Expand All @@ -26,6 +27,7 @@ mod tests {
assert_eq!(bytes.len(), measure(&record));

// decode from binary data
let mut bytes = AlignedBytes::<Struct>::new(&mut bytes);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decode() now expects its input data to be properly aligned, and this is a minimally invasive way to prepare said data.

if let Some((result, rest)) = unsafe { decode::<Struct>(&mut bytes) } {
assert!(result == &record);
assert!(rest.len() == 0);
Expand All @@ -47,6 +49,7 @@ mod tests {
assert_eq!(bytes.len(), measure(&record));

// decode from binary data
let mut bytes = AlignedBytes::<EmptyStruct>::new(&mut bytes);
if let Some((result, rest)) = unsafe { decode::<EmptyStruct>(&mut bytes) } {
assert!(result == &record);
assert!(rest.len() == 0);
Expand All @@ -68,6 +71,7 @@ mod tests {
assert_eq!(bytes.len(), measure(&record));

// decode from binary data
let mut bytes = AlignedBytes::<TupleStruct>::new(&mut bytes);
if let Some((result, rest)) = unsafe { decode::<TupleStruct>(&mut bytes) } {
assert!(result == &record);
assert!(rest.len() == 0);
Expand All @@ -89,6 +93,7 @@ mod tests {
assert_eq!(bytes.len(), measure(&record));

// decode from binary data
let mut bytes = AlignedBytes::<GenericStruct<String, Vec<u8>>>::new(&mut bytes);
if let Some((result, rest)) = unsafe { decode::<GenericStruct<String, Vec<u8>>>(&mut bytes) } {
assert!(result == &record);
assert!(rest.len() == 0);
Expand All @@ -115,6 +120,7 @@ mod tests {
assert_eq!(bytes.len(), measure(&record));

// decode from binary data
let mut bytes = AlignedBytes::<BasicEnum>::new(&mut bytes);
if let Some((result, rest)) = unsafe { decode::<BasicEnum>(&mut bytes) } {
assert!(result == &record);
assert!(rest.len() == 0);
Expand All @@ -126,7 +132,7 @@ mod tests {
pub enum DataEnum {
A(String, u64, Vec<u8>),
B,
C(String, String, String)
C(String, String, u16)
}

#[test]
Expand All @@ -141,6 +147,7 @@ mod tests {
assert_eq!(bytes.len(), measure(&record));

// decode from binary data
let mut bytes = AlignedBytes::<DataEnum>::new(&mut bytes);
if let Some((result, rest)) = unsafe { decode::<DataEnum>(&mut bytes) } {
assert!(result == &record);
assert!(rest.len() == 0);
Expand Down Expand Up @@ -197,9 +204,32 @@ mod tests {
assert_eq!(bytes.len(), measure(&record));

// decode from binary data
let mut bytes = AlignedBytes::<StructUsingCratePath>::new(&mut bytes);
if let Some((result, rest)) = unsafe { decode::<StructUsingCratePath>(&mut bytes) } {
assert!(result == &record);
assert!(rest.len() == 0);
}
}

#[derive(Eq, PartialEq, Abomonation)]
pub struct StructWithRef<'a> {
a: &'a str,
b: bool,
}

#[test]
fn test_struct_with_ref() {
let record = StructWithRef { a: &"test", b: true };

let mut bytes = Vec::new();
unsafe { encode(&record, &mut bytes).unwrap(); }

assert_eq!(bytes.len(), measure(&record));

let mut bytes = AlignedBytes::<StructWithRef>::new(&mut bytes);
if let Some((result, rest)) = unsafe { decode::<StructWithRef>(&mut bytes) } {
assert!(result == &record);
assert!(rest.len() == 0);
}
}
}