Open shinmao opened 2 months ago
We also considered that array_as_u8_slice
and value_as_u8_slice
unsound with usages of casting. The trait of Sized
doesn't pose the limitation on composite types.
Noticed this was tagged as a good first issue, I have made some changes to the function referenced by @shinmao. Instead of adding a dependency on bytemuck, I think I was able to handle the padding issue without the use of any unsafe blocks. Ran into a lot of issues trying to handle any possible value, but noticed it is only referenced three times. Out of each case the vec is either a usize or an f32. If other types need to be supported I can add support.
I have run a modified version of the FPS tutorial as well as the RPG tutorial and all the logs appear the same as running off the latest release. All tests are passing, and I added two tests to cover the transmute_vec_as_bytes.
If I am barking up the wrong tree with this approach let me know and I can follow the suggestion by @orzogc.
I have this playground showing the current function as is and its output as well as the updated function.
I linked my PR to my forked repo and it is in the history above, should I open a PR to the main fork to receive feedback? I haven't made any documentation updates as I wanted to check with everyone if this is a valid solution. Once everything looks good to people more familiar with the project I will add a documentation update to the PR.
functions downstream of transmute_vec_as_bytes:
Hi, we are researchers from SunLab. We consider the safe function
transmute_vec_as_bytes
includes the unsound implementation.transmute_vec_as_bytes
https://github.com/FyroxEngine/Fyrox/blob/3b03ea717d201be24dac98cc9e8ebf812808e898/fyrox-core/src/lib.rs#L325-L331 When casting type to
u8
slice, we also need to make sure whether the type won't contain padding bytes. Otherwise, it could lead to uninitialized memory access or break the reliability of program. Based on the usages of the function, we consider the generic typeT
should also implementPod
trait.PoC
In the program above, we passed the
struct
that might contain padding bytes as the generic typeT
. First, when we run with miri, we can get the following results:In the library, we found that this function uses
Vec
as input as internal usages, which will not trigger the bug. However, since this function is provided as public safe API, we should consider the constraints on the generic typeT
. (It is required also because the function name istransmute_"vec"_as_bytes
:)The consequence of UB
We could find that the results of
fv
break the reliability of program under different environment. Compiled withx86_64
, the results would be:While compiled with
x86
, the results would be:Take the following usages for example, https://github.com/FyroxEngine/Fyrox/blob/25a229690ccb38ebd90bc9e6f81d4bc90b4e752e/fyrox-impl/src/scene/terrain/mod.rs#L101-L112 The
Texture
built on thisbytes
could have different results...