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

Fix UB in Instance::supports_<prop>() #170

Merged
merged 1 commit into from
Sep 15, 2024

Conversation

zarik5
Copy link
Contributor

@zarik5 zarik5 commented Sep 15, 2024

Closes #169

This solution works but it would be even more concise if the structs sys::System<Prop> would implement Default. The Default impl should already set the correct ty, and maybe the out() methods should base off the default value and return T instead of MaybeUninit<T>.
As per review, I switched to checking if the extension is present instead.

@Ralith
Copy link
Owner

Ralith commented Sep 15, 2024

Does the spec guarantee defined behavior when passing an extension struct from a disabled extension at all? I think it would be safer to check if the extension is enabled and bail out without making any XR calls at all if not.

@zarik5
Copy link
Contributor Author

zarik5 commented Sep 15, 2024

Ok. Should I revert back to using T::out()?

@Ralith
Copy link
Owner

Ralith commented Sep 15, 2024

That would be more consistent, yeah.

@zarik5 zarik5 force-pushed the fix-supports-hand-tracking-ub branch from 74db542 to baa266f Compare September 15, 2024 21:01
@Ralith
Copy link
Owner

Ralith commented Sep 15, 2024

It looks like the answer to my question above is actually "yes":

A runtime must ignore all unrecognized structures in a next chain, including those associated with an extension that has not been enabled.

So either approach seems fine. That said, the current form is concise and might make user error a bit more obvious, so I'm happy with it.

@Ralith Ralith merged commit e87c118 into Ralith:master Sep 15, 2024
6 checks passed
@zarik5 zarik5 deleted the fix-supports-hand-tracking-ub branch September 15, 2024 22:04
@zarik5
Copy link
Contributor Author

zarik5 commented Sep 16, 2024

I'm sorry I made a mistake. I opened another PR to check the correct extension

@zarik5 zarik5 mentioned this pull request Sep 17, 2024
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 this pull request may close these issues.

Instance::supports_hand_tracking might have undefined behavior
2 participants