`push_next` being safe to call is unsound

[marc0246]

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]

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]

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

[Ralith]

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]

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]

Opened a draft at https://github.com/ash-rs/ash/pull/909 following my suggestion from https://github.com/ash-rs/ash/issues/905#issuecomment-2052062228, 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]

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]

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]

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.