Stack should be an unsafe trait

[Amanieu]

The Stack trait allows returning arbitrary raw pointers which can be used to write to arbitrary addresses using only safe code.

[whitequark]

I don't understand. You can create as many arbitrary raw pointers as you want using just safe code. E.g.. https://is.gd/zhyu8t. It's dereferencing them that is unsafe.

[Amanieu]

The issue is that Generator::new (which is not unsafe) assumes that these pointers are valid and that the stack base is properly aligned for the current architecture.

Now that I look at it again, this isn't really an issue since Generator::new requires GuardedStack, which is an unsafe trait, while Generator::unsafe_new only requires Stack but is unsafe. However if we do go down that route then we should properly document that GuardedStack should ensure the returned pointers are valid & properly aligned, and that Generator::unsafe_new requires this as well.

[Victor-Savu]

I strongly disagrre with the resolution of this issue. Here is my reasoning:

The whole point of unsafe declarations of traits or functions is to prompt the user to read the contract that needs to be fulfilled in order to implement or respectively call them. When the users types unsafe they mean "I have read the contract and have checked that the implementation / call abides by it". The contract must be written in the documentation accompanying the declaration of the trait or function. The reason for there even be a need for a contract written in natural language is that the Rust type system cannot statically express all the constraints needed by all memory safe (incl. data race free) algorithms.

The resolution of this issue proposes to not use the unsafe keyword to mark the existence of a contract on the Stack trait, but rather to pollute the documentation by putting warnings about the contract everywhere something implementing the Stack trait appears. This essentially sabotages the unsafe mechanism.

My suggestion is: Pretty please, with sugar lumps on top, reconsider this decision! I don't want to feel unsafe when using this library. I want to read unsafe and go to the one obvious place in the docs telling me what I should do to stay safe :)

[whitequark]

When the users types unsafe they mean "I have read the contract and have checked that the implementation / call abides by it". The contract must be written in the documentation accompanying the declaration of the trait or function.

You cannot use a Stack (unlike a GuardedStack, which is unsafe to implement) without going through Generator::unsafe_new. The only difference is where exactly the contract is stated; the unsafe mechanism is working as intended.

[Victor-Savu]

What I am saying is that the responsibility for satisfying a contract should be advertised using unsafe. I am not saying the current library does something that is not sound, but this architecture choice leaves a very dangerous vulnerability in the codebase which may turn really dangerous a few months/years/features/refactorings down the line. This is the whole philosophy distinguishing Rust from C++: the ability to reason locally about the validity of the code.

I am sorry for coming across as such a desperate being, but I have to confess that I have been looking forward to someone implementing this crate for more than a year now. I am so clueless when it comes to assembly that I could not even attempt. Now that the crate exists, I just feel like it should become the most awesome crate in the rust ecosystem and this is the reason why I would like to make sure that it meet the highest standards of quality. I feel like this discussion could benefit from people who are specialists in the unsafe to help make the interface is rock solid. Maybe @RalfJung or @gankro if any of them would be interested?

[whitequark]

FYI we've already added that unsafe (well, it's not merged quite yet) simply because so many people bring this point up. I still think it's pointless but whatever.

[Amanieu]

This will be fixed by #54

[RalfJung]

I feel like this discussion could benefit from people who are specialists in the unsafe to help make the interface is rock solid. Maybe @RalfJung or @gankro if any of them would be interested?

Note that I have very little idea about the inner workings of this library. But here are my feelings on when a (public) trait should be unsafe: It should be unsafe if the consumers of the trait are expecting more than just well-typedness of the functions implementing the trait. For example, I could write this implementation of Stack

struct T;
impl Stack for T {
  fn base(&self) -> *mut u8 { 42 as *mut u8 }
  fn limit(&self) -> *mut u8 { 42 as *mut u8 }
}

This is a perfectly fine implementations of the interface, it does not violate any of Rust's safety assumptions. The fact that it makes little sense is independent of that. this is comparable to implementing the Hash trait such that it returns the current time converted to hex: It makes little sense, but it matches the types given by the trait and that's all that counts (for a safe public trait, that is).

If it is the case that I can now subsequently call some safe functions with T as above, and cause havoc, then the trait should be unsafe. The reason for this is quite simple: It should not be possible for a safe client (i.e., a client not using the unsafe keyword) to cause crashes using this library. (This is the very definition of a "safe library".)

I hope this helps.

[edef1c]

@RalfJung

The reason for this is quite simple: It should not be possible for a safe client (i.e., a client not using the unsafe keyword) to cause crashes using this library.

As discussed above, that isn't possible: you either need to unsafe impl GuardedStack, or call Generator::unsafe_new (which is obviously unsafe).
One day, I'll take a job at GitHub just to add a feature that enforces reading previous discussion..

[RalfJung]

I was scrolling over the previous discussion, but there's lots of details of the API that I do not understand, which probably led to me not noticing these comments. Sorry for that.

The Stack documentation describes a contract that has to be fulfilled by implementations of this trait, and that contract goes beyond the mere types, i.e., my dummy implementation above violates the contract. Effectively, by making Stack a safe trait, what happens is that the obligation of satisfying this contract is actually entirely moved to implementations of GuardedStack and callers of unsafe_new; in particular, if someone used my dummy implementation above with either of these, then that user would be to blame, not my implementation.
That sounds like a valid position to take. It's not the choice I would make, but of course that doesn't matter.

I guess the situation is comparable to code that relies on PartialEq to behave properly (e.g., to be symmetric, transitive and stateless) -- the trait is safe, so that code must be unsafe if safety violations can occur from invalid PartialOrd implementations. There is a contract attached to PartialEq, but that contract may not be relied upon for safety.