Avoid as much trap as possible in the scheduler

ia0 commented 4 months ago

Search for all functions returning Result<_, Trap> and fix them if needed.

Good candidates are:

MemoryApi::alloc() should return World:NotEnough (or something similar).
Applet::{enable,disable}() should probably return User::InvalidState.
Scheduler::disable_event() is similar to above.
or_trap!() should be replaced with or_fail!().

Bad candidates are:

Wrong memory accesses (e.g. MemoryApi::{get,get_mut}()) should continue to Trap.

The distinction is not always clear. It would be good to come up with a criteria of whether an operation should trap or error.

This is part of #43.

lukeyeh commented 4 weeks ago

Here's a list of a search for Result<.*, Trap> I'll go through them checking if we should return errors instead

./crates/scheduler/src/call/timer.rs
105:) -> Result<Id<board::Timer<B>>, Trap> {

This should return Result<_, Error> with Error::user(Code::OutOfBounds) for invalid timer IDs. This should just propagate the error from Id::new. We could also add a Code::ResourceExhausted error for scheduler.timers[timer].is_none().

./crates/scheduler/src/call/crypto/ccm.rs
88:fn ensure_support<B: Board>() -> Result<(), Trap> {

./crates/scheduler/src/call/crypto/gcm.rs
102:fn ensure_support<B: Board>() -> Result<(), Trap> {

This should also just return Result<_, Error> with Error::user(Code::OutOfBounds)

./crates/scheduler/src/lib.rs
419:    fn disable_event(&mut self, key: Key<B>) -> Result<(), Trap> {

./crates/scheduler/src/call/usb/serial.rs
103:fn convert_event(event: u32) -> Result<Event, Trap> {

./crates/scheduler/src/call/radio/ble.rs
82:fn convert_event(event: u32) -> Result<Event, Trap> {

./crates/scheduler/src/call/uart.rs
140:fn convert_event<B: Board>(uart: Id<board::Uart<B>>, event: u32) -> Result<Event<B>, Trap> {

These should return Result<_, Error> with Error::user(Code::InvalidArgument). This would only happen if the user supplied a bad event from the api

./crates/scheduler/src/applet.rs
166:    pub fn enable(&mut self, handler: Handler<B>) -> Result<(), Trap> {
176:    pub fn disable(&mut self, key: Key<B>) -> Result<(), Trap> {

These should return Error::user(Code::InvalidState). User shouldn't be inserting more than one event handler for a given key.

./crates/scheduler/src/applet.rs
106:    pub fn insert(&mut self, hash: HashContext<B>) -> Result<usize, Trap> {
112:    pub fn get_mut(&mut self, id: usize) -> Result<&mut HashContext<B>, Trap> {
116:    pub fn take(&mut self, id: usize) -> Result<HashContext<B>, Trap> {

Looking at references to AppletHashes::{insert, get_mut, take} it looks like user supplies these ids, so i'd say Error::user(Code::OutOfBounds) would be a suitable error code to return instead of trapping.

./crates/scheduler/src/applet/store.rs
36:    fn get(&self, ptr: u32, len: u32) -> Result<&[u8], Trap>;
37:    fn get_mut(&self, ptr: u32, len: u32) -> Result<&mut [u8], Trap>;
37:    fn alloc(&mut self, size: u32, align: u32) -> Result<u32, Error>;

These should remain trapping, though the user supplies what appears to be the key, the result is dependent on the board implementation.

40:    fn get_opt(&self, ptr: u32, len: u32) -> Result<Option<&[u8]>, Trap> {
47:    fn get_array<const LEN: usize>(&self, ptr: u32) -> Result<&[u8; LEN], Trap> {
51:    fn get_array_mut<const LEN: usize>(&self, ptr: u32) -> Result<&mut [u8; LEN], Trap> {
56:    fn from_bytes<T: AnyBitPattern>(&self, ptr: u32) -> Result<&T, Trap> {
61:    fn from_bytes_mut<T: NoUninit + AnyBitPattern>(&self, ptr: u32) -> Result<&mut T, Trap> {
65:    fn alloc_copy(&mut self, ptr_ptr: u32, len_ptr: Option<u32>, data: &[u8]) -> Result<(), Trap> {

Think these should also remain trapping being transitively from the above.

./crates/scheduler/src/call/crypto/hash.rs
247:fn convert_hash_algorithm<B: Board>(algorithm: u32) -> Result<Result<Algorithm, Trap>, Trap> {
262:fn convert_hmac_algorithm<B: Board>(algorithm: u32) -> Result<Result<Algorithm, Trap>, Trap> {

These should return Result<_, Error> with Error::user(Code::InvalidArgument). This would only happen if the user supplied a bad algorithm from the api

./crates/scheduler/src/applet/store/wasm.rs
88:    fn borrow(&self, range: Range<usize>) -> Result<(), Trap> {
92:    fn borrow_mut(&self, range: Range<usize>) -> Result<(), Trap> {
97:    fn borrow_impl(&self, y: (bool, Range<usize>)) -> Result<(), Trap> {

These I'm not sure.

./crates/scheduler/src/call/crypto/ec.rs
220:fn convert_curve<B: Board>(curve: u32) -> Result<Result<Curve, Trap>, Trap> {

These should return Result<_, Error> with Error::user(Code::InvalidArgument). This would only happen if the user supplied a curve from the api

ia0 commented 2 weeks ago

Thanks for putting up this list (though you could have made a PR directly)! I'll go through them using the <file>:<line> you provided. Note that searching for Result<_, Trap> is not enough. For example in call/timer.rs, there are many map_err(|_| Trap) that could be errors instead.

crates/scheduler/src/call/timer.rs:105: I agree we should return an error. Once we have multiple applets, we shouldn't use a 1-to-1 mapping between the board ids and the applet ids, and instead have a level of indirection like file descriptors in linux. This would mean that the error should probably be InvalidArgument.

crates/scheduler/src/call/crypto/ccm.rs:88 and gcm.rs:102: I agree this should return an error, namely User:NotImplemented. Note that contrary to or_fail, the blame is on the user not the world, since there's a function to check support.

crates/scheduler/src/lib.rs:419: I'm splitting this because anything outside src/call is not necessarily an applet error. I agree that disabling a non-existing key should be an error, namely User:NotFound. I'm not sure how to deal with such top-level functions that return Result<_, Error> where the error is already at the applet level. Maybe the best is to return Result<_, Failure> to indicate that it a user-level error.

crates/scheduler/src/call/usb/serial.rs:103 and friends: Yes, we should return an error, namely User:InvalidArgument.

crates/scheduler/src/applet.rs:166: That's similar to the Applet::disable above. We should return Failure with User:InvalidState or User:InvalidArgument. Ideally we should add an AlreadyExist error code.

crates/scheduler/src/applet.rs:106 and friends: For insert the error should be World:NotEnough. For get_mut and take, it should be User:OutOfBounds for the first Trap and User:NotFound (or InvalidArgument) for the second.

crates/scheduler/src/applet/store.rs:36 and friends: Yes, let's keep bad memory accesses as traps.

crates/scheduler/src/call/crypto/hash.rs:247 and friends: The first trap (i.e. outer one) should be User:InvalidArgument. The second User:NotImplemented (i.e. inner one).

crates/scheduler/src/applet/store/wasm.rs:88 and friends: This should stay a trap. This is also bad memory management from the applet (for example trying to break mutability xor aliasing).

crates/scheduler/src/call/crypto/ec.rs:220: Yes, this is similar to the call/crypto/hash.rs:247 case. Outer trap should be User:InvalidArgument. Inner trap should be User:NotImplemented.

google / wasefire

Avoid as much trap as possible in the scheduler #464