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

Fix soundness hole in ComObject #3051

Merged

Conversation

sivadeilra
Copy link
Collaborator

@sivadeilra sivadeilra commented May 21, 2024

This fixes a soundness hole in ComObject. The soundness hole is that we should never allow safe Rust code to hold an owned instance of MyApp_Impl objects (implementations of COM objects) because those objects contain reference counts and provide safely-callable methods that adjust those reference counts. If Rust code holds an owned instance of such a type, then we don't control its lifetime; it may be placed on the stack, in a static, etc. and we have no control over that.

This PR closes the soundness hole by providing only one way to get access to any MyApp_Impl type -- these types are immediately placed into a heap allocation and the only way to access them is through a ComObject reference.

A few other minor improvements:

  • ComObject::as_reference() and friends now work with IInspectable.
  • Switched vtable pointers to use &'static instead of *const.
  • Converted some existing From implementations to use safe ComObject code.
  • Allows "fluid" construction of a ComObject by calling foo.into_object().

Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kennykerr kennykerr merged commit 80a293e into microsoft:master May 22, 2024
89 checks passed
@sivadeilra sivadeilra deleted the user/ardavis/fix-comobject-unsoundness branch May 23, 2024 23:05
mati865 pushed a commit to mati865/windows-rs that referenced this pull request Jun 15, 2024
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.

3 participants