Clean up Abomonated

Author: HadrienG2

src/abomonated.rs

@@ -1,23 +1,27 @@
 
-use std::mem::transmute;
 use std::marker::PhantomData;
 use std::ops::{Deref, DerefMut};
 
 use super::{Abomonation, decode};
 
 /// A type wrapping owned decoded abomonated data.
 ///
-/// This type ensures that decoding and pointer correction has already happened,
-/// and implements `Deref<Target=T>` using a pointer cast (transmute).
+/// This type ensures that decoding and pointer correction has already happened.
+/// It provides a way to move the deserialized data around, while keeping
+/// on-demand access to it via the `as_ref()` method.
+///
+/// As an extra convenience, `Deref<Target=T>` is also implemented if T does
+/// not contain references. Unfortunately, this trait cannot be safely
+/// implemented when T does contain references.
 ///
 /// #Safety
 ///
-/// The safety of this type, and in particular its `transute` implementation of
-/// the `Deref` trait, relies on the owned bytes not being externally mutated
-/// once provided. You could imagine a new type implementing `DerefMut` as required,
-/// but which also retains the ability (e.g. through `RefCell`) to mutate the bytes.
-/// This would be very bad, but seems hard to prevent in the type system. Please
-/// don't do this.
+/// The safety of this type, and in particular of access to the deserialized
+/// data, relies on the owned bytes not being externally mutated after the
+/// `Abomonated` is constructed. You could imagine a new type implementing
+/// `DerefMut` as required, but which also retains the ability (e.g. through
+/// `RefCell`) to mutate the bytes. This would be very bad, but seems hard to
+/// prevent in the type system. Please don't do this.
 ///
 /// You must also use a type `S` whose bytes have a fixed location in memory.
 /// Otherwise moving an instance of `Abomonated<T, S>` may invalidate decoded
@@ -54,9 +58,10 @@ pub struct Abomonated<T, S: DerefMut<Target=[u8]>> {
     decoded: S,
 }
 
-impl<'bytes, T, S> Abomonated<T, S>
-    where S: DerefMut<Target=[u8]> + 'bytes,
-          T: Abomonation<'bytes>,
+impl<'s, 't, T, S> Abomonated<T, S>
+    where S: DerefMut<Target=[u8]> + 's,
+          T: Abomonation<'t>,
+          's: 't
 {
     /// Attempts to create decoded data from owned mutable bytes.
     ///
@@ -95,59 +100,65 @@ impl<'bytes, T, S> Abomonated<T, S>
     /// The type `S` must have its bytes at a fixed location, which will
     /// not change if the `bytes: S` instance is moved. Good examples are
     /// `Vec<u8>` whereas bad examples are `[u8; 16]`.
-    pub unsafe fn new(bytes: S) -> Option<Self> {
-        // FIXME: Our API specification for `T` and `S` interacts with the API
-        //        specification of `decode()` in such an unfortunate way that
-        //        `decode::<T>(bytes.borrow_mut())` borrows `bytes` forever.
+    pub unsafe fn new(mut bytes: S) -> Option<Self> {
+        // Fix type `T`'s inner pointers. Will return `None` on failure.
         //
-        //        This is not what we want here, because it prevents moving
-        //        `bytes` into the `Abomonated` at the end of the function.
+        // FIXME: `slice::from_raw_parts_mut` is used to work around the borrow
+        //        checker marking `bytes` as forever borrowed if `&mut bytes` is
+        //        directly passed as input to `decode()`. But that is itself a
+        //        byproduct of the API contract specified by the `where` clause
+        //        above, which allows S to be `&'t mut [u8]` (and therefore
+        //        require such a perpetual borrow) in the worst case.
         //
-        //        I could not find a way to redesign the API of either
-        //        `Abomonated` or `decode()` so that this doesn't happen.
-        //        Therefore, I will have to work around it the unsafe way...
+        //        A different API contract might allow us to achieve the same
+        //        result without resorting to such evil unsafe tricks.
         //
-        let bytes_uc = std::cell::UnsafeCell::new(bytes);
-
-        {
-            // Create an `&'bytes mut [u8]` slice from `bytes_uc`, without
-            // borrowing `bytes_uc` in the process (`get()` returns `*mut _`)
-            //
-            // This is safe because `UnsafeCell<T>` can alias with `&mut T`.
-            //
-            let bytes: &'bytes mut [u8] = (*bytes_uc.get()).deref_mut();
+        decode::<T>(std::slice::from_raw_parts_mut(bytes.as_mut_ptr(),
+                                                   bytes.len()))?;
 
-            // Fix type `T`'s inner pointers (will exit early on failure)
-            //
-            // This borrows the `bytes` slice for its entire `'bytes` lifetime!
-            // But it's okay, we'll just throw that slice away at the end of the
-            // scope, without letting any reference to `bytes_uc` leak.
-            //
-            // As we have not borrowed `bytes_uc`, we can then keep using it.
-            //
-            decode::<T>(bytes)?;
-        }
-
-        // Get back the original `bytes` and return the Abomonated structure
+        // Build the Abomonated structure
         Some(Abomonated {
             phantom: PhantomData,
-            decoded: bytes_uc.into_inner(),
+            decoded: bytes,
         })
     }
 }
 
-impl<T, S: DerefMut<Target=[u8]>> Abomonated<T, S> {
+impl<'t, T, S> Abomonated<T, S>
+    where S: DerefMut<Target=[u8]>,
+          T: Abomonation<'t>
+{
+    /// Get a read-only view on the deserialized bytes
     pub fn as_bytes(&self) -> &[u8] {
         &self.decoded
     }
-}
 
+    /// Get a read-only view on the deserialized data
+    //
+    // NOTE: This method can be safely used even if type T contains references,
+    //       because it makes sure that the borrow of `self` lasts long enough
+    //       to encompass the lifetime of these references.
+    //
+    //       Otherwise, it would be possible to extract an `&'static Something`
+    //       from a short-lived borrow of a `Box<[u8]>`, then drop the `Box`,
+    //       and end up with a dangling reference.
+    //
+    pub fn as_ref<'a: 't>(&'a self) -> &'a T {
+        unsafe { &*(self.decoded.as_ptr() as *const T) }
+    }
+}
 
-impl<T, S: DerefMut<Target=[u8]>> Deref for Abomonated<T, S> {
+// NOTE: The lifetime constraint that was applied to `as_ref()` cannot be
+//       applied to a `Deref` implementation. Therefore, `Deref` can only be
+//       used on types T which do not contain references, as enforced by the
+//       higher-ranked trait bound below.
+impl<T, S> Deref for Abomonated<T, S>
+    where S: DerefMut<Target=[u8]>,
+          for<'t> T: Abomonation<'t>,
+{
     type Target = T;
     #[inline]
     fn deref(&self) -> &T {
-        let result: &T = unsafe { transmute(self.decoded.get_unchecked(0)) };
-        result
+        self.as_ref()
     }
 }

tests/tests.rs

@@ -157,3 +157,32 @@ fn test_net_types() {
     let (t, r) = unsafe { decode::<SocketAddr>(&mut bytes) }.unwrap(); assert_eq!(*t, socket_addr4);
     let (t, _r) = unsafe { decode::<SocketAddr>(r) }.unwrap(); assert_eq!(*t, socket_addr6);
 }
+
+#[test]
+fn test_abomonated_owned() {
+  use abomonation::abomonated::Abomonated;
+
+  let s_owned = "This is an owned string".to_owned();
+
+  let mut bytes = Vec::new();
+  unsafe { encode::<String, _>(&s_owned, &mut bytes).unwrap(); }
+
+  let abo = unsafe { Abomonated::<String, _>::new(bytes).unwrap() };
+  assert_eq!(abo.as_ref(), &s_owned);
+  assert_eq!(&*abo, &s_owned);
+}
+
+#[test]
+fn test_abomonated_ref() {
+  use abomonation::abomonated::Abomonated;
+
+  let s_owned = "This is an owned string".to_owned();
+  let s_borrow = &s_owned[11..16];  // "owned"
+
+  let mut bytes = Vec::new();
+  unsafe { encode::<&str, _>(&s_borrow, &mut bytes).unwrap(); }
+
+  let abo = unsafe { Abomonated::<&str, _>::new(bytes).unwrap() };
+  assert_eq!(abo.as_ref(), &s_borrow);
+  // NOTE: Cannot use Deref here because &str contains a reference
+}