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

RFC: Add Box::leak to leak Box<T> to &'static mut T #1233

Closed
wants to merge 4 commits into from

Conversation

mystor
Copy link

@mystor mystor commented Aug 1, 2015

Add the Box::leak static method to leak a Box<T> to a &'static mut T. This enables a safe action (leaking boxes into static references) which was previously impossible without unsafe in rust.

rendered

@Gankra
Copy link
Contributor

Gankra commented Aug 2, 2015

👍 this is a great trick to support. I slightly favour going through into_raw rather than transmute, but eh impl details.

I think it needs a where clause of T: 'a.

@Gankra Gankra self-assigned this Aug 2, 2015
@Gankra Gankra added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Aug 2, 2015

# Alternatives

We could also implement this with a `'static` bound on the function, always producing a `&'static mut T`. This has the advantage of making the type signature more explicit, but loses some of the generality of the `leak` method to also support leaking types with arbitrary lifetime parameters.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What generality is lost? Unless there ∃'x ≠ 'static for which 'x:'static (that is, 'x is longer than 'static), I’m not aware of any case where return type of &'static mut T is not as general as &'a mut T for any given 'a.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bound T: 'static is the loss of generality

@nagisa
Copy link
Member

nagisa commented Aug 2, 2015

The main question I have here is whether same thing should be implemented on ptr::Unique or not. Otherwise I’m ambivalent.

@Gankra
Copy link
Contributor

Gankra commented Aug 2, 2015

@nagisa &'static &'a u8 is not welformed. An unbound lifetime is more general.

@bluss
Copy link
Member

bluss commented Aug 2, 2015

Can you even use &'static mut T without unsafe?

@sfackler
Copy link
Member

sfackler commented Aug 2, 2015

Yes, it's no different than any other &mut T. It's just hard to get ahold of one normally.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2015

I'd prefer a Leakable trait, with only an impl for Box now, so it could later also be implemented for String, Vec and similar.

Would it be strictly backwards compatible to implement this for 'static now and extend it to an arbitrary lifetime later?

@mystor
Copy link
Author

mystor commented Aug 3, 2015

I'm not convinced that the extra constraints and finickiness of making the leak method available on a trait is worth it, especially because I can't imagine code which has to be generic over different ways to leaking memory. Box::leak is mostly interesting IMO as a neat trick, rather than as a pattern.

I'll add it to alternatives though.

@Gankra
Copy link
Contributor

Gankra commented Aug 3, 2015

It would indeed be back-compat to conservatively force 'static for now.

I'm not super interested in a Trait, but it wouldn't be unreasonable to
support it on String and Vec now that you mention it (this allows you to
avoid shrinking the capacity to fit for a Box).

On Mon, Aug 3, 2015 at 9:52 AM, Michael Layzell [email protected]
wrote:

I'm not convinced that the extra constraints and finickiness of making the
leak method available on a trait is worth it, especially because I can't
imagine code which has to be generic over different ways to leaking memory.
Box::leak is mostly interesting IMO as a neat trick, rather than as a
pattern.

I'll add it to alternatives though.


Reply to this email directly or view it on GitHub
#1233 (comment).

@SimonSapin
Copy link
Contributor

Box, String, and Vec could all have inherent leak methods, a trait is not needed for that. A trait is also less convenient as it needs to be in scope to be used.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2015

after some thought I agree, the trait is useless for generics and needs to be in scope. If there's need it can be in a crate or simply implemented locally.

@Gankra
Copy link
Contributor

Gankra commented Aug 5, 2015

@mystor Any thoughts on providing this on String and Vec too?

@mystor
Copy link
Author

mystor commented Aug 5, 2015

@gankro I'm fine with doing that, I'll add them to the RFC.

@alexcrichton
Copy link
Member

@gankro, @mystor for Vec and String, aren't they expressible in the form of:

Box::leak(vec.into_boxed_slice()); // => &'static mut [T]
Box::leak(string.into_boxed_str()); // => &'static mut str

@mystor
Copy link
Author

mystor commented Aug 5, 2015

@alexcrichton I think the reason why we'd want leak directly is that those methods resize the backing memory to be an exact fit, which may not be what you want. Leak avoids doing that by simply leaking the backing memory no matter how big it is.

@alexcrichton
Copy link
Member

That's a good point, yeah, but I wouldn't personally consider that justification enough just yet for the apparent duplication. It's unclear whether functions like this will be called much, in which case we may not need to have every use case covered (e.g. you can always implement it manually with transmutes + forgets.

@mystor
Copy link
Author

mystor commented Aug 6, 2015

That's true. I'd be fine with not including String and Vec as well (I believe I still have that option listed).

@Gankra
Copy link
Contributor

Gankra commented Aug 6, 2015

@alexcrichton Yeah that's fine

@Gankra
Copy link
Contributor

Gankra commented Aug 9, 2015

Hmm... looks like no one posted this after the IRC discussion: this API isn't safe if it returns &mut do to details of how the lifetime system actually works, you can't actually borrow an &'static mut -- it can be freely duplicated. So for instance, this function compiles:

fn dupe(data: &'static mut u8) -> (&'static mut u8, &'static mut u8) {
    (data, data)
}

Gotta go for shared refs.

@mystor
Copy link
Author

mystor commented Aug 9, 2015

@gankro weird, is that something special about &'static? That seems like very odd behaviour to me.

@Gankra
Copy link
Contributor

Gankra commented Aug 9, 2015

This is probably a misrepresentation, but my current intuition is:

  • the lifetime system is a constraint solver
  • 'static is already "the hardest to satisfy constraint"
  • we're just reborrowing the data
  • this does nothing to the constraints for 'static

To my knowledge this is basically fundamental. mutable static references are just unsafe.

@eddyb
Copy link
Member

eddyb commented Aug 9, 2015

@gankro That seems largely accurate. The implementation tracks borrows based on individualized scoped lifetimes, but 'static cannot borrow anything, it contains no information linking it to a borrow creating a reference with that ('static) lifetime.
I'm not sure it's even possible to forbid duplication of &'static mut T, due to generics.
Besides, what's the point of leaking data intentionally if you aren't going to share it?

@Gankra
Copy link
Contributor

Gankra commented Aug 9, 2015

@eddyb in principle it lets you do some work with it before fully sharing it (while you know you truly are the only one who has the reference). But yeah the whole lifetime system just falls over -- so ya gotta do all your work with the Box before you leak it (or use interior mutability... oh lord is that another hole...?).

@glaebhoerl
Copy link
Contributor

I still don't follow. Just because it happens to be 'static, shouldn't an &mut still be affine? Or is the hole that you can reborrow multiple times with &* (implicitly in the above example)? Why could you do that with &'static mut but not any other &'a mut?

@eddyb
Copy link
Member

eddyb commented Aug 9, 2015

@glaebhoerl Reborrowing borrows the original, but the borrow happens through the newly created lifetime.
&mut *static_mut_reference can create a new &'static mut T, and that can't borrow anything, not even the static_mut_reference it was created from.

@nrc
Copy link
Member

nrc commented Aug 9, 2015

My intuition is that we shouldn't do this - even though we classify leaking memory as safe behaviour, it is highly undesirable and we shouldn't make it ergonomic to do so, even if the name leak ought to be a hint.

Is there specific motivation for this? I'd like to see a real example of something that can't be done without this.

"it will be very easy for crates to implement this functionality themselves" indicates that this could be in a library crate and we could see if it gets enough usage to justify inclusion in the std libs as an inherent method on Box.

@alexcrichton
Copy link
Member

cc rust-lang/rust#27616, unfortunately it looks like this is currently not sound to implement.

@alexcrichton
Copy link
Member

This RFC is now entering its week-long final comment period.

My personal thoughts on this are that we shouldn't merge at this time because it's unsound, but I also agree with @nrc that it may not be worth exposing this so prominently

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Aug 27, 2015
@nagisa
Copy link
Member

nagisa commented Aug 27, 2015

It cannot get merged, at least until we figure out how to fix soundness issues. This FCP makes no sense to me.

I suggest to either close this PR outright or keep it open until the referenced issue is resolved.

@alexcrichton
Copy link
Member

@nagisa note that the libs team does not close or accept an RFC without an FCP. Moving into a final comment period does not guarantee that an RFC will be accepted, it simply means that the discussion has reached a conclusion or stalled and a decision can likely be made after a week.

@Gankra
Copy link
Contributor

Gankra commented Aug 28, 2015

Also it can trivially be merged with just changing &mut -> &, which is a perfectly reasonable request to make at the end of the FCP.

@glaebhoerl
Copy link
Contributor

Given that the &'static mut problem needs to be fixed anyways why wouldn't it be the right approach to suspend consideration of this RFC until that happens? (Whether that's by keeping it open or postponing it is a separate matter, and probably depends on how soon a fix is expected to happen.)

@strega-nil
Copy link

Agreed, we should postpone this until &'static mut is fixed.

@eternaleye
Copy link

Just to respond to something that I feel passed without enough comment:

@mystor

I think the reason why we'd want leak directly is that those methods resize the backing memory to be an exact fit, which may not be what you want. Leak avoids doing that by simply leaking the backing memory no matter how big it is.

This is unsound: The excess backing capacity may be uninitialized - in fact, that is frequently the only reason that a distinction between "size" and "capacity" exists at all. Therefore, Vec::leak() or String::leak() would have to do the same capacity-minimization as into_boxed_foo().

EDIT: On second thought, they may be advocating leaking the full thing, and returning a subslice. That's safe, though I fail to see the point.

@Gankra
Copy link
Contributor

Gankra commented Sep 9, 2015

@eternaleye yes, leaking a Vec would on yield the initialized elements. By contrast, I fail to see the point of reallocating and copying an array just to free a couple bytes in the heap (if you're "mostly" full). This is of course necessary if you want to Box, but not if you want to leak.

@mystor
Copy link
Author

mystor commented Sep 9, 2015

@eternaleye leaking without resizing doesn't give access to the uninitialized memory at the end of the array, it just makes that memory unavaliable to the allocator. It's just the same as putting the vector in a global variable, and then only passing around the slice into the vector.

@retep998
Copy link
Member

retep998 commented Sep 9, 2015

This would be so useful.
Seriously, get the soundness issue fixed and land this RFC.

@daboross
Copy link

daboross commented Sep 9, 2015

I don't think &'static mut being duplicable is a bug at all - I believe it to be mainly used with static mut, and changing this would be a breaking change to an established feature.

I think using &'static would be completely reasonable for this RFC.

@retep998
Copy link
Member

retep998 commented Sep 9, 2015

Being able to get a &'static mut from a static mut is an unsafe thing and rightly so, but there's nothing stopping you from getting more &'static mut from that static mut. However once you do have a &'static mut and you hand it off to some other safe code, that safe code should not be able to freely duplicate it and cause UB from safe code. Sure it was the unsafe code's fault for getting a &'static mut in the first place, but & is supposed to be a safe pointer! If the unsafe code makes sure there is only one such &'static mut then safe code should be safe to work with it! If you want an unsafe pointer that can be freely duplicated and is mutable use *mut, don't abuse &'static mut.

@mystor
Copy link
Author

mystor commented Sep 11, 2015

The soundness issues were fixed by rust-lang/rust#28321 (I think).

@sp3d
Copy link

sp3d commented Sep 11, 2015

Why should this be in the standard library rather than external to it? Leaking memory can't be "undone" in general, and anything that does leak memory is assuming it knows better than any other piece of software in the memory space what the process's lifecycle looks like: "I must have this memory, even after my destructors have been called, until process exit". That breaks composability in library code, which is definitely not something Rust should encourage by putting this into the standard library.

In a long-lived and complex application which might dynamically load and unload modules, permanent effects like memory leaks deep inside library code do cause negative effects (memory fragmentation, OOM despite having no Box values, etc.), and even though Rust doesn't prevent leaks in safe code with the type system, it should culturally acknowledge that purity and composability are useful properties and that leaking memory is to be avoided whenever possible.

It would suck to come across an application, try to refactor it into a library+wrapper, and then find out that you still can't meaningfully use the library in a larger process because it leaks all its memory. For use cases where you really do know what you're doing, it would be easy enough to write the one line of code here manually or get it from crates.io. Blessing this by placing it in the stdlib does not help Rust align incentives to encourage programmers to produce better code.

@Gankra
Copy link
Contributor

Gankra commented Sep 12, 2015

@sp3d I'll admit, my primary motivation on this was for applications, which do know if this is fine.

@sp3d
Copy link

sp3d commented Sep 12, 2015

Technically speaking, applications don't have to worry about external crates implementing their traits either, nor external API consumers breaking contracts, nor being embedded in multithreaded contexts if they aren't themselves multithreaded. In practice, trait extensibility, contracts in APIs, and thread-safety turn out to be useful things enforce everywhere and Rust benefits from doing so. I think the same is true of freedom from memory leaks. It's very easy for application authors to add one more crate to their Cargo.toml (or write the unsafe block!) if they do need this, but not so easy to redesign the memory management strategies of lots of disparate libraries that have decided leaking memory is easy and fun when you go to write a well-behaved piece of high-level code using said libraries.

@bstrie
Copy link
Contributor

bstrie commented Sep 15, 2015

+1 to including this function in the stdlib, modulo soundness concerns. IMO there is no greater service that the stdlib can provide than removing unsafe blocks from end-user crates by encapsulating unsafe functionality itself.

@alexcrichton
Copy link
Member

The libs team discussed this RFC during triage today and the conclusion was to close this RFC at this time. While this is indeed a pattern that could be added to the standard library, it's currently not greatly motivated in terms of use cases that require this sort of functionality to be in the standard library itself. These sorts of functions can always exist outside libstd without loss functionality.

@gankro brought up that the existence of these functions is a good "heads up" to this sort of functionality and how you may want to plan for it, but it's relatively the same as the existence of a safe mem::forget which already indicates "you must be prepared for your destructor to not be run.

Our thinking is that this should first be prototyped outside the standard library, and then if it gains traction and becomes popular can look into moving it in-tree, but in general we're leaning on a conservative standard library rather than adding "clever niceties" like this.

Regardless, though, thanks again for the RFC @mystor!

@codyps
Copy link

codyps commented Apr 13, 2016

For those interested, there is a leak crate that provides functionality similar to what this RFC was looking for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.