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

Maybe Unsound in from_winapi_d3d12_resource_desc #255

Open
lwz23 opened this issue Dec 10, 2024 · 1 comment · May be fixed by #256
Open

Maybe Unsound in from_winapi_d3d12_resource_desc #255

lwz23 opened this issue Dec 10, 2024 · 1 comment · May be fixed by #256

Comments

@lwz23
Copy link

lwz23 commented Dec 10, 2024

Hello, thank you for your contribution in this project, I am scanning the unsoundness problem in rust project.
I notice the following code:

pub fn from_winapi_d3d12_resource_desc(
        device: *const winapi_d3d12::ID3D12Device,
        desc: &winapi_d3d12::D3D12_RESOURCE_DESC,
        name: &'a str,
        location: MemoryLocation,
    ) -> Self {
        let device = device.as_windows();
        // Raw structs are binary-compatible
        let desc = unsafe {
            std::mem::transmute::<&winapi_d3d12::D3D12_RESOURCE_DESC, &D3D12_RESOURCE_DESC>(desc)
        };
        let allocation_info =
            unsafe { device.GetResourceAllocationInfo(0, std::slice::from_ref(desc)) };
        let resource_category: ResourceCategory = desc.into();

        AllocationCreateDesc {
            name,
            location,
            size: allocation_info.SizeInBytes,
            alignment: allocation_info.Alignment,
            resource_category,
        }
    }

Considering that pub mod d3d12, and from_winapi_d3d12_resource_desc is also a pub function. I assume that users can directly call this function. This potential situation could result in device.GetResourceAllocationInfo(0, std::slice::from_ref(desc)) operating on a null pointer, I am not sure what will null_pointer.GetResourceAllocationInfo do but I guess it might trigger undefined behavior (UB). For safety reasons, I felt it necessary to report this issue. If you have performed checks elsewhere that ensure this is safe, please don’t take offense at my raising this issue.
I suggest Several possible fixes:

  1. If there is no external usage for from_winapi_d3d12_resource_desc it should not marked as pub.
  2. from_winapi_d3d12_resource_desc method should add additional check for null pointer.
  3. mark from_winapi_d3d12_resource_desc method as unsafe and proper doc to let users know that they should provide valid Pointers.
@MarijnS95
Copy link
Member

MarijnS95 commented Dec 10, 2024

Thanks for pointing that out - yeah I think this function is simply lacking unsafe. A null check won't cut it since the pointer is dereferenced: any invalid device pointer will also trigger UB.

I've been meaning to remove the winapi compatibility layer. It's messy, ugly, and UB as you've shown, and serves no purpose given that windows is superior and more complete in every aspect.

@MarijnS95 MarijnS95 linked a pull request Dec 10, 2024 that will close this issue
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 a pull request may close this issue.

2 participants