MatchaChoco010/egui-winit-ash-integration

Undefined behaviour in library

Opened this issue · 3 comments

This library uses Ash. One very tricky tidbit is that the current Ash builder pattern lends itself to accidentally causing undefined behaviour. This causes egui-winit-ash-integration to throw a validation error, followed by a rather impolite crash.

let dsc_alloc_info = vk::DescriptorSetAllocateInfo::builder()
.descriptor_pool(self.descriptor_pool)
.set_layouts(&[self.descriptor_set_layouts[0]])
.build();
unsafe {
self.device
.allocate_descriptor_sets(&dsc_alloc_info)
.unwrap()[0]
}

The issue here is that .set_layouts(&[self.descriptor_set_layouts[0]]) causes an array to get created, and then a reference to it is passed to the set_layouts function. The set_layouts function then takes a raw pointer to it, and casually assumes that the raw pointer will remain valid.
The problem is that Rust drops the array right after the function is done. At this point, that raw pointer points at arbitrary memory.
Thus dsc_alloc_info ends up having an invalid pointer, which then causes a crash.

Btw, I recently ran into that kind of issue in one of my projects. This issue is so common that Ash will change the builder pattern to something much safer in the next release.

More specifically the builder pattern disallows this via the borrow checker, but the .build() call throws that lifetime away.

Per Ash's README .build() should rarely ever be used as builders automatically Deref into their underlying structure (when passed to a nested builder call or Vulkan function), it is only tricky when having to construct an array of "built" objects.

Fortunately we unified builders with their underlying struct and removed the .build() call entirely in the next pending release, making it impossible to violate lifetimes with builders without upsetting the borrow checker.

Sorry for the late reply to your Issue!
And thanks for pointing out the problem!

It looks like #10 will resolve the issue, so I'll check the PR there.

And then I'm looking forward to the next ash release!

Unfortunately, some of the PRs in #10 are not the correct fixes, so it doesn't look like we'll be able to merge them in anytime soon.
@stefnotch If you're having trouble with the crash and want to fix it right away, you may have to get it fixed yourself.
If you have it fixed at hand and are using it, I'd be happy to contribute the fixes together in a PR!

I for one am looking forward to the release of the 0.38 series of ash.
Once they get to 0.38 and get rid of build(), the problems you point out in this issue will seem to be solved.
There will be breaking API changes, and when ash's 0.38 is released, I will rework the code of this crate at that time.
At the latest, this issue will be resolved with that ash 0.38 release.

@stefnotch @MarijnS95 Thank you both for the pointers and clear explanations.