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

Be smarter about the requirement for unsafe keyword when declaring interface methods #1782

Open
kennykerr opened this issue May 25, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@kennykerr
Copy link
Collaborator

Originally posted by @rylev in #1486 (comment)

@kennykerr kennykerr added enhancement New feature or request unsafe and removed enhancement New feature or request labels May 25, 2022
@rbrownwsws
Copy link

I'm not sure exactly what you mean by "smarter" but I noticed in #1506 you talk about making functions which have pointers in their signature unsafe. I think this may not be enough.

While writing an implementation for IShellFolder_Impl I sometimes have to use self.cast<I>().
For example in my implementation of IShellFolder_Impl::CreateViewObject() I need to pass an IShellFolder to SHCreateShellFolderView().
This immediately makes the "safe" trait function unsafe and I cannot see any way to shield the caller from this.
This means that it is possible for a rust-ified COM interface trait to have a function with no pointers in its signature that is still unsafe to implement e.g.:

#[windows::core::implement(IExample)]
struct MyComClass {
    // ...
}

impl IExample_Impl for MyComClass{
    // I am a "safe" COM interface function
    fn CreateFoo(&self) -> windows::core::Result<IFoo> {
        // DANGER!
        // Relies on us being wrapped up in a type generated by the `implement`
        // macro but we have no way of knowing if we are.
        // If we are not we will do bad pointer arithmetic.
        // We just have to trust the caller to keep this invariant.
        let i_example: IExample = unsafe { self.cast::<IExample>().unwrap() };

        let i_foo: IFoo = CreateDefaultFoo(i_example);

        Ok(i_foo)
    }

    // ...
}

// ...

fn main() {
    let my_com_class = MyComClass {};

    // Ooops, I forgot to convert into a real COM interface:
    // let my_com_class: IExample = my_com_class.into();

    // ...

    // DANGER!
    // I have broken the invariant of MyComClass::cast<I>()
    // UB ahead...
    my_com_class.CreateFoo();
}

It seems like it would be very difficult to predict if an implementer of a COM interface might ever need to use self.cast<I>() so you might have to make all functions in COM interface traits unsafe just in case.

@rylev
Copy link
Contributor

rylev commented Jun 20, 2022

This is indeed true, and something we need to fix. We can either make the function unsafe and rely on the user only calling it when a heap allocated and pinned COM interface, or we can somehow ensure that &self refers to the right thing. The former is certainly easier to implement, but if we can get the latter to work than we've made things safer for everyone.

@kennykerr kennykerr changed the title Be smarter about the requirement for unsafe keyword when declaring interface methods Be smarter about the requirement for unsafe keyword when declaring interface methods Jul 29, 2022
@kennykerr kennykerr added enhancement New feature or request and removed unsafe labels Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants