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

Implement macro: generated From<&mut Foo> for IFoo implementation seems sketchy #1290

Closed
samlh opened this issue Nov 7, 2021 · 2 comments
Closed
Labels
bug Something isn't working

Comments

@samlh
Copy link

samlh commented Nov 7, 2021

This generated impl looks sketchy - what happens if you call it on a value that isn't wrapped in the ..._box?

impl <#constraints> ::std::convert::From<&mut #impl_ident> for #interface_ident {

e.g.

#[implement(Windows::UI::Xaml::Data::ICustomProperty)]
pub struct CustomProperty {
  bar: ()
}

Generates:

impl< > ::std::convert::From< &mut CustomProperty:: < > >for Windows::UI::Xaml::Data::ICustomProperty {
  fn from(implementation: &mut CustomProperty:: < >) -> Self {
    unsafe {
      let mut ptr = (implementation as*mut_as*mut::windows::runtime::RawPtr).sub(2+1)as*mut CustomProperty_box:: < > ;
      (*ptr).count.add_ref();
      ::std::mem::transmute_copy(& ::std::ptr::NonNull::new_unchecked(&mut(*ptr).vtables.0 as*mut_as_))
    }
  }
}

Mistaken usage:

let foo = CustomProperty { bar: () };
ICustomProperty::from(foo).DoSomething();      // OK
ICustomProperty::from(&mut foo).DoSomething(); // Out-of-bounds access?

Happily, I've never actually needed this conversion, but if it is kept, I recommend making it unsafe like the to_impl method.

@samlh samlh changed the title Implement macro: generated From< &mut CustomProperty> for IFoo implementation seems sketchy Implement macro: generated From<&mut Foo> for IFoo implementation seems sketchy Nov 7, 2021
@riverar riverar added the bug Something isn't working label Nov 7, 2021
@kennykerr
Copy link
Collaborator

This trait is no longer being generated. Let me know if there any other concerns with the new implement macro. #1345

@samlh
Copy link
Author

samlh commented Jan 29, 2022

Thanks for the update!

I've updated to using the new trait-based implement macro, and I'm quite happy with the results.

Thanks for the hard work - the windows crate just keeps getting better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants