Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect drop order for JoinedCell #20

Closed
steffahn opened this issue Oct 4, 2021 · 6 comments · Fixed by #21
Closed

Incorrect drop order for JoinedCell #20

steffahn opened this issue Oct 4, 2021 · 6 comments · Fixed by #21

Comments

@steffahn
Copy link
Contributor

steffahn commented Oct 4, 2021

use self_cell::self_cell;

struct StrRef<'a>(&'a str);
impl Drop for StrRef<'_> {
    fn drop(&mut self) {
        println!("{}", self.0);
    }
}

self_cell! {
    struct Foo {
        owner: String,
        #[covariant]
        dependent: StrRef,
    }
}

fn main() {
    let foo = Foo::new("Hello World".into(), |s| StrRef(s));
} // prints garbage
@steffahn
Copy link
Contributor Author

steffahn commented Oct 4, 2021

This is a soundness issue you can run into accidentally, hence yanking old versions seems absolutely necessary.

@Voultapher
Copy link
Owner

What I find surprising is how this test case

fn custom_drop() {
didn't uncover this earlier.

@Voultapher
Copy link
Owner

Also one of those things you break during refactoring

// IMPORTANT: drop dependent before owner.
sigh

@Voultapher
Copy link
Owner

Voultapher commented Oct 4, 2021

Regardless, thanks for reporting and fixing the serious bug! I will release v0.10 and yank all older versions once #21 is merged.

@steffahn
Copy link
Contributor Author

steffahn commented Oct 4, 2021

Why not a point release of v0.9? This way users of 0.9.* don’t break; it’s a bugfix, so technically not really a breaking change, right?


What I find surprising is how this test case

fn custom_drop() {

didn't uncover this earlier.

I’m not entirely sure; indeed even MIRIFLAGS="-Zmiri-track-raw-pointers" cargo +nightly miri test doesn’t seem to complain though. Maybe the code is UB, maybe it’s not – what’s certainly a problem is that the Option<Vec<…>> is a None value, so dropping the owner doesn’t really do anything in practice.


Looking at https://crates.io/crates/self_cell/reverse_dependencies, opening a PR to https://crates.io/crates/bracket might be a reasonable idea (since they’re the only crates.io crate using self_cell below 0.9.*). Still assuming a 0.9.3 release instead of 0.10 – otherwise all reverse dependencies would need to be updated.


I’ve never done such a thing, but maybe this broken drop order is a reasonable addition to https://rustsec.org/ as well.

@Voultapher
Copy link
Owner

Regarding rustsec, never having done such a thing myself either, I wonder how applicable it is. I would assume that in practice it only shows up if a programmer writes code in certain way, not via malicious input. And given how relatively small this project still is. But I'm open to the idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants