jonas-schievink / rubble

(going to be a) BLE stack for embedded Rust
BSD Zero Clause License
399 stars 56 forks source link

Trimming required dependencies to avoid unsafe code #122

Open daboross opened 4 years ago

daboross commented 4 years ago

Hi!

I'm looking at seeing if I can use rubble to provide initial BLE support for TockOS. I'm not affiliated with TockOS, I just want to use BLE support in Tock. So far, I've just been writing code locally and talking a bit with the Tock developers.

My question is: how open would you be to removing deps or making them optional, so that the main rubble crate with no-default-features is unsafe-free?

Specifically, I'd be interested in writing the following changes:

These would all be in the attempt to remove unsafe code from the features necessary for insecure connections.

The TockOS project strongly favors minimal dependency trees, and #![forbid(unsafe)] crates, so that's what I'd be aiming for. I realize that this might not be inline with rubbles goals - if it isn't, I think I can still make this work - it would just involve vendoring rubble into tock rather than depending on it. But that would mean not contributing my work back here, and I'd like to avoid that if possible.

I might end up needing to vendor rubble anyways, since I'll eventually need a non-blocking BLE interface - I don't know how possible this is without big changes, or without removing the blocking interface. However, if you'd be open to some or all of these changes, I'd much rather make PRs & comply with reviews/etc. than keep the changes local. Thanks!

jonas-schievink commented 4 years ago

Hey, thanks for your interest in Rubble! I think some things are basically uncontroversial and can be done regardless of any Tock integration even:

Note that I'm playing around with the idea of using zerocopy for (de)serialization of packets (#53), which uses unsafe code internally. Does Tock have some "approved" solution for safely transmuting data?

Regarding making more things optional, that might be a slight issue – I try to avoid this whenever I can, as it results in combinatorial explosion of feature flags that all should be tested in CI. However, depending on the extent, I am definitely willing to compromise here:

make all security features + dependencies (p256, ring, and rand_core) optional

The only unsafe code pulled in by p256 is the compiler black box in subtle – this is trivially safe code that has been well-reviewed by several people. Is that really still a problem for Tock?

(ring is already optional by the way, it does not support no_std platforms properly and is only used as a known-good P-256 implementation to compare against)

make SimpleQueue + heapless optional

Does Tock not use heapless anywhere? Where does it get its data structure implementations from?


Regarding the non-blocking interface, I think I'd be happy to have that in Rubble – the radio interface is due for a redesign anyways (#48), and if we can make it fully non-blocking in the same go that'd be great. If it turns out to be too hard to use we can always offer a simpler, blocking interface on top of it. Unless you mean a non-blocking interface to the high-level parts of the stack, which Rubble already kind of has – you'll get an error if a packet doesn't fit in the packet queue, it will not block until there is space.

daboross commented 4 years ago

Thank you for the quick response!

I'm glad to see some of these changes could make sense.

Removing unsafe code from the uuid crate, or remove the dependency on it entirely (I'd like to avoid making it optional, and cannot really see how that would work as the UUID types are pretty fundamental to BLE).

The optional thing I was thinking would be to define our own Uuid struct which would be replaced with uuid::Uuid if the feature was enabled. I guess that might not be purely additive, though, and wouldn't bring a ton of benefit vs. just removing it or fixing it.

Removing unsafe code from byteorder, or, if necessary, using libstd methods, or custom ones, in favor of byteorder.

:+1: I think our usages of byteorder should be really easy to replace with libstd, so I'll submit a PR for that first.

Regarding making more things optional, that might be a slight issue – I try to avoid this whenever I can, as it results in combinatorial explosion of feature flags that all should be tested in CI.

This makes sense! I wasn't thinking of the exponential blowup from optional features, and most of these probably don't make sense given that.

The only unsafe code pulled in by p256 is the compiler black box in subtle – this is trivially safe code that has been well-reviewed by several people. Is that really still a problem for Tock?

I'll have to check this- it might end up being fine.

Does Tock not use heapless anywhere? Where does it get its data structure implementations from?

As far as I can tell right now, tock builds all of their data structures from scratch - for instance, they roll their own ring buffer. I could definitely look into this more, it might just be the design ends up not using many of the things heapless provides.

I'd suggested making it optional since it doesn't seem necessary for the core bluetooth stack, and I think the recursive dependencies from a medium-side crate like heapless are something tock wants to avoid, even regardless of unsafe code in those deps.


I'm glad to see that a non-blocking interface is on the radar! I don't fully understand all of the limitations or things we want in a better hardware/radio interface, but I'd love to contribute to that. I've been reading through the O'Rielly BLE book and the rubble codebase, and maybe I'll be able to understand the situation better (or at least ask better questions) when I've read through more.

Thanks for making this library, and for being here! I'll probably be back with more questions and/or more information on Tock's constraints soon.

jonas-schievink commented 4 years ago

I've started to work on removing uuid, but got sidetracked. I'll hopefully finish it this week.

daboross commented 4 years ago

Awesome!

I've not done much on this since making the byteorder pull request. I'm hoping to work on the random number interface, but I likely won't be actually working on it until after I get more direct tock integration work done.

I haven't asked more about any of the security crates yet, but I'm also hoping that we can work that out.