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

Restrict Allocator impl to &'static A #94069

Closed
wants to merge 1 commit into from

Conversation

djkoloski
Copy link
Contributor

Arbitrary &'_ Allocators cannot satisfy the safety requirement that
memory blocks returned by an Allocator must remain valid until the
instance and all of its clones are dropped. &'static Allocators can
still satisfy this safety requirement because their lifetime ensures
that they are never moved or dropped.

See #90822 and this comment in particular for additional context.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Feb 16, 2022
@djkoloski
Copy link
Contributor Author

r? @yaahc

@rust-highfive rust-highfive assigned yaahc and unassigned m-ou-se Feb 16, 2022
@rust-log-analyzer

This comment has been minimized.

@djkoloski djkoloski force-pushed the restict_allocator_impl branch 2 times, most recently from 8c21c1b to cdf8314 Compare February 17, 2022 02:56
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 17, 2022
Arbitrary &'_ Allocators cannot satisfy the safety requirement that
memory blocks returned by an Allocator must remain valid until the
instance and all of its clones are dropped. &'static Allocators can
still satisfy this safety requirement because their lifetime ensures
that they are never moved or dropped.
@djkoloski djkoloski force-pushed the restict_allocator_impl branch from cdf8314 to 9c24afd Compare February 17, 2022 03:02
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2022
@apiraino
Copy link
Contributor

apiraino commented Mar 24, 2022

I think this is t-libs rather than t-compiler

@rustbot label -T-compiler +t-libs

@rustbot rustbot added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 24, 2022
@yaahc yaahc added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 28, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2022
@yaahc
Copy link
Member

yaahc commented Apr 29, 2022

I'm reassigning this PR because I'm taking a break from the review rotation for a little while. Thank you for your patience.

r? rust-lang/libs-api

@joboet
Copy link
Member

joboet commented Jun 18, 2022

Couldn't the documentation just be updated to clarify that the allocated blocks are only valid for 'a where A: 'a (not sure how to write that in plain English)? That would match the current assumptions in Box::leak and still allow allocating multiple structures with the same allocator, which therefore can be stack-based.

@djkoloski
Copy link
Contributor Author

#94114 has a more extensive change that pursues a similar goal.

@JohnCSimon
Copy link
Member

JohnCSimon commented Jul 24, 2022

#94114 has a more extensive change that pursues a similar goal.

Triage:
@djkoloski what should happen to this PR? Maybe close this because the other one supersedes it?

@djkoloski
Copy link
Contributor Author

This PR is meant as a minimal fix to unsoundness in the changed impl. The other PR is a more extensive change that provides more flexibility, so if we'd like to go forward with #94114 then I can close this one.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 8, 2022
@pitaj
Copy link
Contributor

pitaj commented Apr 30, 2023

Ping from triage. @djkoloski looks like your other PR has more traction, do you want to close this one?

@Amanieu Amanieu mentioned this pull request May 19, 2023
12 tasks
@djkoloski
Copy link
Contributor Author

Note that we did not go forward with the other PR. We should probably focus on merging this and closing the soundness hole.

@joboet
Copy link
Member

joboet commented Dec 5, 2023

I'm nominating this for team discussion. The question is whether to maintain the current guarantee that

Memory blocks returned from an allocator that are currently allocated [...] retain their validity while [...] at least one of the instance and all of its clones has not been dropped

which would require allocators to have 'static lifetime, or whether to relax this guarantee to apply only to 'static allocators (this is what is currently done in practice, see e.g. Box::leak) and add some lifetime constraints otherwise.

@rustbot label +I-libs-api-nominated

@rustbot rustbot added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Dec 5, 2023
@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Dec 19, 2023
@Amanieu
Copy link
Member

Amanieu commented Feb 3, 2024

Closing in favor of #118890.

@Amanieu Amanieu closed this Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.