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.

Sign up for GitHub

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

Jump to bottom

Pin is unsound due to rogue Deref/DerefMut implementations #66544

Closed
RalfJung opened this issue Nov 19, 2019 · 5 comments · Fixed by #68004
Closed

Pin is unsound due to rogue Deref/DerefMut implementations #66544

RalfJung opened this issue Nov 19, 2019 · 5 comments · Fixed by #68004
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

@comex found a soundness bug in Pin. This issue is to track that.

Discussion is happening on IRLO currently, let's try not to fork that.
@RalfJung RalfJung added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Nov 19, 2019
@jonas-schievink jonas-schievink added the C-bug Category: This is a bug. label Nov 19, 2019
@aturon aturon mentioned this issue Nov 19, 2019
Closed
@Centril Centril added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated P-high High priority labels Nov 19, 2019
@eddyb
Copy link
Member

eddyb commented Nov 29, 2019

I don't think people are necessarily seeing this, pinging all labelled teams:
cc @rust-lang/compiler @rust-lang/lang @rust-lang/libs

@oli-obk oli-obk mentioned this issue Nov 30, 2019
Closed
@est31
Copy link
Member

est31 commented Dec 5, 2019 •
edited
Loading

PR with a proposed (partial, thanks @RalfJung) fix: #67039

@RalfJung
Copy link
Member Author

RalfJung commented Dec 5, 2019

Well, a proposed partial fix.

@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Dec 5, 2019 •
edited
Loading

I still think the partial fix is worth it. I don't think we want to resolve this particular issue in any other way. Meanwhile, other soundness issues are more complicated, and can be solved separately from PartialEq issue (my preferred solution would be to prevent DerefMut implementations for &T and Clone implementations for &mut T, but I think this is still being discussed).

@RalfJung
Copy link
Member Author

RalfJung commented Dec 5, 2019

I still think the partial fix is worth it.

Fully agreed, and thanks for preparing that PR! I was just pointing out that the PR will not be sufficient to consider the problem solved.

This was referenced Jan 8, 2020
Merged
Open
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 21, 2020
@Dylan-DPC
Rollup merge of rust-lang#68004 - nikomatsakis:negative-impls, r=varkor
16338b1 
permit negative impls for non-auto traits

This is a prototype impl that extends `impl !Trait` beyond auto traits. It is not integrated with coherence or anything else, and hence only serves to prevent downstream impls (but not to allow downstream crates to rely on the absence of such impls for coherence purposes).

Fixes rust-lang#66544

TODO:

- [x] need a test that you can't rely on negative impls for coherence purposes
- [x] test that negative impls cannot specialize positive ones
- [x] test that positive impls cannot specialize negative ones
- [x] extend negative impl to `Clone` in order to fully fix rust-lang#66544
- [x] and maybe make `CoerceUnsized` unsafe? -- that problem is now split out into rust-lang#68015
- [x] introduce feature flag and prepare a write-up
- [x] improve diagnostics?
@bors bors closed this as completed in b0a63cb Mar 26, 2020
@tlyu tlyu mentioned this issue May 26, 2021
Open
@y86-dev y86-dev mentioned this issue Nov 18, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

permit negative impls for non-auto traits nikomatsakis/rust
7 participants
@eddyb @nikomatsakis @RalfJung @Centril @KamilaBorowska @jonas-schievink @est31