From e43d3303b58e5918a2454e0c024548c88aef1058 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 21 May 2024 11:13:58 -0700 Subject: [PATCH 1/2] Fix soundness hole in ComObject --- crates/libs/core/src/com_object.rs | 45 ++++++++++++++--- crates/libs/core/src/unknown.rs | 3 -- crates/libs/implement/src/lib.rs | 79 +++++++++++++++++------------- 3 files changed, 84 insertions(+), 43 deletions(-) diff --git a/crates/libs/core/src/com_object.rs b/crates/libs/core/src/com_object.rs index 4b2c23960d..00d80e7b3b 100644 --- a/crates/libs/core/src/com_object.rs +++ b/crates/libs/core/src/com_object.rs @@ -15,10 +15,30 @@ use core::ptr::NonNull; /// User code should not deal directly with this trait. /// /// This trait is sort of the reverse of [`IUnknownImpl`]. This trait allows user code to use -/// `ComObject] instead of `ComObject`. -pub trait ComObjectInner { +/// [`ComObject`] instead of `ComObject`. +pub trait ComObjectInner: Sized { /// The generated `_Impl` type (aka the "boxed" type or "outer" type). type Outer: IUnknownImpl; + + /// Moves an instance of this type into a new ComObject box and returns it. + /// + /// # Safety + /// + /// It is important that safe Rust code never be able to acquire an owned instance of a + /// generated "outer" COM object type, e.g. `_Impl`. This would be unsafe because the + /// `_Impl` object contains a reference count field and provides methods that adjust + /// the reference count, and destroy the object when the reference count reaches zero. + /// + /// Safe Rust code must only be able to interact with these values by accessing them via a + /// `ComObject` reference. `ComObject` handles adjusting reference counts and associates the + /// lifetime of a `&_Impl` with the lifetime of the related `ComObject`. + /// + /// The `#[implement]` macro generates the implementation of this `into_object` method. + /// The generated `into_object` method encapsulates the construction of the `_Impl` + /// object and immediately places it into the heap and returns a `ComObject` reference to it. + /// This ensures that our requirement -- that safe Rust code never own a `_Impl` value + /// directly -- is met. + fn into_object(self) -> ComObject; } /// Describes the COM interfaces implemented by a specific COM object. @@ -57,10 +77,23 @@ pub struct ComObject { impl ComObject { /// Allocates a heap cell (box) and moves `value` into it. Returns a counted pointer to `value`. pub fn new(value: T) -> Self { - unsafe { - let box_ = T::Outer::new_box(value); - Self { ptr: NonNull::new_unchecked(Box::into_raw(box_)) } - } + T::into_object(value) + } + + /// Creates a new `ComObject` that points to an existing boxed instance. + /// + /// # Safety + /// + /// The caller must ensure that `ptr` points to a valid, heap-allocated instance of `T::Outer`. + /// Normally, this pointer comes from using `Box::into_raw(Box::new(...))`. + /// + /// The pointed-to box must have a reference count that is greater than zero. + /// + /// This function takes ownership of the existing pointer; it does not call `AddRef`. + /// The reference count must accurately reflect all outstanding references to the box, + /// including `ptr` in the count. + pub unsafe fn from_raw(ptr: NonNull) -> Self { + Self { ptr } } /// Gets a reference to the shared object stored in the box. diff --git a/crates/libs/core/src/unknown.rs b/crates/libs/core/src/unknown.rs index 40545524b1..1fe088636f 100644 --- a/crates/libs/core/src/unknown.rs +++ b/crates/libs/core/src/unknown.rs @@ -68,9 +68,6 @@ pub trait IUnknownImpl { /// The contained user type, e.g. `MyApp`. Also known as the "inner" type. type Impl; - /// Initializes a new heap box and returns it. - fn new_box(value: Self::Impl) -> Box; - /// Get a reference to the backing implementation. fn get_impl(&self) -> &Self::Impl; diff --git a/crates/libs/implement/src/lib.rs b/crates/libs/implement/src/lib.rs index 3f6ec0c102..9609ba5427 100644 --- a/crates/libs/implement/src/lib.rs +++ b/crates/libs/implement/src/lib.rs @@ -68,7 +68,7 @@ pub fn implement(attributes: proc_macro::TokenStream, original_type: proc_macro: let vtable_news = attributes.implement.iter().enumerate().map(|(enumerate, implement)| { let vtbl_ident = implement.to_vtbl_ident(); let offset = proc_macro2::Literal::isize_unsuffixed(-1 - enumerate as isize); - quote! { #vtbl_ident::new::() } + quote! { #vtbl_ident::new::<#impl_ident::#generics, #original_ident::#generics, #offset>() } }); let offset = attributes.implement.iter().enumerate().map(|(offset, _)| proc_macro2::Literal::usize_unsuffixed(offset)); @@ -96,11 +96,8 @@ pub fn implement(attributes: proc_macro::TokenStream, original_type: proc_macro: impl #generics ::core::convert::From<#original_ident::#generics> for #interface_ident where #constraints { #[inline(always)] fn from(this: #original_ident::#generics) -> Self { - let this = #impl_ident::#generics::new(this); - let mut this = ::core::mem::ManuallyDrop::new(::windows_core::imp::Box::new(this)); - let vtable_ptr = &this.vtables.#offset; - // SAFETY: interfaces are in-memory equivalent to pointers to their vtables. - unsafe { ::core::mem::transmute(vtable_ptr) } + let com_object = ::windows_core::ComObject::new(this); + com_object.into_interface() } } @@ -132,35 +129,45 @@ pub fn implement(attributes: proc_macro::TokenStream, original_type: proc_macro: let tokens = quote! { #[repr(C)] #vis struct #impl_ident #generics where #constraints { - identity: *const ::windows_core::IInspectable_Vtbl, - vtables: (#(*const #vtbl_idents,)*), + identity: &'static ::windows_core::IInspectable_Vtbl, + vtables: (#(&'static #vtbl_idents,)*), this: #original_ident::#generics, count: ::windows_core::imp::WeakRefCount, } - impl #generics #impl_ident::#generics where #constraints { - const VTABLES: (#(#vtbl_idents2,)*) = (#(#vtable_news,)*); - const IDENTITY: ::windows_core::IInspectable_Vtbl = ::windows_core::IInspectable_Vtbl::new::(); - fn new(this: #original_ident::#generics) -> Self { - Self { - identity: &Self::IDENTITY, - vtables:(#(&Self::VTABLES.#offset,)*), - this, - count: ::windows_core::imp::WeakRefCount::new(), - } - } - } impl #generics ::windows_core::ComObjectInner for #original_ident::#generics where #constraints { type Outer = #impl_ident::#generics; + + // IMPORTANT! This function handles assembling the "boxed" type of a COM object. + // It immediately moves the box into a heap allocation (box) and returns only a ComObject + // reference that points to it. We intentionally _do not_ expose any owned instances of + // Foo_Impl to safe Rust code, because doing so would allow unsound behavior in safe Rust + // code, due to the adjustments of the reference count that Foo_Impl permits. + // + // This is why this function returns ComObject instead of returning #impl_ident. + + fn into_object(self) -> ::windows_core::ComObject { + static VTABLES: (#(#vtbl_idents2,)*) = (#(#vtable_news,)*); + static IDENTITY: ::windows_core::IInspectable_Vtbl = ::windows_core::IInspectable_Vtbl::new::<#impl_ident::#generics, #identity_type, 0>(); + + let boxed = ::windows_core::imp::Box::new(#impl_ident::#generics { + identity: &IDENTITY, + vtables: (#(&VTABLES.#offset,)*), + this: self, + count: ::windows_core::imp::WeakRefCount::new(), + }); + unsafe { + let ptr = ::windows_core::imp::Box::into_raw(boxed); + ::windows_core::ComObject::from_raw( + ::core::ptr::NonNull::new_unchecked(ptr) + ) + } + } } impl #generics ::windows_core::IUnknownImpl for #impl_ident::#generics where #constraints { type Impl = #original_ident::#generics; - fn new_box(value: Self::Impl) -> ::windows_core::imp::Box { - ::windows_core::imp::Box::new(Self::new(value)) - } - #[inline(always)] fn get_impl(&self) -> &Self::Impl { &self.this @@ -259,22 +266,16 @@ pub fn implement(attributes: proc_macro::TokenStream, original_type: proc_macro: impl #generics ::core::convert::From<#original_ident::#generics> for ::windows_core::IUnknown where #constraints { #[inline(always)] fn from(this: #original_ident::#generics) -> Self { - let this = #impl_ident::#generics::new(this); - let boxed = ::core::mem::ManuallyDrop::new(::windows_core::imp::Box::new(this)); - unsafe { - ::core::mem::transmute(&boxed.identity) - } + let com_object = ::windows_core::ComObject::new(this); + com_object.into_interface() } } impl #generics ::core::convert::From<#original_ident::#generics> for ::windows_core::IInspectable where #constraints { #[inline(always)] fn from(this: #original_ident::#generics) -> Self { - let this = #impl_ident::#generics::new(this); - let boxed = ::core::mem::ManuallyDrop::new(::windows_core::imp::Box::new(this)); - unsafe { - ::core::mem::transmute(&boxed.identity) - } + let com_object = ::windows_core::ComObject::new(this); + com_object.into_interface() } } @@ -288,6 +289,16 @@ pub fn implement(attributes: proc_macro::TokenStream, original_type: proc_macro: } } + impl #generics ::windows_core::ComObjectInterface<::windows_core::IInspectable> for #impl_ident::#generics where #constraints { + #[inline(always)] + fn as_interface_ref(&self) -> ::windows_core::InterfaceRef<'_, ::windows_core::IInspectable> { + unsafe { + let interface_ptr = &self.identity; + ::core::mem::transmute(interface_ptr) + } + } + } + impl #generics ::windows_core::AsImpl<#original_ident::#generics> for ::windows_core::IUnknown where #constraints { // SAFETY: the offset is guranteed to be in bounds, and the implementation struct // is guaranteed to live at least as long as `self`. From 8a149555a994c1e3ac6eba620a2fafe8eb086a56 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Tue, 21 May 2024 12:12:36 -0700 Subject: [PATCH 2/2] revert generics-related change --- crates/libs/implement/src/lib.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/libs/implement/src/lib.rs b/crates/libs/implement/src/lib.rs index 9609ba5427..06c6536ab4 100644 --- a/crates/libs/implement/src/lib.rs +++ b/crates/libs/implement/src/lib.rs @@ -68,7 +68,7 @@ pub fn implement(attributes: proc_macro::TokenStream, original_type: proc_macro: let vtable_news = attributes.implement.iter().enumerate().map(|(enumerate, implement)| { let vtbl_ident = implement.to_vtbl_ident(); let offset = proc_macro2::Literal::isize_unsuffixed(-1 - enumerate as isize); - quote! { #vtbl_ident::new::<#impl_ident::#generics, #original_ident::#generics, #offset>() } + quote! { #vtbl_ident::new::() } }); let offset = attributes.implement.iter().enumerate().map(|(offset, _)| proc_macro2::Literal::usize_unsuffixed(offset)); @@ -135,6 +135,11 @@ pub fn implement(attributes: proc_macro::TokenStream, original_type: proc_macro: count: ::windows_core::imp::WeakRefCount, } + impl #generics #impl_ident::#generics where #constraints { + const VTABLES: (#(#vtbl_idents2,)*) = (#(#vtable_news,)*); + const IDENTITY: ::windows_core::IInspectable_Vtbl = ::windows_core::IInspectable_Vtbl::new::(); + } + impl #generics ::windows_core::ComObjectInner for #original_ident::#generics where #constraints { type Outer = #impl_ident::#generics; @@ -147,12 +152,9 @@ pub fn implement(attributes: proc_macro::TokenStream, original_type: proc_macro: // This is why this function returns ComObject instead of returning #impl_ident. fn into_object(self) -> ::windows_core::ComObject { - static VTABLES: (#(#vtbl_idents2,)*) = (#(#vtable_news,)*); - static IDENTITY: ::windows_core::IInspectable_Vtbl = ::windows_core::IInspectable_Vtbl::new::<#impl_ident::#generics, #identity_type, 0>(); - let boxed = ::windows_core::imp::Box::new(#impl_ident::#generics { - identity: &IDENTITY, - vtables: (#(&VTABLES.#offset,)*), + identity: &#impl_ident::#generics::IDENTITY, + vtables: (#(&#impl_ident::#generics::VTABLES.#offset,)*), this: self, count: ::windows_core::imp::WeakRefCount::new(), });