rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.9k stars 12.78k forks source link

`#[may_dangle]`, a refined dropck escape hatch (tracking issue for RFC 1327) #34761

Open pnkfelix opened 8 years ago

pnkfelix commented 8 years ago

Tracking issue for https://github.com/rust-lang/rfcs/pull/1327

For docs explaining the feature, also see the forge.

pnkfelix commented 8 years ago

see also #28498

pnkfelix commented 8 years ago

Some people (myself included) have noted that may_dangle is not an ideal name. (Some have gone so far as to call it "terrible".)

Perhaps we could return to calling it an eyepatch:

3vvuqta

RalfJung commented 7 years ago

I have to say I find may_dangle clearer than eyepatch. After all the idea (if I understood it correctly) is that for the given parameters, it may be the case that they do not necessarily outlive the current function call.

asajeffrey commented 6 years ago

An issue that came up in the context of Josephine is https://github.com/asajeffrey/josephine/issues/52. This is mainly an issue for the interaction between the drop checker and untagged unions (https://github.com/rust-lang/rust/issues/32836#issuecomment-362369802) but does have impact on the code guidelines for when may_dangle may be used safely.

The issue is the text "When used on a type, e.g. #[may_dangle] T, the programmer is asserting the only uses of values of that type will be to move or drop them," and whether the programmer is allowed to discard values of type T, that is reclaim the memory used by the value without running T's drop code. In particular, is the programmer allowed to safely mem::forget a value of type T?

SimonSapin commented 6 years ago

std::mem::forget itself is not unsafe. I believe that leaking any value (never running destructors) is considered sound in Rust. (This is why for example scoped threads are based on closures rather than "guard" values with destructors.)

glaebhoerl commented 6 years ago

Yes, he knows, that's not what he's asking.

The part of the equation that's unsafe and whose contract is in question is #[may_dangle].

RalfJung commented 6 years ago

The issue is the text "When used on a type, e.g. #[may_dangle] T, the programmer is asserting the only uses of values of that type will be to move or drop them," and whether the programmer is allowed to discard values of type T, that is reclaim the memory used by the value without running T's drop code. In particular, is the programmer allowed to safely mem::forget a value of type T?

Personally I don't consider mem::forget as using its argument. It does quite explicitly the opposite, actually. ;)

My mental model for #[may_dangle] T is that it provides a way to "opt out" of the usual implicit side-conditions Rust has on all type and lifetime arguments that they be alive in the current function. When you write

fn foo<'a, T>(x: &'a i32, y: T)

then there are some implicit conditions (where clauses) added to the function, namely that 'a and T both outlive the duration of the function call. #[may_dangle] T prevents these where clauses from being added, so if we are in a function that has a #[may_dangle] T type parameter and would try to call a function that has a T type parameter, we'd be unable to satisfy the where clauses of the callee. That's why we are not allowed to call said function.

Right now this is all specialized to be just about drop, but that's mostly because this is the only place the need for such a parameter has come up. If we ignore that, mem::forget could be declared as fn forget<#[may_dangle] T>(x: T) in order to express that it can be called on dangling data.

nox commented 6 years ago

First, why would adding #[may_dangle] on mem::forget allow perma-borrowed values such as values of the Pin type in https://github.com/rust-lang/rust/issues/32836#issuecomment-362369802 to be forgotten? mem::forget takes ownership of its argument and in Alan's code you can't take ownership of the pinned value. Even if the forget function had #[may_dangle] on its type parameter, it would be pretty unsound to be able to call it while the value is borrowed, don't you think?

Second, what even would be the purpose of adding #[may_dangle] on mem::forget? People believe that what Josephine currently does was a happy accident all that time prior to ManuallyDrop's existence, but what is the purpose of allowing to forget dangling data? What new uses cases does it bring to the table of Rust users, apart from breaking Josephine's use case?

RalfJung commented 6 years ago

First, why would adding #[may_dangle] on mem::forget allow perma-borrowed values such as values of the Pin type in #32836 (comment) to be forgotten?

I didn't say that. All I did is answer the question (quoted in my reply) of whether mem::forget can be called in a drop that uses #[may_dangle] affirmatively. The question did not even mention perma-borrowing. I did not make the connection to the Pin trick nor do I see it. :)

Second, what even would be the purpose of adding #[may_dangle] on mem::forget?

No idea. @asajeffrey quite specifically asked about this above, so I gave my 2 cents.

Or did I entirely misunderstand their question...?

asajeffrey commented 6 years ago

Hmm, good point @RalfJung, perhaps my model of may_dangle needs to broaden...

I'd been thinking about it as specific to the drop checker, in that code such as:

fn foo(x: T) {
  // ... do stuff with x that doesn't transfer ownership
}

informally desugars to:

fn foo(mut x: T) {
  // ... do stuff with x that doesn't transfer ownership
  T::drop(&mut x); // if T implements Drop
  mem::forget(x);
}

but that &mut x borrow impacts the borrow-checker, and may_dangle tells the drop-checker to pretend it's not there.

But there may be a way of thinking about it more generally, as loosening the requirement that in &'a T, we have T: 'a. This is what @nikomatsakis was saying in IRC at https://botbot.me/mozilla/rust-lang/2018-01-31/?msg=96353190&page=2

@RalfJung there certainly are ways of reading the text as allowing mem::forget(x), e.g. as you say you could say this is not using x. I guess I'd just like the text to be a bit more explicit about this.

Also, your example at https://github.com/rust-lang/rust/issues/32836#issuecomment-362551916 is interesting. Slightly messed around with, it's:

struct Transmuted<T> {
    data: Vec<u8>,
    marker: PhantomData<T>,
}
impl<T> Transmuted<T> {
    fn new(x: T) { ... horrible mem::transmute to store the byte representation of x in data }
}
impl<T> Deref for Transmuted<T> {
    type Target = T;
    fn deref(&self) -> &T { ... more horrible transmuting ... }
}
impl<#[may_dangle] T> Drop for Transmuted<T> {
    fn drop(&mut self) { ... what can we do with data here? ... }
}

We're not allowed to dereference data as a T, but it's not obvious to me what we're allowed to do with it as a Vec<u8>.

RalfJung commented 6 years ago

(Quoting from another thread but I think it better fits here)

your code shows an interesting corner case, where the run-time type for data is T, but it's compile-time type is [u8; N]. Which type counts as far as may_dangle is concerned?

If the code says #[may_dangle] T, the rules "that type" refer to T, I'd say. Not sure what else it could be?

there certainly are ways of reading the text as allowing mem::forget(x), e.g. as you say you could say this is not using x. I guess I'd just like the text to be a bit more explicit about this.

I think we'd all like to better understand what it takes to make this a usable, modular and ideally safe concept. :)

Also, your example at #32836 (comment) is interesting. Slightly messed around with, it's:

We're not allowed to dereference data as a T, but it's not obvious to me what we're allowed to do with it as a Vec.

T could contain padding and uninitialized data. So in terms of bytes I think we can do with it about as much as we can do with a mem::uninitialized::[u8; 32]().

Except, of course, we know it was a T at some time! I think this is no different than a struct that actually contains a T field, except that there's no automatic destructor because the compiler has no idea. So I think transmuting to a T should be fine, and then we can do anything on it that respects the #[may_dangle] rules. Whatever that means. In my interpretation, it means we can call any method that has relaxed its implicit bounds so as to not require T to be alive, like mem::forget.

Actually most methods that work for any generic T could be soundly written that way, which lead to the original parametricity-based rule -- after all, if you don't know anything about T, all you can do is move it, and that's fine by #[may_dangle] rules.

(And this is where I realized that I can explain the failure of the parametricity-based argument in terms of my #[may_dangle] interpretation.)

Of course, it turned out this does not work. The counterexample involves a type like

struct S<T> { x: Option<T>, f: fn (T) }

And of course, f does have all the usual implicit where clauses! So calling it from drop that opted out of the where clauses is unsound. We were in a context where the WF assumptions about T are relaxed, and called into a function that expects the WF assumptions to fully hold.

RalfJung commented 6 years ago

I'd been thinking about it as specific to the drop checker, in that code such as:

Thanks a lot for this! I came up with my model mostly because I didn't see any model defined anywhere, so this is very useful.

What I'm currently struggling with is reconciling "but that &mut x borrow impacts the borrow-checker, and may_dangle tells the drop-checker to pretend it's not there" with the fact that #[may_dangle] is an attribute on a type parameter, not on a function argument. If we have

unsafe impl <#[may_dangle] T, A: Allocator> Drop for AllocVec<T, A> {
  fn drop(&mut AllocVec<T, A>) {
    // can rely on A being alive, but not T
  }
}

then how is that reconciled by the borrow checker at the drop call site?

I like your desugaring into drop-wth-borrow + mem::forget as it gives us some idea of why the type of drop is &mut. However, that type is still blatantly wrong -- if it were correct, it'd be safe to desugar things to

drop(&mut x);
drop(&mut x);
mem::forget(x);

which is clearly not the case. drop will stay a fishy function.

What I think I'd like to add to the picture is where exactly the lifetimes end. In a usual situation, we may have something like this:

// Assume some outer lifetime `'a`
let v : Vec<&'a i32> = Vec::new(); // lifetime of this variable: 'v
// ...
// The vector "dies" during the drop call, so we have to end the lifetime *before*
// calling drop.
EndLifetime('v);
drop(&mut v); mem::forget(v);
EndLifetime('a);

Everything is fine here as 'a is alive while Vec<&'a i32>::drop is executed.

However, if we have cyclic references between elements in the Vec (or with your Pin inside a ManuallyDrop), we have a situation more like

let v : Vec<&'v i32> = Vec::new(); // lifetime of this variable: 'v
// It's literally cyclic! The type of this variable refers to its own lifetime.
EndLifetime('v); // As usual end the lifetime of a variable before it is dropped
drop(&mut v); // Uh-oh. 'v is dead and yet we are calling Vec<&'v i32>::drop. *explosion sound*

Because the type of this variable involves its own lifetime, there just is no way to end that lifetime and call the destructor in any order. So Rust rejects this, unless it knows that Vec<&'v i32>::drop is actually safe to call even though &'v i32 is not well-formed (because the lifetime has ended).

This scales to AllocVec: AllocVec<&'v i32, Allocator<'a>>::drop is safe to call even if 'v has ended, but 'alloc must still be a live. So we can safely have a

let v : AllocVec<&'v i32, Allocator<'a>>

but we cannot have a

let v : AllocVec<&'v i32, Allocator<'v>>

So, I guess I just repeated my point about WF checks from the perspective of the drop call site: The actual &mut x borrow for drop is just fine (x itself is not going to get deallocated or borrowed anywhere else!), but the type of &mut x is not entirely well-formed because some of the lifetimes it refers to may have already ended. So before calling drop we have to make sure that the concrete drop implementation we call is actually fine with relaxing the WF constraints to the extend that we can satisfy them.

Please let me know what you think... I'm kind of making this up as I go here; I've had plenty of fuzzy thoughts for a while but this is the first time I'm writing them down.

asajeffrey commented 6 years ago

Trying to make my slightly incoherent examples more precise...

The kind of thing I'm worried about is something like:

struct Bar<'a>(*const String, PhantomData<&'a()>);
impl<'a> Bar<'a> {
    fn new(x: &'a String) -> Bar<'a> { Bar(x, PhantomData) }
}
impl<'a> Deref for Bar<'a> {
    Target = String;
    fn deref(&self) -> &String { unsafe { &*self.0 } }
}
unsafe impl<#[may_dangle] 'a> Drop for Bar<'a> {
    fn drop(&mut self) { unsafe { println!("{}", &*self.0) } }
}

AFAICT

a) without the may_dangle this is safe, and b) it passes the may_dangle rules since it never uses anything whose static type has lifetime 'a. (it derefs self.0 but that has type *const String not &'a String).

But it's unsafe, since it's printing a string that might have been reclaimed.

This is what I'm meaning about which type (e.g. for self.0) are we talking about, it's run-time "real" type &'a String or its compile-time type *const String.

asajeffrey commented 6 years ago

OK, the desugaring is a bit inaccurate, instead of:

T::drop(&mut x);
T::drop(&mut x);
mem::forget(x);

it should be something like:

if !x.drop_flag { T::drop(&mut x); x.drop_flag = true; }
if !x.drop_flag { T::drop(&mut x); x.drop_flag = true; }
mem::forget(x);

but I don't think this affects the typing.

Putting in the explicit lifetimes is interesting, as it makes clear the difference between the usual desugaring:

  T<'a>::drop(&mut x);
  EndLifetime('a);
  mem::forget(x);

and the one that #[may_dangle] 'a causes:

  EndLifetime('a);
  T<'static>::drop(&mut x);
  mem::forget(x);

that is a T<'a> value is being used after EndLifetime('a).

arielb1 commented 6 years ago

But it's unsafe, since it's printing a string that might have been reclaimed.

Bar with a may_dangle is unsafe because it prints out of a raw pointer that might be deallocated.

The implementation of Bar might maintain an invariant that the raw pointer is live for 'a - in that case, an implementation without #[may_dangle] would be sound, because 'a is known to be alive.

asajeffrey commented 6 years ago

@arielb1 indeed, the issue is one of the definition of the may_dangle safety rules, that you can have code that is correct without may_dangle, satisfies the may_dangle rules, but is incorrect. Oops.

RalfJung commented 6 years ago

it passes the may_dangle rules since it never uses anything whose static type has lifetime 'a. (it derefs self.0 but that has type *const String not &'a String).

I disagree. The wording in the RFC is

When used on a lifetime, e.g. #[may_dangle] 'a, the programmer is asserting that no data behind a reference of lifetime 'a will be accessed by the destructor.

Notice it doesn't say "static". Semantically speaking, your *const String is a reference with lifetime 'a, and you are accessing data behind that reference. You are hiding this from the compiler, but that doesn't make any difference when reasoning about the operations you are safely permitted on this data.

So, your code violates my reading of the #[may_dangle] rules. And rule lawyering aside, if you think in terms of "what are my proof obligations when I want to show this unsafe code to be safe" (and I think this should always be the mindset when justifying unsafe code; counterexample-guided reasoning doesn't scale), one of the things you have to do is justify why you should be permitted to dereference this raw pointer. As @arielb1 said, this is where #[may_dangle] makes the difference because it tells you whether your proof can rely on 'a being alive.


it should be something like:

if !x.drop_flag { T::drop(&mut x); x.drop_flag = true; }
if !x.drop_flag { T::drop(&mut x); x.drop_flag = true; }
mem::forget(x);

I wasn't talking about the drop flag. My comment about the type being a lie refers to the fact that drop destroys the data behind the reference. If you call some "normal" function of type foo(&mut Vec<i32>), you can rely on this function to leave the vector in a valid state. That's part of the contract of &mut. On the other hand, drop(&mut Vec<i32>) blatantly violates that contract because it wrecks havoc on the data structure.

and the one that #[may_dangle] 'a causes:

 EndLifetime('a);
 T<'static>::drop(&mut x);
 mem::forget(x);

This should be T<'a>::drop(&mut x);, right?

arielb1 commented 6 years ago

So one version of the invariant josephine wants is basically this:

If a place is borrowed, and then (of course, after the borrow is over) the memory in that place is overwritten, then either:

  1. The data in the place was alive and unborrowed at the same time.
  2. The destructor is called at the place

That makes josephine sound: after a place is borrowed for &'a Foo<'a>, it can never be simultaneously live and unborrowed from that point on, which means the memory can't be invalidated without the destructor being called.

The invariant also probably holds in "specified pre-1.20 Rust": in safe code, if a place is memory-overwritten, then (up to the VecDeque bug, which we could declare "just a soundness bug") the data at that place was already freed, which requires it to either have been moved out (which requires it to be both alive and unborrowed at the point of the move) or have its destructor called.

OTOH, that invariant is never something we tried to guarantee. In fact, a #[may_dangle] Rc on top of an arena allocator breaks it:

struct Cyclic<A, T> {
    cycle: RefCell<Option<Rc<Cyclic<T, A>, A>>,
    data: T
}

fn foo() {
    let arena = make_arena();
    let rc1 = arena <- Cyclic {
        cycle: Default::default(),
        data: Pin::new(),
    };

    // make a cycle    
    *rc1.cycle.borrow_mut() = Some(rc1.clone());
    rc1.data.pin();

    // at end-of-scope, no destructors are run, but the arena (and its containing Rc) are free-ed.
}
Centril commented 6 years ago

What is blocking stabilization of generic_param_attrs ? I think that should be stabilized irrespective of #[may_dangle] as it is useful for custom derive.

nikomatsakis commented 6 years ago

@Centril nothing really...

nox commented 6 years ago

What can we do to stabilise generic_param_attrs? I want to use custom derive attributes in type parameters in Stylo, which runs on stable.

SimonSapin commented 6 years ago

@nox I would guess: make a PR that stabilizes it and ask the lang team to do a FCP.

nikomatsakis commented 6 years ago

@nox

What can we do to stabilise generic_param_attrs? I want to use custom derive attributes in type parameters in Stylo, which runs on stable.

Best thing: create a new issue for just that stabilization. Describe briefly the behavior you want to stabilize (as there is no RFC we need a bit more explanation, but then this is very simple so that seems easy enough). Ideally, show pointers to some tests that include that behavior. I would particularly be interested in tests that show that we properly error on unrecognized attributes (like #[foo]) used in that position (i.e., we want to avoid accepting things we don't expect). cc me or the team and we'll get it going!

To be clear: I think we can stabilize this. =)

petrochenkov commented 6 years ago

I would particularly be interested in tests that show that we properly error on unrecognized attributes (like #[foo]) used in that position

ui/feature-gate-custom_attribute2.rs All the other notable tests can be found in https://github.com/rust-lang/rust/pull/34764 I'll send the stabilization PR today.

i.e., we want to avoid accepting things we don't expect

Unknown attributes are easy, but this is much harder because known attributes are generally accepted in any weird positions with any syntax without validation, e.g. we already accept a lot of things we don't expect

fn main() {
    #[inline(x = "y")] // OK
    let x = 10;
}

Fixing this is somewhere in my queue though! Maybe I'll even get to it this year.

tesuji commented 5 years ago

62568 will remove support for #[unsafe_destructor_blind_to_params] attribute.

qnighy commented 5 years ago

Is there anything I can help to stabilize this feature?

pnkfelix commented 4 years ago

I don't think we intend to stabilize #[may_dangle] as is. It is a stop-gap measure until we can develop a more disciplined solution to the problem (e.g., some kind of where-clause on the type, perhaps coupled with an effect-system to allow us to actually check that one is adhering to the may-dangle rules).

vincent-163 commented 4 years ago

Hello. I'm proposing out-of-lifetime references which may be a potential solution to this problem: https://internals.rust-lang.org/t/proposal-about-out-of-lifetime-references/11675/11. Please take a look and leave any feedback or suggestions!

KodrAus commented 4 years ago

We have some new docs in the forge on #[may_dangle] that folks trying to understand it may find helpful. It includes a real example of where we (by we I mean me) got it wrong the first time.

m-ou-se commented 2 years ago

I don't think we intend to stabilize #[may_dangle] as is. It is a stop-gap measure until we can develop a more disciplined solution to the problem (e.g., some kind of where-clause on the type, perhaps coupled with an effect-system to allow us to actually check that one is adhering to the may-dangle rules).

It's been a few years, and it doesn't seem like there will be a proper solution for this problem any time soon. Maybe it's time to stabilze #[may_dangle], to allow crates other than core/alloc/std to use it?

camsteffen commented 2 years ago

Trivial blocker. There doesn't seem to be any restriction on where #[may_dangle] is allowed. It is currently allowed on const params, types, statements, etc. and shouldn't be AFAICT. (playground)

Manishearth commented 2 years ago

perhaps coupled with an effect-system to allow us to actually check that one is adhering to the may-dangle rules

I feel like for an unsafe escape hatch it's probably not a great idea to have a strong effect system situation? If you're using this you're already doing some unsafe tricky thing; the user is already in a situation where they can't prove safety to the compiler; asking them to prove safety seems suboptimal

I also do agree that this should be stable; being able to implement a stable Vec<T> seems like something that rust should let you do, and there's no easy workaround to this at the moment.

Manishearth commented 2 years ago

A workaround hack that can sometimes work is taking types of the form:

pub struct MyAbstraction<'a, T> {
   stuff: *const T,
   more_stuff: bool,
   even_more_stuff: usize,
   marker: PhantomData<&'a T>,
}

and turning them into this:


pub struct MyAbstraction<'a, T> {
   eyepatch: MyAbstractionEyepatchHack< T>,
      marker: PhantomData<&'a T>,
}
struct MyAbstractionEyepatchHack<T> {
   stuff: *const T,
   more_stuff: bool,
   even_more_stuff: usize,
}

and implementing Drop on the inner type. It's more of a problem if the lifetime is really needed in your internal stuff, though.

GoldsteinE commented 3 months ago

It is a stop-gap measure until we can develop a more disciplined solution to the problem (e.g., some kind of where-clause on the type, perhaps coupled with an effect-system to allow us to actually check that one is adhering to the may-dangle rules).

Nearly five years have passed since this comment. As far as I’m aware, there’ve been no proposed solution and no goal to make one. I think it’s safe to say by this point that unless some actions are deliberately taken, the status quo will remain indefinitely.

I think this state of affairs is less than satisfactory. As of now, it’s impossible to properly implement collections outside of the standard library. This interacts poorly with std’s (completely reasonable) desire to remain lean and leave more complex features to the ecosystem. For example, #56167 was recently closed because one can just use hashbrown, but hashbrown can’t provide a proper dropck interaction on stable. You need to choose between accurate dropck and advanced collection features.

I don’t think blocking this on figuring out effect system, a massive feature that’s not really planned, is reasonable. A different syntax (e.g. where clause instead of an attribute) doesn’t need to block stabilization of this one — a lot of features changed syntax over time, e.g. try!() or recently addr_of!(), and it’s usually not a big deal.

I think it’s time to at least reevaluate the decision to leave this perma-unstable (even if it results in “yes, we’re leaving this perma-unstable because {reasons}” from T-lang). For what it’s worth, I fixed the technical blocker highlighted by @camsteffen.

GoldsteinE commented 2 months ago

I chatted in Zulip for a bit, so here’s the status of this as I understood it.

The current version of #[may_dangle] will not be stabilized as it’s too error-prone. There’re some proposals for a different semantics, most notably https://github.com/rust-lang/rfcs/pull/3417, but they’re kinda stalled. lcnr, who is the author of that one, said:

I would be happy if someone were to look into that RFC to push it's core idea over the finish line though and would then be fine with stabilizing it. That's a lang team question though

I’m not totally sure, but as far as I understand it means it’s blocked on t-lang.

Here’re the Zulip threads:

RalfJung commented 2 months ago

It's not fully blocked on t-lang -- it needs someone who's taking over that PR to suggest and explain the idea to t-lang.

However, before doing that it'd probably be good to ask for a t-lang liaison to ensure the team has the bandwidth to consider the proposal.