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

AsIUnknown for safer ComPtr s #961

Open
wants to merge 1 commit into
base: 0.3
Choose a base branch
from

Conversation

MaulingMonkey
Copy link
Contributor

#571 originally suggested AsRef<T>, and was rejected as a lot of boilerplate to merely streamline niche use cases. This PR adds minimal code to allow "properly" handling something much less niche: COM smart pointers.

There are unfortunately interfaces which do not inherit from IUnknown (interfaces like ID3D12FunctionReflection come to mind). There is currently no way to assert in the type system that an interface eventually derefs to IUnknown [...]

Actually, there is! While the compiler will reject having two "potentially conflicting" impls of AsIUnknown if implemented in an exterior crate (on the basis that a future winapi version might add impl Deref for IUnknown), we can add such traits to winapi itself just fine without conflict. This PR would allow writing a ComPtr that rejects types like ID3D12FunctionReflection:

use winapi::shared::d3d9::IDirect3D9Ex;
use winapi::shared::d3d9::IDirect3D9;
use winapi::um::d3d12shader::ID3D12FunctionReflection;
use winapi::um::unknwnbase::IUnknown;

use winapi::um::unknwnbase::AsIUnknown;
use winapi::Interface;

use std::ptr::NonNull;

fn a(_: &ComPtr<IUnknown>) {} // Deref x0
fn b(_: &ComPtr<IDirect3D9>) {} // Deref x1
fn c(_: &ComPtr<IDirect3D9Ex>) {} // Deref x2

//error[E0277]: the trait bound `winapi::um::d3d12shader::ID3D12FunctionReflection: std::ops::Deref` is not satisfied
//fn d(ptr: &ComPtr<ID3D12FunctionReflection>) {}
//          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::ops::Deref` is not implemented for
//                                            `winapi::um::d3d12shader::ID3D12FunctionReflection`

struct ComPtr<I: Interface + AsIUnknown>(NonNull<I>);
//                           ---------- required by this bound in `ComPtr`

// note: required because of the requirements on the impl of `winapi::um::unknwnbase::AsIUnknown` for
//       `winapi::um::d3d12shader::ID3D12FunctionReflection`

This would help avoid some nasty corner cases at compile time.

Future Directions

This alone isn't enough to allow wio::com::ComPtr::new to be made safe, as there's nothing stopping you from implementing Deref returning a totally bogus/dangling IUnknown vtable in 100% safe rust:

struct Evil {
    data: std::sync::Mutex<String>,
}
impl Interface for Evil {
    fn uuidof() -> winapi::shared::guiddef::GUID { IUnknown::uuidof() }
}
impl std::ops::Deref for Evil {
    type Target = IUnknown;
    fn deref(&self) -> &IUnknown { &IUnknown { lpVtbl: 42 as _ } }
}
fn explode1(e: &ComPtr<Evil>) {
    let _ = e.clone(); // bogus vtable presumably used
}
fn explode2(unk: &ComPtr<IUnknown>) {
    let evil = unk.cast::<Evil>().unwrap(); // bogus uuidof allows miscast
    *evil.data.lock().unwrap() = format!("Explode");
}

That could potentially be "fixed" by making one of the traits involved unsafe.

Option A: Breaking changes requiring winapi 0.4

-pub trait Interface {
+pub unsafe trait Interface {
     // Returns the IID of the Interface
     fn uuidof() -> shared::guiddef::GUID;
     (@uuid $interface:ident
         $l:expr, $w1:expr, $w2:expr,
         $b1:expr, $b2:expr, $b3:expr, $b4:expr, $b5:expr, $b6:expr, $b7:expr, $b8:expr
     ) => (
-        impl $crate::Interface for $interface {
+        unsafe impl $crate::Interface for $interface {

Option B: 0.3-friendly "replacement" trait

  • Only need to implement one of the traits
  • Awkward duplication of API shape
/// ### Safety
/// * By implementing this interface, you assert that the type is a valid COM interface
/// * Any `Deref<Target = IUnknown>` implementations etc. must return valid, sound-to-use COM interfaces
/// ...
unsafe trait SoundInterface : Interface {
    // Returns the IID of the Interface
    fn uuidof() -> shared::guiddef::GUID;
}

impl<I: SoundInterface> Interface for I {
    fn uuidof() -> shared::guiddef::GUID { <I as SoundInterface>::uuidof() }
}
     (@uuid $interface:ident
         $l:expr, $w1:expr, $w2:expr,
         $b1:expr, $b2:expr, $b3:expr, $b4:expr, $b5:expr, $b6:expr, $b7:expr, $b8:expr
     ) => (
-        impl $crate::Interface for $interface {
+        unsafe impl $crate::SoundInterface for $interface {

Option C: 0.3-friendly Marker Trait

  • Awkward for non-macro users (have to implement two traits)
  • Macro users don't use unsafe anywhere (unless "interface" might be replaced with "unsafe interface"?)
/// ### Safety
/// * By implementing this interface, you assert that the type is a valid COM interface
/// * Any `Deref<Target = IUnknown>` implementations etc. must return valid, sound-to-use COM interfaces
/// ...
unsafe trait SoundInterface : Interface {}
     (@uuid $interface:ident
         $l:expr, $w1:expr, $w2:expr,
         $b1:expr, $b2:expr, $b3:expr, $b4:expr, $b5:expr, $b6:expr, $b7:expr, $b8:expr
     ) => (
+        unsafe impl $crate::SoundInterface for $interface {}
         impl $crate::Interface for $interface {

@MaulingMonkey
Copy link
Contributor Author

MaulingMonkey commented Nov 18, 2020

I'll also note that an alternative implementation per the rambling (Sound)Interface suggestions might be desirable, that would limit AsIUnknown to "sound" implementations?

pub trait AsIUnknown : SoundInterface {
    fn as_iunknown(&self) -> &IUnknown;
}
impl AsIUnknown for IUnknown {
    fn as_iunknown(&self) -> &IUnknown { self }
}
impl<I> AsIUnknown for I where I : ::core::ops::Deref, I::Target : AsIUnknown, I : SoundInterface {
    fn as_iunknown(&self) -> &IUnknown { AsIUnknown::as_iunknown(&**self) }
}

@MaulingMonkey
Copy link
Contributor Author

I've recently discovered another advantage of introducing AsIUnknown: not all interfaces that derive from IUnknown have publicly available interface IIDs that can be passed to QueryInterface or CoCreateInterface. This makes winapi::Interface awkward to implement - you can synthesize a fake GUID from the ether, but anything using said GUIDs at runtime will presumably always fail, indicative of a bug.

A basic COM pointer does not need such IIDs for most methods, and could instead only require winapi::AsIUnknown. Individual fns on said COM pointer requiring such IIDs can then require winapi::Interface. I've put this to practice for mcom::Rc in MaulingMonkey/mcom@82e6368

This presumably allows safely casting from IXAudio2SourceVoice -> IXAudio2Voice -> IUnknown, while turning the reverse into a compile time error due to IXAudio2*Voice lacking publicly available IIDs by which such QueryInterface could succeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant