Closed dtolnay closed 3 years ago
In other words the UB repros in #17 and #18 both continue to be an issue.
@dtolnay
What do you mean it's safe to implement? Does Rust allows to implement a trait method that is marked unsafe
without unsafe
?
Also if you have suggestion how to the handle it better, I'd prefer to do this. The issue is we want to implement Borsh serialization and deserialization of Vec<u8>
and &[u8]
differently from default implementation of Vec<T>
and &[T]
where T: BorshSerialize
or T: BorshDeserialize
BorshSerialize and BorshDeserialize are safe traits -- by language semantics that means it is unsound for the memory safety / thread safety of the program to hinge on either of them being implemented "correctly". This is 100% orthogonal to whether an individual trait method is safe or unsafe to call (which is what unsafe fn means). A trait that is unsafe to implement can contain methods that are safe to call, and a trait that is safe to implement can contain methods that are unsafe to call.
Reference:
https://doc.rust-lang.org/1.50.0/reference/items/traits.html#unsafe-traits https://doc.rust-lang.org/1.50.0/nomicon/safe-unsafe-meaning.html https://doc.rust-lang.org/1.50.0/book/ch19-01-unsafe-rust.html https://doc.rust-lang.org/1.50.0/std/keyword.unsafe.html
Yeah, that's all true. I believe there's no clean way to express this in Rust (what we ideally want is an unsafe super-trait, but that requires specialization, bringing us back to the square 1).
https://github.com/near/borsh-rs/pull/25 implements a simpler solution of just hiding the method from crate's public API.
I believe there's no clean way to express this in Rust
unsafe trait
sounds like a clean way to express "the user must be careful when implementing this trait (e.g., by not overwriting a certain method)".
One pattern that can be used in order to have a safe trait rely on guarantees is the following:
struct Guarantee { _private: () }
impl Guarantee {
// Safety: only call if ...
pub unsafe fn promise() -> Self { Guarantee { _private: () } }
}
trait RequiresGuarantee {
fn check_guarantee(&self) -> Guarantee;
}
Then users can call check_guarantee
, which, if it returns without diverging, must have internally called promise
using an unsafe
block, so the requirements on promise
must be satisfied.
To be clear, the underlying issue was solved way nicer by dtolnay here by just not using is_u8
at all.
@RalfJung there's a twist here, in that we want to (unsafely) override this for u8, and, for all other types, use default (safe) impl. Or, as I've said, we could use unsafe trait
, but that needs specialization.
@cramertj that won't work exactly as spelled -- the user user can get Guarantee
without unsafe by calling RequiresGuarantee::check_guarantee
on some other type that implements it. You need to make check_guarantee
unsafe to call as well:
// You write code like
struct CramertjType;
impl RequiresGuarantee for CramertjType {
fn check_guarantee(&self) -> Guarantee {
// Carefully verify invariants
unsafe { Guarantee::promise() }
}
}
// I write code like
struct MatkladType;
impl RequiresGuarantee for MatkladType {
fn check_guarantee(&self) -> Guarantee {
// Life's too short for verifying invariants, lets use someone else's promise
CramertjType::check_guarantee()
}
}
You're right, good point!
On Wed, Apr 21, 2021, 7:50 AM Aleksey Kladov @.***> wrote:
To be clear, the underlying issue was solved way nicer by dtolnay here by just not using is_u8 at all.
@RalfJung https://github.com/RalfJung there's a twist here, in that we want to (unsafely) override this for u8, and, for all other types, use default (safe) impl. Or, as I've said, we could use unsafe trait, but that needs specialization.
@cramertj https://github.com/cramertj that won't work exactly as spelled -- the user user can get Guarantee without unsafe by calling RequiresGuarantee::check_guarantee on some other type that implements it. You need to make check_guarantee unsafe to call as well:
// You write code likestruct CramertjType; impl RequiresGuarantee for CramertjType { fn check_guarantee(&self) -> Guarantee { // Carefully verify invariants unsafe { Guarantee::promise() } } } // I write code likestruct MatkladType; impl RequiresGuarantee for MatkladType { fn check_guarantee(&self) -> Guarantee { // Life's too short for verifying invariants, lets use someone else's promise CramertjType::check_guarantee() } }
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/near/borsh-rs/issues/24#issuecomment-824123254, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABNP2KPFXKTZC7T7VAK7F2TTJ3Q3XANCNFSM4ZNIANIQ .
there's a twist here, in that we want to (unsafely) override this for u8, and, for all other types, use default (safe) impl. Or, as I've said, we could use unsafe trait, but that needs specialization.
You could use unsafe trait
without specialization, but I guess that's not as ergonomic as you'd like it to be?
BorshSerialize and BorshDeserialize have an
is_u8
trait method which is unsafe to call (but safe to implement). What contract is the caller required to uphold when callingis_u8
in order for the call to be sound?