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

push_next being safe to call is unsound #905

Open
marc0246 opened this issue Apr 12, 2024 · 8 comments · May be fixed by #909
Open

push_next being safe to call is unsound #905

marc0246 opened this issue Apr 12, 2024 · 8 comments · May be fixed by #909

Comments

@marc0246
Copy link

This code triggers UB using only safe code:

use ash::vk;

let create_info = vk::DeviceCreateInfo::default();
let mut features11 = vk::PhysicalDeviceVulkan11Features {
    p_next: 1 as _,
    ..vk::PhysicalDeviceVulkan11Features::default()
};
let create_info = create_info.push_next(&mut features11);

Personally I would prefer having a function that doesn't dereference raw pointers at all, because at least we I don't think need this functionality and it would alleviate having the unsafe block when calling push_next.

@MarijnS95
Copy link
Collaborator

Yeah that is definitely a problem. Any solution involves a breaking change, and I'm not sure about yanking or releasing a breaking followup soon because of this. It's been there for quite some time.

For solving this, I see two options: mark push_next() as unsafe, and add a push_next_one() version that is safe, but asserts that p_next equals null() to not risk breaking an existing chain.

@marc0246
Copy link
Author

marc0246 commented Apr 12, 2024

That sulution sounds awesome to me! I also think yanking is futile when a bug has existed for many (breaking) releases.

@Ralith
Copy link
Collaborator

Ralith commented Apr 12, 2024

We should mark push_next as unsafe. Perhaps we can justify doing so in a patch release:

  • it's fixing a serious bug
  • it's rare to be building Vulkan structs outside of an unsafe block anyway, so cases of downstream breakage should be few.

@marc0246
Copy link
Author

Seeing as it's good practice to minimize the scope of unsafe, I wouldn't bet on it. Some people might even use #![deny(unsafe_op_in_unsafe_fn)].

@MarijnS95
Copy link
Collaborator

Opened a draft at #909 following my suggestion from #905 (comment), while keeping implementation discussions here.

I've also considered not marking push_next() as unsafe, but stripping this chain-walk pointer cast out of it (and obviously inheriting the assert!(next.p_next.is_none()) to ensure the caller is not loosing any information). I'm not entirely certain if such behavioural changes fall behind "semver compatible" (it surely is walking the edge) because the signature would no longer be changing and allow us to push this as a patch release.

After all I've never actually seen anyone building up pointer chains like this, as @Rua brought up in #906 we don't even provide push_next() functions on non-root structs making it even less likely (though we can probably come up with one or two examples where this is a possibility).

And obviously, if we go this route, there'll be an accompanying unsafe fn push_next_multiple() (name to be bikeshedded) containing the original implementation of push_next().

@Ralith
Copy link
Collaborator

Ralith commented Apr 14, 2024

I concur that the use case for pushing a whole chain at once is unclear, and a simpler method is appealing. I'm not even sure we should it's worth preserving the existing logic under another name. Still, I'm wary because (semver break or not) introducing a dynamic assert is much easier to miss than a change that actually breaks the build.

@MarijnS95
Copy link
Collaborator

Still, I'm wary because (semver break or not) introducing a dynamic assert is much easier to miss than a change that actually breaks the build.

Exactly, this is my only reason to hold back such a "sneaky" UB fix workaround.

@rikusalminen
Copy link

Just to chime in my opinion as a user of this lib: I've always assumed that .push_next() will only push one struct to the linked list, and will assert NULL or even silently overwrite anything in the pNext chain. Didn't even cross my mind that it would walk the linked list (because that's obviously unsafe).

For most use cases the compiler ought to be smart enough to strip away the assert!(pNext == null), so I don't see much value in adding a .push_next_unchecked.

Having unsafe push_next_many() is not a common pattern either, but can be added in case someone finds it useful.

tl;dr: IMO just make push_next() assert pNext == null.

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.

4 participants