get_physical_device_memory_properties return invalid heap_index

[BigAngryPanda]

In ash 0.38 memory for VkPhysicalDeviceMemoryProperties became uninitialized
let mut memory_prop = mem::MaybeUninit::uninit();

This change produce invalid data in (at least) memory_types field (specifically heap_index becomes random) which violates restrictions in VkMemoryType

As for ash 0.37 it is fine as memory is zeroed
let mut memory_prop = mem::zeroed();

Driver 545.29.06-0ubuntu0.22.04.2

[MarijnS95]

Let us first link some relevant sources:

The PR that made this MaybeUninit::uninit(): #798

The bit of code that you're bringing up for discussion:

ash/ash/src/instance.rs

Lines 440 to 452 in 331724c

	/// <https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkGetPhysicalDeviceMemoryProperties.html>
	#[inline]
	pub unsafe fn get_physical_device_memory_properties(
	&self,
	physical_device: vk::PhysicalDevice,
	) -> vk::PhysicalDeviceMemoryProperties {
	let mut memory_prop = mem::MaybeUninit::uninit();
	(self.instance_fn_1_0.get_physical_device_memory_properties)(
	physical_device,
	memory_prop.as_mut_ptr(),
	);
	memory_prop.assume_init()
	}

Also relevant to mention that the "545.29.06 driver" you're mentioning is an Nvidia driver. Other hardware vendors, which also implement the Vulkan API, have a different numbering scheme.

---

Have you looked at the memory_type_count field of vk::PhysicalDeviceMemoryProperties? It describes how many elements in memory_types are initialized when Instance::get_physical_device_memory_properties() returns. Keying off of heap_index on its own wouldn't make much sense (if it was mem::zeroed()) as heap_index 0 is valid if there's at least one heap.

Specifically we added slice getters in #858 to make it easier to extract the valid portion of this struct.

[MarijnS95]

Now, given that the fields of struct vk::PhysicalDeviceMemoryProperties are safe to access, I don't know if unsafeness of Instance::get_physical_device_memory_properties() allows us to say that the caller is responsible for only looking at the first ..memory_type_count number of elements in memory_types (same for heaps) in the returned struct instance.

[MarijnS95]

https://doc.rust-lang.org/std/pin/struct.Pin.html#method.get_unchecked_mut might be a nice example of this, where the safety docs don't specify any preconditions to uphold, but do say how you have to handle the returned value to not cause any UB.

On the other hand the safety docs for MaybeUninit::assume_init() specify that T must be fully initialized, which is not the case here.

Maybe this is one of the few cases where we should move back to ::zeroed() though these uninitialized invalid values do force callers to actually look at the _count field or (better yet) use our helper slice getters.

[BigAngryPanda]

Thanks for answering

You are right I didn't check memory_type_count so it works with zeroed memory but not with uninitialized

As it is my bad I guess issue is resolved

[MarijnS95]

I don't want to close this prematurely as there's definitely something actionable on the Ash side, even if the Vulkan specification specifically calls out that the *Count fields define how many valid elements there are in either array.

In the case that unsafe on the function is enough to return uninitialized memory in a safely-accessible struct (which I doubt, because we're already violating the safety constraints on MaybeUninit::assume_init()), the action point is to extend the method with a # Safety paragraph.

Otherwise, we could move back to MaybeUninit::zeroed() (or rather: Default::default() and drop assume_init() altogether), and still update the docs that the user is strongly encouraged to use our _as_slice() helpers instead of piecing together the size manually (or uselessly read completely zeroed vk::MemoryType elements). This wouldn't be a breaking change and can easily go into a patch release (on the side we also need to consider whether we want to solve #905).

Finally, we could wrap this struct in such a way that the fields are not exposed, and that the user can only access a slice view through a helper (also without being able to update/overwrite the *_count fields safely). After all this struct is returnedonly and doesn't need to be altered by the user (except for layers).

That would go nicely with changing the sized array type (because it now has a len attribute in vk.xml) to MaybeUninit<T>, putting even more emphasis on using the helpers. But again, we should make sure the user cannot safely update *_count in this case.

---

All of the above applies to a few more functions that return ::uninit() structs with length-bounded static-sized arrays.

[MarijnS95]

I started to inventorize all static-sized arrays that are runtime length bounded (either by a field or for strings a NULL terminator) at https://github.com/ash-rs/ash/compare/static-runtime-bounded-array-maybeuninit. Might get a little nasty to deal with this properly, as all accessor functions now have to be unsafe because we can never assume that the "bound" was initialized or tampered with.

(either by overwriting the count field or rewriting when/where the NULL terminator is)

And since there isn't always a "guarantee" that (parts of) the struct was uninitialized (or left uninitialized), specifically when the caller passes a Default::default() structure into a &mut-taking function or p_next chain, forcing these fields to always be MaybeUninit seems too harsh. It would definitely be easier to revert parts of #798 and properly document when and why we didn't zero-initialize. Above all that's a change we can push out in a patch.