uazu / qcell

Statically-checked alternatives to RefCell and RwLock
Apache License 2.0
356 stars 22 forks source link

`TCell` is unsound due to covariant `Q` parameter of `TCellOwner` (and the same applies to `TLCell`) #20

Closed steffahn closed 2 years ago

steffahn commented 2 years ago
use qcell::{TCell, TCellOwner};

type T1 = fn(&());
type T2 = fn(&'static ());

// T1 subtype of T2, both 'static

// TCellOwner covariant
fn _demo(x: TCellOwner<T1>) -> TCellOwner<T2> {
    x
}

// and that's obviously bad

fn main() {
    let first_owner = TCellOwner::<T2>::new();
    let mut second_owner = TCellOwner::<T1>::new() as TCellOwner<T2>;

    let mut x = TCell::<T2, _>::new(vec!["Hello World!".to_owned()]);
    let reference = &first_owner.ro(&x)[0];
    second_owner.rw(&x).clear();

    println!("{}", reference); // ��&d��i
                               // (or similar output)
}
steffahn commented 2 years ago

Similar problem with covariance of Q for the cell itself

use qcell::{TCell, TCellOwner};

type T1 = fn(&());
type T2 = fn(&'static ());

// T1 subtype of T2, both 'static

fn main() {
    let first_owner = TCellOwner::<T1>::new();
    let mut second_owner = TCellOwner::<T2>::new();

    let mut x = TCell::<T1, _>::new(vec!["Hello World!".to_owned()]);
    let reference = &first_owner.ro(&x)[0];
    second_owner.rw(&x as &TCell::<T2, Vec<_>>).clear();

    println!("{}", reference); // ��&d��i
                               // (or similar output)
}
steffahn commented 2 years ago

AFAICT, all published versions of qcell are affected.

uazu commented 2 years ago

Thanks for spotting this. So I guess this is because Rust will cast the type, but gives it a different type-ID. I will go through your PR later and make sure I fully understand the issue and the solution. Thanks again.

uazu commented 2 years ago

Fixed in version 0.4.3. All previous versions have been yanked. I think it's unlikely anyone is stuck on pre-0.4 versions. Let me know if this causes someone a problem.

Note for anyone trying to understand the issue: this bug does not affect non-malicious programmers. Existing non-malicious code remains sound. By fixing this bug and yanking all other affected versions all that we're doing is potentially stopping a malicious coder from creating undefined behaviour without using unsafe, and sneaking bad effects through a safety review.