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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion physx-sys/pxbind/src/consumer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,9 @@ impl<'ast> AstConsumer<'ast> {
// If a record decl doesn't have any inner nodes, it's just
// a foreward declaration and we can skip it
if inn.inner.is_empty() || rec.definition_data.is_none() {
let Some(name) = rec.name.as_deref() else { continue; };
let Some(name) = rec.name.as_deref() else {
continue;
};

if !self.classes.contains_key(name) {
self.recs
Expand Down
4 changes: 3 additions & 1 deletion physx-sys/pxbind/src/consumer/enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,9 @@ impl<'ast> super::AstConsumer<'ast> {
// PhysX uses a PxFlags<> template typedef to create a bitfield type for
// a specific enum, we use this typedef to also generate an appropriate
// bitflags that can be transparently passed between the FFI boundary
let Some(flags) = super::no_physx(&td.kind.qual_type).strip_prefix("PxFlags<") else { return Ok(()) };
let Some(flags) = super::no_physx(&td.kind.qual_type).strip_prefix("PxFlags<") else {
return Ok(());
};
// Get rid of `>`
let flags = &flags[..flags.len() - 1];

Expand Down
12 changes: 9 additions & 3 deletions physx-sys/pxbind/src/consumer/record.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ impl Constructor {
}
});

let Some(first) = iter.next() else { return false };
let Some(first) = iter.next() else {
return false;
};

if first.ends_with(" &&") {
return true;
Expand Down Expand Up @@ -443,7 +445,9 @@ impl<'ast> super::AstConsumer<'ast> {
return Ok(());
}

let Some(rname) = rec.name.as_deref() else { return Ok(()) };
let Some(rname) = rec.name.as_deref() else {
return Ok(());
};

anyhow::ensure!(
rec.definition_data.is_some(),
Expand Down Expand Up @@ -674,7 +678,9 @@ impl<'ast> super::AstConsumer<'ast> {
template_types: &[(&str, &super::TemplateArg<'ast>)],
fields: &mut Vec<FieldBinding<'ast>>,
) -> anyhow::Result<()> {
let Some(rname) = rec.name.as_deref() else { return Ok(()) };
let Some(rname) = rec.name.as_deref() else {
return Ok(());
};
let mut is_public = !matches!(rec.tag_used, Some(Tag::Class));

for inn in &node.inner {
Expand Down
3 changes: 3 additions & 0 deletions physx/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ into mutable and immutable variants.
- `ActorMap::as_rigid_static` -> `ActorMap::as_rigid_static_mut`
- `ActorMap::as_articulation_link` -> `ActorMap::as_articulation_link_mut`

### Fixed
- [PR#211](https://github.com/EmbarkStudios/physx-rs/pull/211) fixed misaligned pointer dereference when using `UserData` with small-size values.

## [0.18.0] - 2023-03-03
### Changed
- [PR#191](https://github.com/EmbarkStudios/physx-rs/pull/191) replaced `PxCooking` with regular functions as `PxCooking` is deprecated in the C++ code.
Expand Down
61 changes: 55 additions & 6 deletions physx/src/traits/user_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,11 @@ pub unsafe trait UserData: Sized {
unsafe fn init_user_data(&mut self, user_data: Self::UserData) -> &mut Self {
if size_of::<Self::UserData>() > size_of::<*mut c_void>() {
// Too big to pack into a *mut c_void, kick it to the heap.
let data = Box::new(user_data);
*self.user_data_ptr_mut() = Box::into_raw(data) as *mut c_void;
let data = Box::into_raw(Box::new(user_data));
*(self.user_data_ptr_mut() as *mut *mut c_void as *mut *mut Self::UserData) = data;
} else {
// DATA_SIZE <= VOID_SIZE
unsafe {
*self.user_data_ptr_mut() =
*(&user_data as *const Self::UserData as *const *mut c_void)
}
*(self.user_data_ptr_mut() as *mut *mut c_void as *mut Self::UserData) = user_data;
}
self
}
Expand All @@ -49,6 +46,7 @@ pub unsafe trait UserData: Sized {
}

/// # Safety
///
/// The user data field must have previously been initialized via `init_user_data`.
unsafe fn get_user_data_mut(this: &mut Self) -> &mut Self::UserData {
unsafe {
Expand All @@ -63,3 +61,54 @@ pub unsafe trait UserData: Sized {
}
}
}

#[cfg(test)]
mod tests {
use std::{ffi::c_void, fmt::Debug, marker::PhantomData, ptr::null_mut};

use super::UserData;

struct TestUserData<U> {
user_data: *mut c_void,
phantom: PhantomData<U>,
}

impl<U> Default for TestUserData<U> {
fn default() -> Self {
Self {
user_data: null_mut(),
phantom: PhantomData,
}
}
}

unsafe impl<U> UserData for TestUserData<U> {
type UserData = U;

fn user_data_ptr(&self) -> &*mut c_void {
&self.user_data
}

fn user_data_ptr_mut(&mut self) -> &mut *mut c_void {
&mut self.user_data
}
}

fn do_test<U: PartialEq + Clone + Debug>(user_data: U) {
unsafe {
let mut object: TestUserData<U> = TestUserData::default();
object.init_user_data(user_data.clone());

assert_eq!(UserData::get_user_data(&object), &user_data);
assert_eq!(UserData::get_user_data_mut(&mut object), &user_data);
}
}

#[test]
fn test_user_data() {
do_test(()); // unit type
do_test(100u8); // smaller than pointer
do_test(100usize); // same size as pointer
do_test([100usize; 4]); // larger than pointer
}
}