Skip to content

Fix misaligned pointer dereference when storing UserData #211

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

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

rlidwka
Copy link
Contributor

@rlidwka rlidwka commented Aug 30, 2023

see discussion in #210

I've also added a test, which verifies userdata storing/retrieving with various types, and may also reproduce a problem with older code. You can run it with the following command:

cargo test --package physx --lib -- traits::user_data::tests --nocapture

line 28 is where the bug is at, lines 24-25 are changed only for consistency reasons

@rlidwka rlidwka requested a review from tgolsson as a code owner August 30, 2023 05:54
@tgolsson
Copy link
Member

tgolsson commented Aug 30, 2023

Awesome work, thanks! Quick cargo fmt and we should be good to go!

@rlidwka
Copy link
Contributor Author

rlidwka commented Aug 30, 2023

Quick cargo fmt and we should be good to go!

Okay, I've added formatting changes as the second commit.

Note: cargo fmt errors happened because rust people finally figured out "the only right way" to write let..else statements in a recent rust release, and those changes have nothing to do with the changes in the first commit.

@tgolsson
Copy link
Member

👍 Yeah; figures. Thanks for fixing anyways. I'll see if I can cut a release later this week, a bit swamped right now.

@mergify mergify bot merged commit 4cbf2c2 into EmbarkStudios:main Aug 30, 2023
@rlidwka
Copy link
Contributor Author

rlidwka commented Aug 31, 2023

I've just noticed that other two methods in the same file use double-unsafe blocks, i.e.:

unsafe fn fn get_user_data_mut(...) {
    unsafe {
        ...
    }
}

Example:

unsafe fn get_user_data_mut(this: &mut Self) -> &mut Self::UserData {
unsafe {
if size_of::<Self::UserData>() > size_of::<*mut c_void>() {
// Data is stored in a Box<UserData> on the heap, and userData is just a pointer to it.
&mut *((*this.user_data_ptr_mut()) as *mut Self::UserData)
} else {
// DATA_SIZE <= VOID_SIZE
// Data is stored directly in the userData field.
&mut *(this.user_data_ptr_mut() as *mut *mut c_void as *mut Self::UserData)
}
}
}

What's the reason for that, and should I have wrapped if..else in unsafe block in a similar way?

@tgolsson
Copy link
Member

I'm going to guess it comes from this lint: https://doc.rust-lang.org/beta/nightly-rustc/rustc_lint/builtin/static.UNSAFE_OP_IN_UNSAFE_FN.html, though it doesn't seem like we apply it consistently...

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.

2 participants