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

Link Vec leak doc to Box #77709

Merged
merged 3 commits into from
Oct 10, 2020
Merged

Link Vec leak doc to Box #77709

merged 3 commits into from
Oct 10, 2020

Conversation

pickfire
Copy link
Contributor

@pickfire pickfire commented Oct 8, 2020

No description provided.

@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 8, 2020
@jyn514 jyn514 added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Oct 8, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 8, 2020

r=me with CI passing

@pickfire
Copy link
Contributor Author

pickfire commented Oct 8, 2020

This function is mainly useful for data that lives for the remainder of the program's life. Dropping the returned reference will cause a memory leak.

I still don't quite know how to prevent memory leak from reading the docs, I found this in the release notes.

@jyn514
Copy link
Member

jyn514 commented Oct 8, 2020

I think the right way to avoid a leak is Vec::from(Box::from_raw(ptr))? But I don't know how that interacts with fat pointers and raw slices.

cc @rust-lang/wg-unsafe-code-guidelines - how do you get a Vec back after you've leaked it?

@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 8, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Oct 8, 2020

Turning the leak back into a Vec can be tricky and dangerous. If the leak() is 'static, anything could've copied that reference and access it at any point until the end of the program.

@RalfJung
Copy link
Member

RalfJung commented Oct 8, 2020

The docs do not even guarantee that into_boxed_slice is used, so in principle there could be extra capacity after the initialized part of the Vec -- which means restoring the original Vec actually is impossible as the capacity is lost.

If recovering from the leak is an intended feature, that would require a new guarantee currently not made by the docs.

@jyn514
Copy link
Member

jyn514 commented Oct 8, 2020

Got it, thanks. So I think we should document that this is not currently supported; right now it seems to imply there's a way if only you figure it out:

   /// This function is mainly useful for data that lives for the remainder of
    /// the program's life. Dropping the returned reference will cause a memory
    /// leak.

@pickfire
Copy link
Contributor Author

pickfire commented Oct 9, 2020

Maybe we should mention that there is no way to recover the leak unlike box? Since for box it show some methods to recover the leak but I don't see that for vec, maybe it would be good to mention it to be explicit.

Sounds like a potential footgun, I feel like the function should be unsafe even though the word "leak" already feels scary enough.

@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

I feel like the function should be unsafe

#24456

@RalfJung
Copy link
Member

RalfJung commented Oct 9, 2020

To elaborate on what @jyn514 said: "footgun" is not what unsafe is about.

unsafe is about whether safe code can cause UB by calling this function in arbitrary ways. The answer here is "no", and thus it should not be unsafe.

@pickfire
Copy link
Contributor Author

pickfire commented Oct 9, 2020

It can't cause UB but it can cause memory leak, so memory leak is safe? Yeah, I agree that causing UB is definitely unsafe but memory leak feels unsafe to me too.

@Mark-Simulacrum
Copy link
Member

Memory leaking must be safe; we cannot prevent it: e.g., mem::forget is safe, see also the issue @jyn514 linked #24456.

@RalfJung
Copy link
Member

RalfJung commented Oct 9, 2020

UB for Rust is defined here. Memory leaks indeed are not on that list.

@pickfire
Copy link
Contributor Author

Warning: The following list is not exhaustive. There is no formal model of Rust's semantics for what is and is not allowed in unsafe code, so there may be more behavior considered unsafe. The following list is just what we know for sure is undefined behavior. Please read the Rustonomicon before writing unsafe code.

Should we clarify that memory leak won't be part of that list?

@RalfJung
Copy link
Member

Many things are not part of that list, listing them all could get exhausting. ;)
But anyway that is off-topic for this PR. Please open an issue for the reference if you want to continue this discussion.

For the PR, I agree the docs here should be clarified to say that there is no supported way to "undo the leak".

library/alloc/src/vec.rs Outdated Show resolved Hide resolved
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

r=me with nit fixed

library/alloc/src/vec.rs Outdated Show resolved Hide resolved
Co-authored-by: Joshua Nelson <joshua@yottadb.com>
@jyn514
Copy link
Member

jyn514 commented Oct 10, 2020

@bors r+ rollup

Thanks for the PR and starting a good discussion :)

@bors
Copy link
Contributor

bors commented Oct 10, 2020

📌 Commit 8688fa8 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 10, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 10, 2020
Rollup of 10 pull requests

Successful merges:

 - rust-lang#77195 (Link to documentation-specific guidelines.)
 - rust-lang#77629 (Cleanup of `eat_while()` in lexer)
 - rust-lang#77709 (Link Vec leak doc to Box)
 - rust-lang#77738 (fix __rust_alloc_error_handler comment)
 - rust-lang#77748 (Dead code cleanup in windows-gnu std)
 - rust-lang#77754 (Add TraitDef::find_map_relevant_impl)
 - rust-lang#77766 (Clarify the debug-related values should take boolean)
 - rust-lang#77777 (doc: disambiguate stat in MetadataExt::as_raw_stat)
 - rust-lang#77782 (Fix typo in error code description)
 - rust-lang#77787 (Update `changelog-seen` in config.toml.example)

Failed merges:

r? `@ghost`
@bors bors merged commit 45e3574 into rust-lang:master Oct 10, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 10, 2020
@pickfire pickfire deleted the patch-1 branch October 11, 2020 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants