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

vulkan: Don't use pointer to dropped PhysicalDeviceDriverProperties. #3076

Merged
merged 3 commits into from
Oct 12, 2022

Conversation

jimblandy
Copy link
Member

@jimblandy jimblandy commented Oct 6, 2022

Detected by ASAN, but seems impossible to exploit.

Since ash::vk::definitions::PhysicalDeviceDriverProperties is Copy, the statement

if let Some(driver) = phd_capabilities.driver { ... }

actually makes driver a local copy of the struct. The code uses as_ptr to create a pointer to the driver_name field of this local copy, and then tries to use that pointer outside the if let, when the local copy has gone out of scope. This is UB.

Taking a reference to the properties struct is correct and more efficient.

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
Link to the issues addressed by this PR, or dependent PRs in other repositories

Description
Describe what problem this is solving, and how it's solved.

Testing
Explain how this change is tested.

@jimblandy jimblandy added type: bug Something isn't working area: correctness We're behaving incorrectly api: vulkan Issues with Vulkan labels Oct 6, 2022
@jimblandy
Copy link
Member Author

We could avoid those unsafe blocks altogether if CStr::from_bytes_until_nul were stable.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

This is good as is, but it might be useful to have our own utility function that implements from_bytes_until_nul so that we get the lifetime and don't need the unsafe anymore

Detected by ASAN, but seems impossible to exploit.

Since `ash::vk::definitions::PhysicalDeviceDriverProperties` is
`Copy`, the statement

    if let Some(driver) = phd_capabilities.driver { ... }

actually makes `driver` a local copy of the struct. The code uses
`as_ptr` to create a pointer to the `driver_name` field of this local
copy, and then tries to use that pointer outside the `if let`, when
the local copy has gone out of scope. This is UB.

Taking a reference to the properties struct is correct and more
efficient.
Remove a handful of `unsafe` blocks from the Vulkan back end.
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Music to my eyes!

@cwfitzgerald
Copy link
Member

CI is mad

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) October 12, 2022 20:34
@cwfitzgerald cwfitzgerald merged commit f3d455b into gfx-rs:master Oct 12, 2022
@jimblandy jimblandy deleted the hal-vk-asan-dead-copy branch October 13, 2022 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: vulkan Issues with Vulkan area: correctness We're behaving incorrectly type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants