Use MaybeUninit to restore soundness

README.md

@@ -10,6 +10,10 @@ Dual licensed under MIT / Apache 2.0.
 While this crate is `no_std` compatible, it still requires the `alloc` crate.
 
 Version notes:
+- Version `0.17.0` fixes a potential soundness issue, but removes support for
+  template parameters in the process. Lifetime parameters are still supported.
+  As of 2023-06-13, the compiler is practically sound with 0.16 but this may
+  stop being the case at any time and without warning.
 - Version `0.13.0` and later contain checks for additional situations which
   cause undefined behavior if not caught.
 - Version `0.11.0` and later place restrictions on derive macros, earlier

examples/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "ouroboros_examples"
-version = "0.15.7"
+version = "0.17.0"
 authors = ["Joshua Maros <joshua-maros@github.com>"]
 edition = "2018"
 license = "MIT OR Apache-2.0"
@@ -21,7 +21,7 @@ std = []
 __tokio = ["tokio", "std"]
 
 [dependencies]
-ouroboros = { version = "0.15.7", path = "../ouroboros" }
+ouroboros = { version = "0.17.0", path = "../ouroboros" }
 tokio = { version = "1.27.0", features = [ "macros", "rt" ], optional = true }
 
 [dev-dependencies]

examples/src/fail_tests/auto_covariant.stderr

@@ -1,6 +1,11 @@
 error: Ouroboros cannot automatically determine if this type is covariant.
 
-       If it is covariant, it should be legal to convert any instance of that type to an instance of that type where all usages of 'this are replaced with a smaller lifetime. For example, Box<&'this i32> is covariant because it is legal to use it as a Box<&'a i32> where 'this: 'a. In contrast, Fn(&'this i32) cannot be used as Fn(&'a i32).
+       As an example, a Box<&'this ()> is covariant because it can be used as a
+       Box<&'smaller ()> for any lifetime smaller than 'this. In contrast,
+       a Fn(&'this ()) is not covariant because it cannot be used as a
+       Fn(&'smaller ()). In general, values that are safe to use with smaller
+       lifetimes than they were defined with are covariant, breaking this
+       guarantee means the value is not covariant.
 
        To resolve this error, add #[covariant] or #[not_covariant] to the field.
   --> src/fail_tests/auto_covariant.rs:11:12

examples/src/ok_tests.rs

@@ -1,5 +1,4 @@
-use alloc::borrow::ToOwned;
-use alloc::boxed::Box;
+use alloc::{boxed::Box, borrow::ToOwned};
 use core::fmt::Debug;
 
 use ouroboros::self_referencing;
@@ -32,7 +31,7 @@ struct ChainedAndUndocumented {
     data: i32,
     #[borrows(data)]
     ref1: &'this i32,
-    #[borrows(data)]
+    #[borrows(ref1)]
     ref2: &'this &'this i32,
 }
 
@@ -51,32 +50,32 @@ struct AutoDetectCovarianceOnFieldsWithoutThis {
     self_reference: &'this (),
 }
 
-/// This test just makes sure that the macro copes with a ton of template parameters being thrown at
-/// it, specifically checking that the templates work fine even when a generated struct doesn't need
-/// all of them. (E.G. heads will only contain 'd, A, and B.)
-#[self_referencing]
-struct TemplateMess<'d, A, B: 'static, C: 'static>
-where
-    A: ?Sized,
-    B: 'static,
-    C: 'static,
-{
-    external: &'d A,
-    data1: B,
-    #[borrows(data1)]
-    data2: &'this C,
-    data3: B,
-    #[borrows(mut data3)]
-    data4: &'this mut C,
-}
+// /// This test just makes sure that the macro copes with a ton of template parameters being thrown at
+// /// it, specifically checking that the templates work fine even when a generated struct doesn't need
+// /// all of them. (E.G. heads will only contain 'd, A, and B.)
+// #[self_referencing]
+// struct TemplateMess<'d, A, B: 'static, C: 'static>
+// where
+//     A: ?Sized,
+//     B: 'static,
+//     C: 'static,
+// {
+//     external: &'d A,
+//     data1: B,
+//     #[borrows(data1)]
+//     data2: &'this C,
+//     data3: B,
+//     #[borrows(mut data3)]
+//     data4: &'this mut C,
+// }
 
-/// Regression test for #46
-#[self_referencing]
-struct PreviouslyBrokeAutoGeneratedChecker<T: 'static> {
-    x: T,
-    #[borrows(mut x)]
-    y: &'this (),
-}
+// /// Regression test for #46
+// #[self_referencing]
+// struct PreviouslyBrokeAutoGeneratedChecker<T: 'static> {
+//     x: T,
+//     #[borrows(mut x)]
+//     y: &'this (),
+// }
 
 #[test]
 fn box_and_ref() {
@@ -201,25 +200,25 @@ fn box_and_mut_ref() {
     assert!(bar.with_dref(|dref| **dref) == 34);
 }
 
-#[test]
-fn template_mess() {
-    let ext_str = "Hello World!".to_owned();
-    let mut instance = TemplateMessBuilder {
-        external: &ext_str[..],
-        data1: "asdf".to_owned(),
-        data2_builder: |data1_contents| data1_contents,
-        data3: "asdf".to_owned(),
-        data4_builder: |data3_contents| data3_contents,
-    }
-    .build();
-    instance.with_external(|ext| assert_eq!(*ext, "Hello World!"));
-    instance.with_data1(|data| assert_eq!(data, "asdf"));
-    instance.with_data4_mut(|con| **con = "Modified".to_owned());
-    instance.with(|fields| {
-        assert!(**fields.data1 == **fields.data2);
-        assert!(*fields.data4 == "Modified");
-    });
-}
+// #[test]
+// fn template_mess() {
+//     let ext_str = "Hello World!".to_owned();
+//     let mut instance = TemplateMessBuilder {
+//         external: &ext_str[..],
+//         data1: "asdf".to_owned(),
+//         data2_builder: |data1_contents| data1_contents,
+//         data3: "asdf".to_owned(),
+//         data4_builder: |data3_contents| data3_contents,
+//     }
+//     .build();
+//     instance.with_external(|ext| assert_eq!(*ext, "Hello World!"));
+//     instance.with_data1(|data| assert_eq!(data, "asdf"));
+//     instance.with_data4_mut(|con| **con = "Modified".to_owned());
+//     instance.with(|fields| {
+//         assert!(**fields.data1 == **fields.data2);
+//         assert!(*fields.data4 == "Modified");
+//     });
+// }
 
 const STATIC_INT: i32 = 456;
 #[test]
@@ -246,6 +245,12 @@ fn single_lifetime() {
         #[borrows(external)]
         internal: &'this &'a str,
     }
+
+    let external = "Hello world!".to_owned();
+    let instance = Struct::new(&external, |field_ref| field_ref);
+    drop(instance.borrow_external());
+    drop(instance.borrow_internal());
+    drop(instance);
 }
 
 #[test]

ouroboros/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "ouroboros"
-version = "0.15.7"
+version = "0.17.0"
 authors = ["Joshua Maros <joshua-maros@github.com>"]
 edition = "2018"
 license = "MIT OR Apache-2.0"
@@ -11,7 +11,8 @@ repository = "https://github.com/joshua-maros/ouroboros"
 
 [dependencies]
 aliasable = "0.1.3"
-ouroboros_macro = { version = "0.15.7", path = "../ouroboros_macro" }
+ouroboros_macro = { version = "0.17.0", path = "../ouroboros_macro" }
+static_assertions = "1.1.0"
 
 [features]
 default = ["std"]

ouroboros/src/lib.rs

@@ -123,7 +123,7 @@
 ///         immutable: i32,
 ///         mutable: i32,
 ///         #[borrows(immutable, mut mutable)]
-///         #[covariant]
+///         #[not_covariant]
 ///         complex_data: ComplexData<'this, 'this>,
 ///     }
 ///
@@ -351,6 +351,7 @@ pub mod macro_help {
     pub extern crate alloc;
 
     pub use aliasable::boxed::AliasableBox;
+    pub use static_assertions::const_assert_eq;
     use aliasable::boxed::UniqueBox;
 
     pub struct CheckIfTypeIsStd<T>(core::marker::PhantomData<T>);

ouroboros_macro/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "ouroboros_macro"
-version = "0.15.7"
+version = "0.17.0"
 authors = ["Joshua Maros <joshua-maros@github.com>"]
 edition = "2018"
 license = "MIT OR Apache-2.0"

ouroboros_macro/src/generate/constructor.rs

@@ -13,6 +13,7 @@ pub fn create_builder_and_constructor(
 ) -> Result<(Ident, TokenStream, TokenStream), Error> {
     let struct_name = info.ident.clone();
     let generic_args = info.generic_arguments();
+    let generic_args_with_static_lifetimes = info.generic_arguments_with_static_lifetimes();
 
     let vis = if options.do_pub_extras {
         info.vis.clone()
@@ -154,12 +155,23 @@ pub fn create_builder_and_constructor(
         BuilderType::Sync => quote! { fn new },
     };
     let field_names: Vec<_> = info.fields.iter().map(|field| field.name.clone()).collect();
+    let internal_ident = &info.internal_ident;
     let constructor_def = quote! {
         #documentation
         #vis #constructor_fn(#(#params),*) -> #struct_name <#(#generic_args),*> {
+            ::ouroboros::macro_help::const_assert_eq!(
+                ::core::mem::size_of::<#struct_name<#(#generic_args_with_static_lifetimes),*>>(),
+                ::core::mem::size_of::<#internal_ident<#(#generic_args_with_static_lifetimes),*>>()
+            );
+            ::ouroboros::macro_help::const_assert_eq!(
+                ::core::mem::align_of::<#struct_name<#(#generic_args_with_static_lifetimes),*>>(),
+                ::core::mem::align_of::<#internal_ident<#(#generic_args_with_static_lifetimes),*>>()
+            );
             #(#code)*
-            Self {
-                #(#field_names),*
+            unsafe {
+                ::core::mem::transmute(#internal_ident::<#(#generic_args),*> {
+                    #(#field_names),*
+                })
             }
         }
     };

ouroboros_macro/src/generate/drop.rs

@@ -0,0 +1,25 @@
+use crate::info_structures::StructInfo;
+use proc_macro2::TokenStream;
+use quote::quote;
+use syn::Error;
+
+pub fn create_drop_impl(info: &StructInfo) -> Result<TokenStream, Error> {
+    let ident = &info.ident;
+    let internal_ident = &info.internal_ident;
+    let generics = &info.generics;
+    let generic_args = info.generic_arguments();
+
+    let mut where_clause = quote! {};
+    if let Some(clause) = &generics.where_clause {
+        where_clause = quote! { #clause };
+    }
+    Ok(quote! {
+        impl #generics ::core::ops::Drop for #ident<#(#generic_args,)*> #where_clause {
+            fn drop(&mut self) {
+                unsafe {
+                    ::core::ptr::drop_in_place(::core::mem::transmute::<_, *mut #internal_ident <#(#generic_args,)*>>(self));
+                }
+            }
+        }
+    })
+}

ouroboros_macro/src/generate/into_heads.rs

@@ -13,12 +13,16 @@ pub fn make_into_heads(info: &StructInfo, options: Options) -> (TokenStream, Tok
     let mut code = Vec::new();
     let mut field_initializers = Vec::new();
     let mut head_fields = Vec::new();
+    let internal_struct = &info.internal_ident;
     // Drop everything in the reverse order of what it was declared in. Fields that come later
     // are only dependent on fields that came before them.
     for field in info.fields.iter().rev() {
         let field_name = &field.name;
-        if !field.self_referencing {
-            code.push(quote! { let #field_name = self.#field_name; });
+        if field.self_referencing {
+            // Heads are fields that do not borrow anything.
+            code.push(quote! { ::core::mem::drop(this.#field_name); });
+        } else {
+            code.push(quote! { let #field_name = this.#field_name; });
             if field.is_borrowed() {
                 field_initializers
                     .push(quote! { #field_name: ::ouroboros::macro_help::unbox(#field_name) });
@@ -27,9 +31,6 @@ pub fn make_into_heads(info: &StructInfo, options: Options) -> (TokenStream, Tok
             }
             let field_type = &field.typ;
             head_fields.push(quote! { #visibility #field_name: #field_type });
-        } else {
-            // Heads are fields that do not borrow anything.
-            code.push(quote! { ::core::mem::drop(self.#field_name); });
         }
     }
     for (ty, ident) in info.generic_consumers() {
@@ -71,6 +72,7 @@ pub fn make_into_heads(info: &StructInfo, options: Options) -> (TokenStream, Tok
         #[allow(clippy::drop_copy)]
         #[allow(clippy::drop_non_drop)]
         #visibility fn into_heads(self) -> Heads<#(#generic_args),*> {
+            let this: #internal_struct<#(#generic_args),*> = unsafe { ::core::mem::transmute(self) };
             #(#code)*
             Heads {
                 #(#field_initializers),*

ouroboros_macro/src/generate/mod.rs

@@ -1,5 +1,6 @@
 pub mod constructor;
 pub mod derives;
+pub mod drop;
 pub mod into_heads;
 pub mod struc;
 pub mod summon_checker;

ouroboros_macro/src/generate/struc.rs

@@ -6,12 +6,35 @@ use proc_macro2::TokenStream;
 use quote::quote;
 use syn::Error;
 
-/// Creates the struct that will actually store the data. This involves properly organizing the
-/// fields, collecting metadata about them, reversing the order everything is stored in, and
-/// converting any uses of 'this to 'static.
+/// Creates the struct that will actually store the data.
 pub fn create_actual_struct_def(info: &StructInfo) -> Result<TokenStream, Error> {
-    let vis = utils::submodule_contents_visiblity(&info.vis);
+    let visibility = utils::submodule_contents_visiblity(&info.vis);
+    let mut fields = Vec::new();
+    for (ty, ident) in info.generic_consumers() {
+        fields.push(quote! { #ident: ::core::marker::PhantomData<#ty> });
+    }
+    let generic_params = info.generic_params();
+    let generic_args = info.generic_arguments_with_static_lifetimes();
+    let generic_where = &info.generics.where_clause;
     let ident = &info.ident;
+    let internal_ident = &info.internal_ident;
+    Ok(quote! {
+        #visibility struct #ident <#generic_params> #generic_where {
+            actual_data: ::core::mem::MaybeUninit<[u8; ::core::mem::size_of::<#internal_ident<#(#generic_args),*>>()]>,
+            _alignment: [#internal_ident<#(#generic_args),*>; 0],
+            #(#fields),*
+        }
+    })
+}
+
+/// Creates a struct with fields like the original struct. Instances of the
+/// "actual" struct are reinterpreted as instances of the "internal" struct
+/// whenever data needs to be accessed. (This gets around the problem that
+/// references passed to functions must be valid through the entire function,
+/// but references *created* inside a function can be considered invalid
+/// whenever, even during the duration of the function.)
+pub fn create_internal_struct_def(info: &StructInfo) -> Result<TokenStream, Error> {
+    let ident = &info.internal_ident;
     let generics = &info.generics;
 
     let field_defs: Vec<_> = info
@@ -37,7 +60,7 @@ pub fn create_actual_struct_def(info: &StructInfo) -> Result<TokenStream, Error>
         where_clause = quote! { #clause };
     }
     let def = quote! {
-        #vis struct #ident #generics #where_clause {
+        struct #ident #generics #where_clause {
             #(#field_defs),*
         }
     };

ouroboros_macro/src/generate/try_constructor.rs

@@ -223,6 +223,7 @@ pub fn create_try_builder_and_constructor(
         quote! { #struct_name::#or_recover_ident(#(#builder_struct_field_names),*).map_err(|(error, _heads)| error) }
     };
     let field_names: Vec<_> = info.fields.iter().map(|field| field.name.clone()).collect();
+    let internal_ident = &info.internal_ident;
     let constructor_def = quote! {
         #documentation
         #visibility #constructor_fn<Error_>(#(#params),*) -> ::core::result::Result<#struct_name <#(#generic_args),*>, Error_> {
@@ -231,7 +232,9 @@ pub fn create_try_builder_and_constructor(
         #or_recover_documentation
         #visibility #or_recover_constructor_fn<Error_>(#(#params),*) -> ::core::result::Result<#struct_name <#(#generic_args),*>, (Error_, Heads<#(#generic_args),*>)> {
             #(#or_recover_code)*
-            ::core::result::Result::Ok(Self { #(#field_names),* })
+            ::core::result::Result::Ok(unsafe {
+                ::core::mem::transmute(#internal_ident { #(#field_names),* })
+            })
         }
     };
     builder_struct_generic_producers.push(quote! { Error_ });