p-avital / token-cell-rs

An alternative to GhostCell
5 stars 1 forks source link

TokenCell's design is unsound #1

Open Qwaz opened 3 years ago

Qwaz commented 3 years ago

TokenCell's README says that it provides the same compile-time guarantees as ghost-cell. However, TokenCell's API design doesn't prevent generating the same brand multiple times. For instance, it is possible to generate aliased mutable references with TokenCell as follows:

#![forbid(unsafe_code)]
use token_cell::*;

generate_static_token!(Token);

fn main() {
    let mut token1 = Token::new();
    let mut token2 = Token::new();

    let cell = TokenCell::new(1);

    let ref1 = cell.borrow_mut(&mut token1);
    let ref2 = cell.borrow_mut(&mut token2);

    assert_ne!(ref1 as *mut _, ref2 as *mut _);
}

Output:

thread 'main' panicked at 'assertion failed: `(left != right)`
  left: `0x7fffeaec8660`,
 right: `0x7fffeaec8660`', src/main.rs:15:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
p-avital commented 3 years ago

Hi,

Yes, I'm aware that TokenCell with static tokens isn't sound by design, the comparison with GhostCell is mostly used because it seemed like the most appropriate comparisonn in its purpose. My issue with GhostCell is that it's poorly suited to async, which expects lifetimes to be 'static.

In my very first attempt, I made static tokens instantiable only once, which would make them sound, but this isn't compatible with having multiple instances of a type that contains such tokens. To address this soundness issue, RuntimeToken is a token which adds a runtime check on a unique ID to ensure the cell matches the token it was created with.

I think I placed token_cell in 1.x.x prematurely, I'm planning on working a bit with procedural macros to try and make static cells sounder. I'm trying to figure out ways to prevent the user from having multiple instances of a token-type co-exist within a scope.

I also think I'll change static tokens such that in debug mode, they behave like RuntimeToken, allowing debug to catch these soundness-holes, while letting it be zero-cost in release mode.

Thanks for getting me to think a bit more about this project. If you have any ideas, I'm very willing to take advice.

Qwaz commented 3 years ago

The reason I brought up this issue was that I saw someone in the Rust community asking if they should use token-cell instead of GhostCell. I think the current README is slightly misleading in this regard because it does not provide the exactly same compile-time guarantee with GhostCell and doesn't clearly state the safety condition. Thanks for sharing the future plan and being responsive!

In terms of API design, I think tokenlock's approach is worth checking. The code is a little complicated because it tries to be generic over runtime checked tokens and compile time checked tokens, but I think it is well-designed at high level.

p-avital commented 3 years ago

Yeah, the purpose is similar, but I went for 1.0.0 way to early. You can check #2 for more details. I've decided to rework this create hard, by traitifying all the things. Latest commit on master is a draft of that.

Unfortunately, I must be missing something because the branding doesn't appear to work properly with the version of GhostToken currently on master :(