Misaligned pointer dereference about physx-rs HOT 24 CLOSED

Please rerun with RUST_BACKTRACE=1 so we can see the callstack.

from physx-rs.

Thanks for the update! The misalignment here makes me wonder whether the passed in material itself is dead or incorrect. Is there a repository where I can run this code with a debugger attached?

@rlidwka CC :-)

from physx-rs.

@tgolsson, can you maybe spot anything that can cause this error in the following else branch? UserData being stored here is empty tuple, ().

This is a spurious error. It failed at the first material being created, only on one machine, only with one example, only in debug mode, and it worked on the same machine before. I cannot reproduce it myself.

The line it fails at is here:
https://github.com/rlidwka/bevy_mod_physx/blob/467a452eb94b069a6c997eadb0dcd13211e44673/src/lib.rs#L334

Be aware that that branch uses older physx-rs version that works with physx4 (because bindings for vehicles from physx5 don't exist yet), but relevant code in physx-rs hasn't changed since then.

If you want, you can run cargo run --example vehicle on the repository https://github.com/rlidwka/bevy_mod_physx, branch physx4_20230717, but I wouldn't expect it to do very much.

from physx-rs.

gonna debug it later today

from physx-rs.

I'm really confused to be honest. Either the write is misaligned, or the read is... () has alignment of 1, but I don't think it could ever be aligned < 8 without breaking stack alignment in this code. Similarly, the write target is definitely offset by 16 from the start of the struct, which means that the PxMaterial would also be misaligned, which seems odd.

from physx-rs.

Ok, as usual, the problem is apparent after refuting it. The read must be the problem (if the material is valid). We're reading an 8 byte word from a location containing 1 byte. And reading those bytes together enforces the 8-byte alignment. Doesn't matter how we get to the invalid alignment, it's clearly UB. We should probably cast the target instead of the source... writing 1 byte to an 8-byte location is legal.

@rlidwka if you have repro, do you care to try switching the cast to the other side and seeing what falls out?

from physx-rs.

do you care to try switching the cast to the other side and seeing what falls out?

@tgolsson, on @fgadaleta's machine read seems to have alignment of 1 for the reasons I don't fully understand.

The following code works. Is this what you had in mind?

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::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
        *(self.user_data_ptr_mut() as *mut *mut c_void as *mut Self::UserData) = user_data;
    }
    self
}

from physx-rs.

@tgolsson, on @fgadaleta's machine read seems to have alignment of 1 for the reasons I don't fully understand.

You mean that the pointee has a 1-byte alignment? That's correct given that () has 1-byte alignment. I can't repro it actually having that alignment using std::mem::align_of-- when we take the address it would be spilled to stack and be 8-byte aligned as far as I can tell. But it might be debug vs release differences, architecture, etc.

The following code works. Is this what you had in mind?

<snip>

Yes, pretty much. The userData is going to be aligned by 8-bytes inside its parent object in every situation, and always 8 bytes big.

from physx-rs.

@tgolsson, on @fgadaleta's machine read seems to have alignment of 1 for the reasons I don't fully understand.

You mean that the pointee has a 1-byte alignment? That's correct given that () has 1-byte alignment. I can't repro it actually having that alignment using std::mem::align_of-- when we take the address it would be spilled to stack and be 8-byte aligned as far as I can tell. But it might be debug vs release differences, architecture, etc.

The following code works. Is this what you had in mind?

<snip>

Yes, pretty much. The userData is going to be aligned by 8-bytes inside its parent object in every situation, and always 8 bytes big.

Indeed in release there is no alignment problem.

from physx-rs.

I have just updated Rustc 1.73 and have the same problem. I have never had it before. The problem is exactly the same - creating a material without user data (so with a unit struct). I am also confused now as to why this has never been a problem before. And what should I do now? Also, why doesn't the example have the same problem? Or does it? I'll check.

The example crashes funny:

❯ /home/user/workspace/physx-rs/target/debug/examples/ball_physx
zsh: IOT instruction (core dumped)  /home/user/workspace/physx-rs/target/debug/examples/ball_physx

Thread 1 "ball_physx" received signal SIGABRT, Aborted.
__pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0)
    at pthread_kill.c:44
44	      return INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0;
(gdb) bt
#0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6,
    no_tid=no_tid@entry=0) at pthread_kill.c:44
#1  0x00007ffff79bf8a3 in __pthread_kill_internal (signo=6, threadid=<optimized out>)
    at pthread_kill.c:78
#2  0x00007ffff796f668 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x00007ffff79574b8 in __GI_abort () at abort.c:79
#4  0x00005555555f6a06 in physx::PxCreateDynamic (sdk=..., transform=..., geometry=..., material=...,
    density=10, shapeOffset=...)
    at /home/user/workspace/physx-rs/physx-sys/physx/physx/source/physxextensions/src/ExtSimpleFactory.cpp:91
#5  0x00005555555ad001 in physx::rigid_dynamic::RigidDynamic::new<physx::rigid_dynamic::PxRigidDynamic<(), physx::shape::PxShape<(), physx::material::PxMaterial<()>>>, physx::physics::PhysicsFoundation<physx::foundation::DefaultAllocator, physx::shape::PxShape<(), physx::material::PxMaterial<()>>>, physx_sys::PxSphereGeometry> (physics=0x7fffffffd958, transform=..., geometry=0x7fffffffdbd4,
    material=0x555555d40510, density=10, shape_transform=..., user_data=())
    at physx/src/rigid_dynamic.rs:114
#6  0x00005555555a544a in physx::physics::Physics::create_rigid_dynamic<physx::physics::PhysicsFoundation<physx::foundation::DefaultAllocator, physx::shape::PxShape<(), physx::material::PxMaterial<()>>>, (), physx_sys::PxSphereGeometry> (self=0x7fffffffd958, transform=..., geometry=0x7fffffffdbd4,
    material=0x555555d40510, density=10, shape_transform=..., user_data=()) at physx/src/physics.rs:497
#7  0x00005555555afda3 in ball_physx::main () at physx/examples/ball_physx.rs:104

from physx-rs.

😕 Are you able to repro with the test too?

from physx-rs.

I am also confused now as to why this has never been a problem before.

Rust added these asserts in debug mode in Rust 1.70 (rust-lang/rust#98112).

from physx-rs.

I am also confused now as to why this has never been a problem before.

Rust added these asserts in debug mode in Rust 1.70 (rust-lang/rust#98112).

I have always been using the latest Rust and started seeing this issue just now after the upgrade to 1.73.

from physx-rs.

😕 Are you able to repro with the test too?

By test, what exactly do you mean?

from physx-rs.

There's a unit test here:

from physx-rs.

There's a unit test here:

physx-rs/physx/src/traits/user_data.rs

Lines 65 to 113 in 1ef3b01

	#[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
	}

This one passes, but another one fails:

test flags ... FAILED

failures:

---- flags stdout ----
thread 'flags' panicked at physx-sys/pxbind/tests/enums.rs:58:6:
called `Result::unwrap()` on an `Err` value: failed to consume Record { id: None, name: Some("PxProcessPxBaseCallback"), tag_used: Some(Class), definition_data: Some(DefinitionData { dtor: Dtor { irrelevant: false, simple: false }, is_abstract: true, is_polymorphic: true }), bases: [] }

Caused by:
    0: failed to parse parameter 'anon_param0 (PxBase &)' for function 'process'
    1: Unknown type 'Simple("PxBase")'
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    flags

test result: FAILED. 3 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.71s

error: test failed, to rerun pass `-p pxbind --test enums`

Note that the ball_physx example fails.

from physx-rs.

This looks like a different issue; cc @Jake-Shadle -- do you recognize it from rewriting the pxbind setup?

from physx-rs.

This looks like a different issue; cc @Jake-Shadle -- do you recognize it from rewriting the pxbind setup?

Please note that there are two issues - one is the example of ball_physx.rs, and another is this test. Neither works fine.

One piece of news: this is probably related to the changes in 1.73. Before the update of Rustc, I had never had this issue. Now, I also have other issues with Vulkan, really-really-really weird ones (my code is plain simple, and I can't use it wrong, besides the fact that since 1.6x and until just 1.73, it has always been working). I suspect a regression in 1.73.

UPD: 1.72.1 works okay (in debug), I don't have this issue. Contrary, 1.73, 1.74 and 1.75 have the same behaviour.

from physx-rs.

I can repro the issue with flags test, but we'll track that in the other issue. I can't repro the issue with ball_physx, and detect no issues with sanitizers. What platform/architecture are you using?

from physx-rs.

I can repro the issue with flags test, but we'll track that in the other issue. I can't repro the issue with ball_physx, and detect no issues with sanitizers. What platform/architecture are you using?

Interesting, what that might be then? ... 🤔
I am using AMD 7950x3d, ArchLinux (the latest), and the latest Rust via Rustup.

from physx-rs.

Fixed the ball_physx.rs issue myself. Now, the only problem is this pointer.

If we trust gdb, then there is probably a bit more information on where this misaligned pointer dereference happens:

Thread 1 "test-binary" hit Breakpoint 1.3, physx::traits::user_data::UserData::init_user_data<physx::material::PxMaterial<()>> (self=0x5555623d5170, user_data=())
    at /home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/physx-0.18.0/src/traits/user_data.rs:30
30	                    *(&user_data as *const Self::UserData as *const *mut c_void)
(gdb) n
30	                    *(&user_data as *const Self::UserData as *const *mut c_void)
(gdb)
INFO: Message: misaligned pointer dereference: address must be a multiple of 0x8 but is 0x7fffffffa606
Location: Location { file: "/home/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/physx-0.18.0/src/traits/user_data.rs", line: 30, col: 21 }
thread caused non-unwinding panic. aborting.

Thread 1 "test-binary" received signal SIGABRT, Aborted.
__pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
44	      return INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0;

I guess the problem is that the fix for the issue hasn't been published as a new version of the crate, and I still depend on the version in crates.io instead of this git repository. Will the fix ever be uploaded to crates.io?

from physx-rs.

One last update: the problem is actually fixed but hasn't been published to crates.io. I do not see this issue anymore with the changes made to fix it earlier by changing the dependency from crates.io to this git repository. I, however, would like to depend on it through crates.io to avoid some in-between breakages.

from physx-rs.

Ah, ok. Closing again! I've wanted to make a release but haven't had the time. Will close this and try to do a release today.

from physx-rs.

Sure thing! I apologise for having taken your time, but I didn't know I had been using an outdated version all this time. Thank you!

from physx-rs.