Improve IUnknown Unsafe Annotations
#1734
Conversation
|
I'm just going to check with @eholk before merging. |
Oops, I mixed this up with the other PR. I'll take a look :) |
| self.count.add_ref() | ||
| } | ||
| fn Release(&mut self) -> u32 { | ||
| unsafe fn Release(&self) -> u32 { |
There was a problem hiding this comment.
Is it possible to make Release take self? If so, I think you could make this safe.
I'm guessing that doesn't fit with lots of COM patterns, since I remember making lots of manual calls to Release back when I've done COM programming before. Still, using this object after calling Release seems suspect, since Release might drop itself, but I guess that's why this is unsafe.
There was a problem hiding this comment.
Release gets called in the Drop impl of COM objects, and we only have a &mut self in drop so I don't think it's possible for Release to take self. This is unfortunately where COM's semantics and Rust semantics don't 100% align and so we're left with a dangling pointer for a short amount of time. I think the unfortunate answer is that we just have to be careful. Luckily, users will almost never interact with this method and instead Release will get called automatically on drop.
| /// | ||
| /// This function is safe to call as long as the interface pointer is non-null and valid for writes | ||
| /// of an interface pointer. | ||
| unsafe fn QueryInterface(&self, iid: &GUID, interface: *mut *const core::ffi::c_void) -> HRESULT; |
There was a problem hiding this comment.
You could use NonNull to encode the non-null safety requirement here if you wanted.
There was a problem hiding this comment.
I consider that, but I'm not sure it's worth it since NonNull is a bit awkward to work with, and the user still has to verify the pointer is valid for writing an interface pointer to. Since NULL is not valid for writes, the safety check should already tip the caller to check for non-null.
| @@ -28,7 +28,7 @@ impl IClassFactory_Impl for Factory { | |||
| assert!(outer.is_none()); | |||
| let unknown: IInspectable = Object().into(); | |||
| // TODO: https://github.com/microsoft/windows-rs/issues/1441 | |||
| unsafe { unknown.query(&*iid, object).ok() } | |||
| unsafe { unknown.query(&*iid, object as *mut _).ok() } | |||
There was a problem hiding this comment.
It might be worth adding a SAFETY comment here, although I don't know that it'd be terribly informative. It looks like you're just calling an unsafe function, query, and following its requirements.
There was a problem hiding this comment.
I think that's a good idea, but we can take care of that when we document query's safety needs (which I personally don't yet have a good grasp on).
This PR does two things:
QueryInterfaceandReleasemethods onIUknownImplunsafe.Releasecan free memory so the user might be sure that once they callReleasethe no longer useself.QueryInterface,AddRef, andReleasetake non-exclusive references since they are effectively methods on aliased references and thus cannot be&mut self.QueryInterface's out param to be*mut *const c_voidinstead of*mut *mut c_void. I believe this more closely models what an interface out param is.cc @eholk who has expressed interest in helping review safety related PRs