search

trussed-dev / trussed

Modern Cryptographic Firmware
https://trussed.dev
Apache License 2.0
413 stars 26 forks source link

Dangerous `unsafe` in `impl_reply` #84

Closed sosthene-nitrokey closed 1 year ago

sosthene-nitrokey commented 1 year ago

https://github.com/trussed-dev/trussed/blob/main/src/api/macros.rs#L96

    impl From<Reply> for $reply {
        fn from(reply: Reply) -> reply::$reply {
            match reply {
                Reply::$reply(reply) => reply,
                _ => { unsafe { unreachable_unchecked() } }
            }
        }
    }

This unsafe is not sound and this impl is part of the public API. I think we could replace it with a panic!, since it is only used by PollClient::request. To reduce the risk of panicking I would instead use TryFrom.

robin-nitrokey commented 1 year ago

Yeah, this should really be a TryFrom. If we make changes to the request and reply types, it could make sense to have a trait that defines the request and reply types for a syscall.

robin-nitrokey commented 1 year ago

I’ll prepare a PR. Will also be useful for API extensions too because I’m using TryFrom there, so I had to duplicate FutureResult. Edit: Nevermind, I still have to do the serialization and deserialization.

sosthene-nitrokey commented 1 year ago

I'm making a PR that adds a trait that does bindings between the request and reply types.