Skip to contentSkip to conversation

rust: pin-init: fix incorrect accessor reference lifetime

PATCH New Gary Guo 1 participant 1 message Apr 20

Subsystems (3)
lore
Share & tools
lore

Gary Guo

author
#0 lore raw

PATCH 1/1 rust: pin-init: fix incorrect accessor reference lifetime

From: Gary Guo <[email protected]>

When a field has been initialized, init!/pin_init! create a reference or pinned reference to the field so it can be accessed later during the initialization of other fields. However, the reference it created is incorrectly &'static rather than just the scope of the initializer.

This means that you can do

    init!(Foo {
        a: 1,
        _: {
            let b: &'static u32 = a;
        }
    })

which is unsound.

This is caused by &mut (*#slot).#ident, which actually allows arbitrary lifetime, so this is effectively 'static. Somewhat ironically, the safety justification of creating the accessor is.. "SAFETY: TODO".

Fix it by adding Deref/DerefMut implementation to DropGuard and derive the reference from dereferencing DropGuard instead. The lifetime of DropGuard is exactly what we want for these accessors.

The old accessor creation code also has a purpose of preventing unaligned fields. So a let _ = &(*#slot).#ident still needs to be present, but this no longer has to care about pinning, so it is moved to just before the guard generation.

Fixes 42415d163e5d ("rust: pin-init: add references to previously initialized fields")

---
The patch is also available at https://github.com/Rust-for-Linux/pin-init/pull/132
which has regression tests added and runs through pin-init's test suite.

---
 rust/pin-init/internal/src/init.rs | 107 +++++++++++++----------------
 rust/pin-init/src/__internal.rs    |  27 +++++++-
 2 files changed, 73 insertions(+), 61 deletions(-)
rust/pin-init/internal/src/init.rs +48 -59
index daa3f1c6466e..c5f848103c92 100644
@@ -249,22 +249,6 @@ fn init_fields(
                 });
                 // Again span for better diagnostics
                 let write = quote_spanned!(ident.span()=> ::core::ptr::write);
-                // NOTE: the field accessor ensures that the initialized field is properly aligned.
-                // Unaligned fields will cause the compiler to emit E0793. We do not support
-                // unaligned fields since `Init::__init` requires an aligned pointer; the call to
-                // `ptr::write` below has the same requirement.
-                let accessor = if pinned {
-                    let project_ident = format_ident!("__project_{ident}");
-                    quote! {
-                        // SAFETY: TODO
-                        unsafe { #data.#project_ident(&mut (*#slot).#ident) }
-                    }
-                } else {
-                    quote! {
-                        // SAFETY: TODO
-                        unsafe { &mut (*#slot).#ident }
-                    }
-                };
                 quote! {
                     #(#attrs)*
                     {
@@ -272,51 +256,31 @@ fn init_fields(
                         // SAFETY: TODO
                         unsafe { #write(&raw mut (*#slot).#ident, #value_ident) };
                     }
-                    #(#cfgs)*
-                    #[allow(unused_variables)]
-                    let #ident = #accessor;
                 }
             }
             InitializerKind::Init { ident, value, .. } => {
                 // Again span for better diagnostics
                 let init = format_ident!("init", span = value.span());
-                // NOTE: the field accessor ensures that the initialized field is properly aligned.
-                // Unaligned fields will cause the compiler to emit E0793. We do not support
-                // unaligned fields since `Init::__init` requires an aligned pointer; the call to
-                // `ptr::write` below has the same requirement.
-                let (value_init, accessor) = if pinned {
-                    let project_ident = format_ident!("__project_{ident}");
-                    (
-                        quote! {
-                            // SAFETY:
-                            // - `slot` is valid, because we are inside of an initializer closure, we
-                            //   return when an error/panic occurs.
-                            // - We also use `#data` to require the correct trait (`Init` or `PinInit`)
-                            //   for `#ident`.
-                            unsafe { #data.#ident(&raw mut (*#slot).#ident, #init)? };
-                        },
-                        quote! {
-                            // SAFETY: TODO
-                            unsafe { #data.#project_ident(&mut (*#slot).#ident) }
-                        },
-                    )
+                let value_init = if pinned {
+                    quote! {
+                        // SAFETY:
+                        // - `slot` is valid, because we are inside of an initializer closure, we
+                        //   return when an error/panic occurs.
+                        // - We also use `#data` to require the correct trait (`Init` or `PinInit`)
+                        //   for `#ident`.
+                        unsafe { #data.#ident(&raw mut (*#slot).#ident, #init)? };
+                    }
                 } else {
-                    (
-                        quote! {
-                            // SAFETY: `slot` is valid, because we are inside of an initializer
-                            // closure, we return when an error/panic occurs.
-                            unsafe {
-                                ::pin_init::Init::__init(
-                                    #init,
-                                    &raw mut (*#slot).#ident,
-                                )?
-                            };
-                        },
-                        quote! {
-                            // SAFETY: TODO
-                            unsafe { &mut (*#slot).#ident }
-                        },
-                    )
+                    quote! {
+                        // SAFETY: `slot` is valid, because we are inside of an initializer
+                        // closure, we return when an error/panic occurs.
+                        unsafe {
+                            ::pin_init::Init::__init(
+                                #init,
+                                &raw mut (*#slot).#ident,
+                            )?
+                        };
+                    }
                 };
                 quote! {
                     #(#attrs)*
@@ -324,9 +288,6 @@ fn init_fields(
                         let #init = #value;
                         #value_init
                     }
-                    #(#cfgs)*
-                    #[allow(unused_variables)]
-                    let #ident = #accessor;
                 }
             }
             InitializerKind::Code { block: value, .. } => quote! {
@@ -339,18 +300,46 @@ fn init_fields(
         if let Some(ident) = kind.ident() {
             // `mixed_site` ensures that the guard is not accessible to the user-controlled code.
             let guard = format_ident!("__{ident}_guard", span = Span::mixed_site());
+
+            // NOTE: The reference is derived from the guard so that it only lives as long as the
+            // guard does and cannot escape the scope. If it's created via `&mut (*#slot).#ident`
+            // like the unaligned field guard, it will become effectively `'static`.
+            let accessor = if pinned {
+                let project_ident = format_ident!("__project_{ident}");
+                quote! {
+                    // SAFETY: the initialization is pinned.
+                    unsafe { #data.#project_ident(&mut *#guard) }
+                }
+            } else {
+                quote! {
+                    &mut *#guard
+                }
+            };
+
             res.extend(quote! {
+                // Take a reference of the field to ensure that the initialized field is
+                // properly aligned. Unaligned fields will cause the compiler to emit E0793. We
+                // do not support unaligned fields since `Init::__init` requires an aligned
+                // pointer; the call to `ptr::write` above has the same requirement.
+                // SAFETY: `slot` is valid.
+                #(#cfgs)*
+                let _ = unsafe { &(*#slot).#ident };
+
                 #(#cfgs)*
                 // Create the drop guard:
                 //
                 // We rely on macro hygiene to make it impossible for users to access this local
                 // variable.
                 // SAFETY: We forget the guard later when initialization has succeeded.
-                let #guard = unsafe {
+                let mut #guard = unsafe {
                     ::pin_init::__internal::DropGuard::new(
                         &raw mut (*slot).#ident
                     )
                 };
+
+                #(#cfgs)*
+                #[allow(unused_variables)]
+                let #ident = #accessor;
             });
             guards.push(guard);
             guard_attrs.push(cfgs);
rust/pin-init/src/__internal.rs +25 -2
index 90adbdc1893b..d122ab840c44 100644
@@ -5,6 +5,8 @@
 //! These items must not be used outside of this crate and the pin-init-internal crate located at
 //! `../internal`.
 
+use core::ops::{Deref, DerefMut};
+
 use super::*;
 
 /// See the [nomicon] for what subtyping is. See also [this table].
@@ -238,6 +240,10 @@ struct Foo {
 /// When a value of this type is dropped, it drops a `T`.
 ///
 /// Can be forgotten to prevent the drop.
+///
+/// # Invariants
+///
+/// `ptr` is convertible to a mutable reference and `*ptr` is owned by `DropGuard`.
 pub struct DropGuard<T: ?Sized> {
     ptr: *mut T,
 }
@@ -259,11 +265,28 @@ pub unsafe fn new(ptr: *mut T) -> Self {
     }
 }
 
+impl<T: ?Sized> Deref for DropGuard<T> {
+    type Target = T;
+
+    #[inline]
+    fn deref(&self) -> &T {
+        // SAFETY: `ptr` is convertible to reference
+        unsafe { &*self.ptr }
+    }
+}
+
+impl<T: ?Sized> DerefMut for DropGuard<T> {
+    #[inline]
+    fn deref_mut(&mut self) -> &mut T {
+        // SAFETY: `ptr` is convertible to a mutable reference
+        unsafe { &mut *self.ptr }
+    }
+}
+
 impl<T: ?Sized> Drop for DropGuard<T> {
     #[inline]
     fn drop(&mut self) {
-        // SAFETY: A `DropGuard` can only be constructed using the unsafe `new` function
-        // ensuring that this operation is safe.
+        // SAFETY: `DropGuard` owns the `*ptr`.
         unsafe { ptr::drop_in_place(self.ptr) }
     }
 }

base-commit: 1c7cc4904160c6fc6377564140062d68a3dc93a0

Signature · 1 line
2.51.2