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

Make UserData pointer-packing actually sound #223

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

fu5ha
Copy link
Member

@fu5ha fu5ha commented Nov 7, 2023

This rewrites a lot of handling of user data both in the sys and high level wrapper crate. First of all, it puts all (well, as much as possible) handling of void* userData pointers through a UserData type on the Rust side. That type is this union:

#[repr(C)]
#[derive(Clone, Copy)]
pub union UserData {
    heap_pointer: *mut c_void,
    packed_data: MaybeUninit<*mut c_void>,
}

The reason this is necessary is that if we want to be able to pack arbitrary T inside the space occupied by the user data, we need to make Rust aware that all or part of that data might be uninitialized since there can be arbitrary padding bytes in T's layout.

In addition, we fix #210 actually for-real this time, including in other places that did their packing adhoc other than the UserData trait. The reason for that issue is that in order to write some T into a *mut c_void, we interpreted the T as a *mut c_void and wrote that reinterpretation into the *mut c_void location. This is backwards though -- T's size and alignment are subsets of *mut c_void, so we should instead cast a pointer to the UserData's memory (*mut UserData) as a *mut T and then write the T into that. Which is what we do now. Finally we make sure to not create even a temporary reference to the UserData when viewed as some other T when the UserData is not yet actually initialized to a valid value of that T (using addr_of_mut!).

Another notable advantage of this is that it's much more readable what's going on at higher level uses of user data, and to justify that what is happening is actually sound or not.

Next, I also went through some more of the high level wrapper's handling of user data. The UserData trait is renamed to HasUserData and is modified somewhat, implemented on top of the new UserData. Importantly, the Controller trait's handling of user data was unsound (it returned a reference to a temporary in get_user_data when the stored T was packed into the pointer). Because of the reference-to-temporary problem, I decided to follow suit of the PxFoundation user data pieces by forcing them onto the heap rather than trying to pack them.

@fu5ha fu5ha requested a review from tgolsson as a code owner November 7, 2023 23:04
physx-sys/src/lib.rs Outdated Show resolved Hide resolved
@fu5ha fu5ha mentioned this pull request Nov 9, 2023
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.

Misaligned pointer dereference
2 participants