Skip to content
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

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

Open
Nashenas88 opened this issue Oct 16, 2023 · 3 comments
Open

Comments

@Nashenas88
Copy link

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Member

Dirbaio commented Oct 16, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants