Impl of FixedGattValue for &str seems wrong

[jscatena88]

I was attempting to send a string as a characteristic with the following code:

#[gatt_service(uuid = DEVICE_INFO_SERVICE_UUID)]
struct DeviceInfoService {
    #[characteristic(uuid = "00002a25-0000-1000-8000-00805f9b34fb", value = "1337", read)]
    serial_number: &'static str,
}

I could see the characteristic but it had an odd nonsense value that was not valid utf8

Looking at the code for the impl<T: Primitive> FixedGattValue for T it seems that SIZE is set as mem::size_of::<Self>(); and in the case of T = &str this wont actually be the size of the str slice, just the size of the reference.

Having realized this I switched to trying to use heapless::String (as shown below) and that worked as expected.

#[gatt_service(uuid = DEVICE_INFO_SERVICE_UUID)]
struct DeviceInfoService {
    #[characteristic(uuid = "00002a25-0000-1000-8000-00805f9b34fb", value = heapless::String::from_str("1337").unwrap(), read)]
    serial_number: heapless::String<8>,
}

[jscatena88]

For completeness here is the current code that seems to be flawed for &str

trait Primitive: Copy {}
impl Primitive for u8 {}
impl Primitive for u16 {}
impl Primitive for u32 {}
impl Primitive for u64 {}
impl Primitive for i8 {}
impl Primitive for i16 {}
impl Primitive for i32 {}
impl Primitive for i64 {}
impl Primitive for f32 {}
impl Primitive for f64 {}
impl Primitive for BluetoothUuid16 {} // ok as this is just a NewType(u16)
impl Primitive for &'_ str {}

impl<T: Primitive> FixedGattValue for T {
    const SIZE: usize = mem::size_of::<Self>();

    fn from_gatt(data: &[u8]) -> Result<Self, FromGattError> {
        if data.len() != Self::SIZE {
            Err(FromGattError::InvalidLength)
        } else {
            // SAFETY
            // - Pointer is considered "valid" as per the rules outlined for validity in std::ptr v1.82.0
            // - Pointer was generated from a slice of bytes matching the size of the type implementing Primitive, and all types implementing Primitive are valid for all possible configurations of bits
            // - Primitive trait is constrained to require Copy
            unsafe { Ok((data.as_ptr() as *const Self).read_unaligned()) }
        }
    }

    fn to_gatt(&self) -> &[u8] {
        // SAFETY
        // - Slice is of type u8 so data is guaranteed valid for reads of any length
        // - Data and len are tied to the address and size of the type
        unsafe { slice::from_raw_parts(self as *const Self as *const u8, Self::SIZE) }
    }
}

[petekubiak]

I woke up in the middle of the night thinking about this the other night because I noticed this while attempting to implement GattValue on &[u8] haha! Unfortunately I then forgot to do anything about it.

I think you're right that it's essentially returning the string slice's memory structure, rather than the data it's referencing.

[jscatena88]

It seems to me that the fix should be pretty simple. I'm guessing it wouldn't be a a FixedGattValue anymore since it is an &str which could be any size. But either way to_gatt would just be calling the as_bytes() function and from_gatt should just be str::from_utf8

[petekubiak]

Yeah I thought it would be a simple fix too, unfortunately I was wrong! I'm going to attempt to describe the journey I've been on so far just in case anything jumps out at anyone as a potential solution.

The reason that I can't just call as_bytes and from_utf8 and have done is that the compiler complains that the lifetimes of the input and output references don't match, e.g. in from_gatt the lifetime of data isn't linked to the lifetime of Self (which in this case is &str).

With the definition of GattValue as is, you can't just specify a lifetime to link the two together because that apparently violates the GattValue definition.

pub trait GattValue: Sized {
    // ...

    fn from_gatt(data: &[u8]) -> Result<Self, FromGattError>;
}

impl<'a> GattValue for &'a str {
    // ,,,

    fn from_gatt(data: &'a [u8]) -> Result<Self, FromGattError> {  // Error
    }

So I experimented with adding a lifetime to GattValue:

pub trait GattValue<'a>: Sized {
    // ...

    fn from_gatt(data: &'a [u8]) -> Result<Self, FromGattError>;

    fn to_gatt(&'a self) -> &'a [u8];
}

It was reasonably easy to refactor the various implementations to accommodate this, but it has massive implications where these types are used. For example, Characteristics need to hold onto a phantom lifetime, and this blows up into borrowing conflicts when iterating over the attribute table. I'm not saying it's insurmountable, but it's a cascading issue which I think will take no small effort to fully fix and will result in a fair amount of extra complexity.

I do have one alternative: the nuclear option of mem::transmute to effectively transmute &'1 T into &'2 T. I am twitchy about this though as it essentially allows us to completely circumvent lifetime checking. I don't think this is an issue within the TrouBLE library itself, as the attribute table is stored in static memory and so has 'static lifetime. For a user however this does present a possible footgun, albeit an obscure one.

As such, I think there are two options:

1. Implement GattValue for &str (and potentially &[u8] too for Allow getting and setting all attributes #261) with mem::transmute, but make the trait itself private to the TrouBLE repo so that the user can't accidentally blow themselves up by calling from_gatt on a reference with an insufficient lifetime.
2. Leave GattValue as public so the user can use it to implement GattValue on their own types and just don't implement GattValue for &str.

Please let me know what you think to these options - @lulf and @jamessizeland I'd be very keen to get your steer on which way we should go here too.

That being said, if anyone has any other solutions I might have missed, please let me know!

[lulf]

Yeah I thought it would be a simple fix too, unfortunately I was wrong! I'm going to attempt to describe the journey I've been on so far just in case anything jumps out at anyone as a potential solution.

The reason that I can't just call as_bytes and from_utf8 and have done is that the compiler complains that the lifetimes of the input and output references don't match, e.g. in from_gatt the lifetime of data isn't linked to the lifetime of Self (which in this case is &str).

With the definition of GattValue as is, you can't just specify a lifetime to link the two together because that apparently violates the GattValue definition.

pub trait GattValue: Sized {
// ...

fn from_gatt(data: &[u8]) -> Result<Self, FromGattError>;

}

impl<'a> GattValue for &'a str {
// ,,,

fn from_gatt(data: &'a [u8]) -> Result<Self, FromGattError> {  // Error
}

So I experimented with adding a lifetime to GattValue:

pub trait GattValue<'a>: Sized {
// ...

fn from_gatt(data: &'a [u8]) -> Result<Self, FromGattError>;

fn to_gatt(&'a self) -> &'a [u8];

}

It was reasonably easy to refactor the various implementations to accommodate this, but it has massive implications where these types are used. For example, Characteristics need to hold onto a phantom lifetime, and this blows up into borrowing conflicts when iterating over the attribute table. I'm not saying it's insurmountable, but it's a cascading issue which I think will take no small effort to fully fix and will result in a fair amount of extra complexity.

I do have one alternative: the nuclear option of mem::transmute to effectively transmute &'1 T into &'2 T. I am twitchy about this though as it essentially allows us to completely circumvent lifetime checking. I don't think this is an issue within the TrouBLE library itself, as the attribute table is stored in static memory and so has 'static lifetime. For a user however this does present a possible footgun, albeit an obscure one.

As such, I think there are two options:

1. Implement GattValue for &str (and potentially &[u8] too for [Allow getting and setting all attributes #261](https://github.com/embassy-rs/trouble/pull/261)) with `mem::transmute`, but make the trait itself private to the TrouBLE repo so that the user can't accidentally blow themselves up by calling `from_gatt` on a reference with an insufficient lifetime.

2. Leave GattValue as public so the user can use it to implement GattValue on their own types and just don't implement GattValue for &str.

I'm wondering if most use cases for being able to use slices is for static data, so would it work if you only implemented this for &'static str and &'static [u8]?

[jscatena88]

I wonder if to_gatt and from_gatt should be in separate traits. As you mention @lulf it seems like the most common use case for &str would be 'static , but then you would probably only want to offer to_gatt. I don't believe you would be able to easily implement from_gatt

[petekubiak]

I'm wondering if most use cases for being able to use slices is for static data, so would it work if you only implemented this for &'static str and &'static [u8]?

Yeah that's a good idea, and means we don't lose any functionality.

I wonder if to_gatt and from_gatt should be in separate traits. As you mention @lulf it seems like the most common use case for &str would be 'static , but then you would probably only want to offer to_gatt. I don't believe you would be able to easily implement from_gatt

This sounds sensible, I think the only alternative would be to use unimplemented!() in the from_gatt method for those statics, but I'm not keen on the idea of run-time errors for compile-time issues.