Skip to content
This repository has been archived by the owner on May 18, 2022. It is now read-only.

Trimming required dependencies to avoid unsafe code #122

Open
daboross opened this issue Jul 8, 2020 · 4 comments
Open

Trimming required dependencies to avoid unsafe code #122

daboross opened this issue Jul 8, 2020 · 4 comments
Labels
type: enhancement A new feature or an improvement for an existing one

Comments

@daboross
Copy link
Contributor

daboross commented Jul 8, 2020

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:

  • replace byteorder in favor of std methods
  • make SimpleQueue + heapless optional
  • make all security features + dependencies (p256, ring, and rand_core) optional
  • make uuid dependency optional

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

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

  • 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).
  • Removing rand_core in favor of defining our own traits for PRNGs and CSPRNGs (also see Obtaining random numbers #2) – nowadays the rand family of crates strikes me as not terribly reliable, and we mostly just need some traits for random numbers anyways.
  • Removing unsafe code from byteorder, or, if necessary, using libstd methods, or custom ones, in favor of byteorder.

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.

@jonas-schievink jonas-schievink added the type: enhancement A new feature or an improvement for an existing one label Jul 8, 2020
@daboross
Copy link
Contributor Author

daboross commented Jul 10, 2020

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.

👍 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
Copy link
Owner

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

@daboross
Copy link
Contributor Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement A new feature or an improvement for an existing one
Projects
None yet
Development

No branches or pull requests

2 participants