wgpu icon indicating copy to clipboard operation
wgpu copied to clipboard
verified publisher icon gfx-rs

The ComPtr API is unsound.

Open Brezak opened this issue 1 year ago • 5 comments

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

Brezak avatar Jun 14 '24 11:06 Brezak

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 https://github.com/gfx-rs/wgpu/issues/3207).

teoxoy avatar Jun 14 '24 13:06 teoxoy

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 avatar Jun 14 '24 14:06 ErichDonGubler

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

onkoe avatar Jun 19 '24 16:06 onkoe

@onkoe per https://github.com/gfx-rs/wgpu/issues/3207#issuecomment-2170950326 the migration to the windows crate ecosystem is already being worked on 🙂

MarijnS95 avatar Jun 19 '24 16:06 MarijnS95

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! 😄

onkoe avatar Jun 19 '24 17:06 onkoe

Closing since we have https://github.com/gfx-rs/wgpu/issues/3207 to track progress on the migration.

teoxoy avatar Jul 10 '24 14:07 teoxoy