Fix lifetimes and mutability of get_buf and get_buf_mut

[adrianparvino]

Lifetimes should have been '_ and not static.
get_buf_mut created a mutable reference from an immutable reference.

SAFETY annotation has also been added.

[jannic]

This sounds reasonable, but I don't know enough about this code to evaluate the implications. @ithinuel, what do you think?

[nilclass]

I'm curious, why did you remove get_buf_parts and duplicate it's code instead? Does it have any benefits?

[ithinuel]

I'd rather avoid code duplication and keep the unsafe fn get_buf_parts as this is orthogonal to the issue mentionned about lifetimes. I'm not against using a match statement vs an if statement. In this situation I don't think it will have a significant impact on the generated code.

Lifetimes should have been '_ and not static.

Based on my reading of lifetime elision, '_ is expands to fn get_buf_mut<'self>(&'self mut self) -> &'self mut [u8].
The reference targets somewhere in the DPRAM block which is a hardware element that lives for 'static.
So arguably, reusing the lifetime of 'self isn't more correct than using 'static.
A benefit of this would be that it'd prevent one from keeping that reference and continue using it after the Endpoint has be destroyed but it is not possible to do such a thing at present anyway.

get_buf_mut created a mutable reference from an immutable reference.

It is not. It is built from a *mut u8 pointer to a dedicated ram region and a length.
As far as I know, *const T as *mut T is UB but not *mut T as *const T.
If the concern comes from the use of offset, it is implemented for both *const T and *mut T (the latter being our usecase in get_buf_parts) so there's no hidden conversion there.

[jannic]

With the old signature, fn get_buf_mut(&self) -> &'static mut [u8], one could call the function multiple times and acquire multiple &mut references to the same memory location at the same time. That's instant UB as far as I know.

[ithinuel]

Fair enough :)
Ohh, that's the non-mut reference OP's refering to!

[adrianparvino]

Thank you for the replies.

I'm curious, why did you remove get_buf_parts and duplicate it's code instead? Does it have any benefits?

I'm currently unsure about the safety rules regarding temporarily mutable references and pointers, and unsafe fn get_buf_parts(&self) -> (*mut u8, usize) makes a *mut u8 from an &self, hence why I removed it. In hindsight, as previously mentioned, this is probably okay, as the "provenance" is from DPRAM_BASE and not self.

The reference targets somewhere in the DPRAM block which is a hardware element that lives for 'static.

In my perspective, ep_allocate acts just like a normal allocator. It takes a 'static memory(the heap in a hosted environment, USB_DPRAM in this example) and lends it to a value(Endpoint). That is, the Endpoint logically owns that region of memory until it gets dropped, hence why I consider their lifetimes intrinsically linked.

More pragmatically:

With the old signature, fn get_buf_mut(&self) -> &'static mut [u8], one could call the function multiple times and acquire multiple &mut references to the same memory location at the same time. That's instant UB as far as I know.

changing the type to be fn get_buf_mut(&mut self) -> &'static mut [u8] is not enough to establish the logical ownership, and it's still possible to call get_buf_mut twice. fn get_buf_mut(&mut self) -> &mut [u8] is the required signature to actually "lock" the memory.

[ithinuel]

Indeed. If you don't mind keeping the get_buf_parts function (to avoid code duplication) and simply change the get_buf and get_buf_mut signatures, that should be enough to fix the issue and I'd gladly approve the PR :)

[jannic]

Just as a side note: *mut don't have the same rules as &mut, you can have multiple of them as long as you don't deference them. That's one of the reasons pointer dereferencing is unsafe.