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.

Sign up for GitHub

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

Jump to bottom

Undefined behaviour in library #9

Open
stefnotch opened this issue Nov 12, 2023 · 3 comments · May be fixed by #10
Open

Undefined behaviour in library #9

stefnotch opened this issue Nov 12, 2023 · 3 comments · May be fixed by #10

Comments

@stefnotch
Copy link
Contributor

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.

egui-winit-ash-integration/src/integration.rs

Lines 1185 to 1193 in 73c2e12

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.
@YouSafe YouSafe linked a pull request Nov 12, 2023 that will close this issue
Open
@MarijnS95
Copy link

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.

@MatchaChoco010
Copy link
Owner

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!

@MatchaChoco010
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Upgrade ash to 0.38 and egui to 0.27.2 YouSafe/egui-winit-ash-integration
3 participants
@MarijnS95 @stefnotch @MatchaChoco010