embassy-rs / nrf-softdevice

Apache License 2.0
256 stars 76 forks source link

`FixedGattValue` improperly implemented for primitives that are not u8/i8 #196

Open Nashenas88 opened 9 months ago

Nashenas88 commented 9 months ago

ble::gatt_traits::FixedGattValue is written with the assumption that &[u8] can be safely dereferenced from *const T where T is a primitive type (T: ble::gatt_traits::Primitive). It fails for any T: Primitive that is not u8/i8 because only u8 and i8 have an alignment of 1. I ran into this on ARM where it triggers a trap (see https://medium.com/@iLevex/the-curious-case-of-unaligned-access-on-arm-5dd0ebe24965 for a more thorough dissection of the different and unpredictable behavior that can be seen on ARM).

The current code:

unsafe { *(data.as_ptr() as *const Self) }

should be updated to:

// Safety: data should be valid for reads and contain properly
// initialized values of T.
unsafe { core::ptr::read_unaligned(data.as_ptr() as *const Self) }
Nashenas88 commented 9 months ago

I would create a PR, but it's not clear to me what would be preferred since it's no longer possible to use the existing T: Primitive impl (unless you want to pay the cost of emitting an unaligned read even for u8/i8).

alexmoon commented 9 months ago

I think the cost of an unaligned read of a single u8/i8 should be relatively insignificant. That seems like the most straightforward solution to me.

Dirbaio commented 9 months ago

u8/i8 reads are always aligned, they have an alignment of 1 :D