[Merged by Bors] - [bevy_core/bytes] Fix UB with accessing memory with incorrect alignment #1966
Conversation
alice-i-cecile
added
C-Bug
NathanSWard
force-pushed
the
nward/bevy_core-bytes-ub
branch
from
ec1388f
to
01c8937
Compare
crates/bevy_core/src/bytes.rs
Outdated
| let len = bytes.len() / std::mem::size_of::<T>(); | ||
| let slice = core::slice::from_raw_parts::<T>(byte_ptr, len); | ||
| slice.to_vec() | ||
| let (_, body, _) = bytes.align_to::<T>(); |
There was a problem hiding this comment.
Should there be any assertions that the head and tail of this tuple are empty slices?
There was a problem hiding this comment.
There is abaolutely no guarantee that the passed in slice is aligned. This should use ptr::read_unaligned instead.
There was a problem hiding this comment.
I think, like my other comment, that align_to is just the wrong approach here.
As a sketch, I think that perhaps it should be more like
assert_eq!(bytes.len() % std::mem::size_of::<T>(), 0);
let len = bytes.len() / std::mem::size_of::<T>();
let mut v = Vec::<T>::with_capacity(len);
ptr::copy_nonoverlapping(bytes.as_ptr(), v.as_mut_ptr() as *mut u8, len * std::mem::size_of::<T>());
v.set_len(len);
v
because it needs a 1-aligned copy to read from the slice correctly.
There was a problem hiding this comment.
For copy_nonoverlapping doesn't T have to be Copy?
Currently T is only bounded on Clone.
crates/bevy_core/src/bytes.rs
Outdated
| let ptr = byte_ptr as *const Self; | ||
| (*ptr).clone() | ||
| let ptr = bytes.as_ptr() as *const Self; | ||
| std::ptr::read_unaligned(ptr) |
There was a problem hiding this comment.
Now that I think about it. I still don't think this is right.
This only works if T is Copy, however, T is only bounded on Clone :/
There was a problem hiding this comment.
So perhaps something like:
fn from_bytes(bytes: &[u8]) -> Self {
let value = unsafe { bytes.as_ptr().cast::<Self>().read_unaligned() };
let clone = value.clone();
std::mem::forget(value);
clone
}would work?
NathanSWard
force-pushed
the
nward/bevy_core-bytes-ub
branch
2 times, most recently
from
d7425d3
to
746905f
Compare
NathanSWard
force-pushed
the
nward/bevy_core-bytes-ub
branch
from
746905f
to
4d293c3
Compare
|
I ended up changing the requirement on |
|
This looks good to me. It would be nice to retain Clone (and it feels like it should be possible to do that in a non-ub way), but safety comes first and I'm clearly not an expert here 😄 I think I'll wait for @bjorn3 to weigh in before merging (unless we don't hear back in the next day or so). |
Yep, I agree that retaining |
|
bors r+ |
…nt (#1966) After running `bevy_core` through `miri`, errors were reported surrounding incorrect memory accesses within the `bytes` test suit. Specifically: ``` test bytes::tests::test_array_round_trip ... error: Undefined Behavior: accessing memory with alignment 1, but alignment 4 is required --> crates/bevy_core/src/bytes.rs:55:13 | 55 | (*ptr).clone() | ^^^^^^ accessing memory with alignment 1, but alignment 4 is required | ``` and ``` test bytes::tests::test_vec_bytes_round_trip ... error: Undefined Behavior: accessing memory with alignment 2, but alignment 4 is required --> /home/nward/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:95:14 | 95 | unsafe { &*ptr::slice_from_raw_parts(data, len) } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ accessing memory with alignment 2, but alignment 4 is required | ``` Solution: The solution is to use `slice::align_to` method to ensure correct alignment.
|
I don't think |
|
Oops |
|
@cart it may be necessary to bors r+ it again. |
Cool. Lets just leave it as-is for now then. We really only use Byteable for writing data to the gpu and I expect that we will eventually encourage (1) defining separate types for the gpu data and (2) using a derive macro that properly lays out the memory according to gpu layout requirements (ex
Cool Ill watch it and retry if it doesn't work out. |
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
|
There's also work ongoing in the safe transmute project (see their recent MCP: rust-lang/compiler-team#411) to provide traits and APIs for this stuff in |
…nt (bevyengine#1966) After running `bevy_core` through `miri`, errors were reported surrounding incorrect memory accesses within the `bytes` test suit. Specifically: ``` test bytes::tests::test_array_round_trip ... error: Undefined Behavior: accessing memory with alignment 1, but alignment 4 is required --> crates/bevy_core/src/bytes.rs:55:13 | 55 | (*ptr).clone() | ^^^^^^ accessing memory with alignment 1, but alignment 4 is required | ``` and ``` test bytes::tests::test_vec_bytes_round_trip ... error: Undefined Behavior: accessing memory with alignment 2, but alignment 4 is required --> /home/nward/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:95:14 | 95 | unsafe { &*ptr::slice_from_raw_parts(data, len) } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ accessing memory with alignment 2, but alignment 4 is required | ``` Solution: The solution is to use `slice::align_to` method to ensure correct alignment.
After running
bevy_corethroughmiri, errors were reported surrounding incorrect memory accesses within thebytestest suit.Specifically:
and
Solution:
The solution is to use
slice::align_tomethod to ensure correct alignment.