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

The ComPtr API is unsound. #5813

Open
Brezak opened this issue Jun 14, 2024 · 5 comments
Open

The ComPtr API is unsound. #5813

Brezak opened this issue Jun 14, 2024 · 5 comments
Labels
api: dx12 Issues with DX12 or DXGI platform: windows Issues with integration with windows type: bug Something isn't working

Comments

@Brezak
Copy link
Contributor

Brezak commented Jun 14, 2024

Description
The ComPtr API let's you de reference a null pointer in safe code.

Repro steps

  1. Safely construct a null ComPtr with the ComPtr::null function.
    A.2 Clone your new pointer.
    A.3 Look on in horror as as_unknown de references a null pointer.
    B.2 Be a bit careless and let rust autoderef your pointer under your nose.
    B.3 Watch as your pointer Deref impl executes a rapid unplanned program exit.

Possible solutions

The simplest solution would be two fold. The clone impl should only call AddRef when the pointer isn't null. The deref impl should be replaced by a deref function that's either safe and returns a Option<&T> or is unsafe and returns a &T.

I'm also attempting a solution where ComPtr is turned into a basic wrapper over a *mut ptr and a new type is introduced that always contains a valid (non null and pointing to a valid allocation) pointer to a Interface. The problem is it touches a lot of code and I'm not very familiar with this repo.

Platform
Windows

@teoxoy
Copy link
Member

teoxoy commented Jun 14, 2024

It looks like we do have debug asserts for this but I agree our implementation is not great. Rather than rewriting it, I think we should just move over to windows-rs or a subset of it (see #3207).

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Jun 14, 2024

Speaking for myself and @cwfitzgerald (with whom I've spoken about this directly), I think we have no attachment to the d3d12 crate; we'd also rather migrate directly to the windows ecosystem, rather than fix d3d12.

@ErichDonGubler ErichDonGubler added type: bug Something isn't working api: dx12 Issues with DX12 or DXGI platform: windows Issues with integration with windows labels Jun 14, 2024
@onkoe
Copy link
Contributor

onkoe commented Jun 19, 2024

it's probably not scarier than trusting in libloading, but the bindings for d3d12 are automatically generated and kinda verbose.

keep msdn handy if you tackle this issue! https://github.com/microsoft/windows-rs/tree/master/crates/libs/windows/src/Windows/Win32/Graphics/Direct3D12

@MarijnS95
Copy link
Contributor

@onkoe per #3207 (comment) the migration to the windows crate ecosystem is already being worked on 🙂

@onkoe
Copy link
Contributor

onkoe commented Jun 19, 2024

sounds great! just wanted to warn anyone who expected a drop-in replacement

that said, your branch looks very nice already. can see why you'd start immediately! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: dx12 Issues with DX12 or DXGI platform: windows Issues with integration with windows type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants