Covariance check with `_assert_covariance` is insufficient, unsound

[steffahn]

use std::{cell::RefCell, fmt};

use self_cell::self_cell;

self_cell! {
    struct WrongVarianceExample {
        owner: (),
        #[covariant]
        dependent: Dependent,
    }
}

// this type is not covariant
type Dependent<'a> = RefCell<Box<dyn fmt::Display + 'a>>;

Dependent<'a> is not covariant in 'a, however unsize coercion still allows an OWNED value

RefCell<Box<dyn fmt::Display + 'x>>

to be coerced into

RefCell<Box<dyn fmt::Display + 'y>>

for lifetimes 'x: 'y, hence _assert_covariance compiles fine.

Unsoundness follows.

fn main() {
    let cell = WrongVarianceExample::new((), |_| RefCell::new(Box::new("")));
    let s = String::from("Hello World");

    // borrow_dependent unsound due to incorrectly checked variance
    *cell.borrow_dependent().borrow_mut() = Box::new(s.as_str());

    // s still exists
    cell.with_dependent(|_, d| println!("{}", d.borrow()));

    drop(s);

    // s is gone
    cell.with_dependent(|_, d| println!("{}", d.borrow()));
}

(run in Rust Explorer)

Hello World
p����U���

A better _assert_covariance needs to check whether coercing the Dependents behind a level of indirection is still possible.

Options include coercing Box<Dependent<'x>> to Box<Dependent<'y>>, or (more close to the actual use-case) coercing &'y Dependent<'x> into &'y Dependent<'y>.

[Voultapher]

Sigh, here we go again. Variance the bane of self-referential abstractions. Thanks for finding the issue and reporting and fixing it!

[Voultapher]

With the bug fixed on main, here are the next steps I propose:

• Release 1.0.2 with fix
• Release 0.10.3 with fix
• Yank all prior versions
• Open a RustSec advisory https://rustsec.org/advisories/RUSTSEC-2023-0070.html
• Write a release note

Please let me know if that aligns with your expectations.

[steffahn]

For 0.10, you could also add a point release containing the fix (for example, just the minimal fix changing the single line in the implementation),
or – assuming that it’s supposed to be identical / compatible with 1.0.* versions anyways – release a point release with a semver trick style construction (i.e. new version 0.10.3 that simply depends on version 1.0.2 and re-exports the API from it [provided you that that actually works and doesn’t do anything weird like breaking the $crate::… macro calls 🤷🏻]; that way, users of 0.10 can also benefit from future patches, without any further work/versions required).

I think it would make sense not to unnecessarily break the 0.10.* versions completely.

[Voultapher]

I'd really like to avoid having to maintain multiple versions, the migration from 0.10 to 1.0 is without any breakage.

[steffahn]

The point of a 0.10.3 version that depends on and re-exports the 1.* versions is that there’s nothing to maintain :-) This dependency means that if any further soundness issues is found, the transitive dependency from a 0.10.2 version will still work, even if 1.0.2 is yanked.

Migration is “without breakage” for the author of a migrating library, but yanking all 0.10 versions will be breakage in and by itself for any users that are not developers of the directly depending library in question - for example libraries or applications that depend on a library depending on self_cell 0.10 transitively, or users that try to install an application without lockfile. This kind of breakage can be worth it in case there is an actual issue, but among users of 0.10, it’s very likely that 100% of users don’t actually have involve unsound usage of self_cell in the first place.

In my personal view, the only reasonable approaches are to either not yank all 0.10 versions and keep the unsoundness around in those, or to publish a minimal 0.10.3 version that re-exports the API of version 1 and thus benefits from soundness fixes now and in the future, automatically.

I would even believe that publishing a single, minimal self_cell 0.10.3 version that re-exports the self_cell version 1 (and maybe contains a sentence of documentation that new users should use version 1 directly) is significantly less effort than opening PRs on all known dependents and waiting for them to be merged.

[steffahn]

I’ve written such a 0.10.3 version myself quickly. As far as I can tell, this works flawlessly (including the old_rust feature): https://github.com/steffahn/self_cell/tree/0.10.3

[Voultapher]

I find your arguments convincing. I want to publish a 0.10.3 based on your branch, but when I run cargo t I get:

error[E0464]: multiple candidates for `rlib` dependency `self_cell` found
   --> src/lib.rs:162:9
    |
162 | pub use self_cell::self_cell;
    |         ^^^^^^^^^
    |
    = note: candidate #1: <dir>/self_cell/target/debug/deps/libself_cell-57d4b17c39f385cc.rlib
    = note: candidate #2: <dir>/self_cell/target/debug/deps/libself_cell-6694495f354d6a30.rlib

[steffahn]

Hmm… I don’t think doctests failing is a problem though.

This does exactly follow the pattern of the semver trick repo, as far as I can tell. I think the error makes sense since doctests will add the library itself and its dependencies as a dependency, which then means in this case, two dependencies (of the doctest-generated code) with the same name. However, a crate that actually depends on self_cell would only specify one version as a dependency, and it works fine (I tested it locally with a path dependency on the 0.10.3 branch).

[steffahn]

It’s interesting though that it does point out the error in the self_cell source code here; I’m not familiar with the inner workings of doctests… I’ll check if there’s a workaround (such as renaming the dep) without any negative consequences.

[steffahn]

Tested it now; renaming seems to incur no problems, too, so I pushed a commit with the rename.

[Voultapher]

Thanks, I've published 0.10.3, and have yanked the affected versions.