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

Use deref target in Pin trait implementations #67039

Merged
merged 4 commits into from Dec 10, 2019

Conversation

@xfix
Copy link
Contributor

xfix commented Dec 4, 2019

Using deref target instead of pointer itself avoids providing access to &Rc<T> for malicious implementations, which would allow calling Rc::get_mut.

This is a breaking change necessary due to unsoundness, however the impact of it should be minimal.

This only fixes the issue with malicious PartialEq implementations, other Pin soundness issues are still here.

See https://internals.rust-lang.org/t/unsoundness-in-pin/11311/73 for more details.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 4, 2019

r? @joshtriplett

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

@jonas-schievink

This comment has been minimized.

Copy link
Member

jonas-schievink commented Dec 4, 2019

Should we crater this?

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 4, 2019

⌛️ Trying commit e537bf8 with merge 70e8b7b...

bors added a commit that referenced this pull request Dec 4, 2019
Use deref target in Pin trait implementations

Using deref target instead of pointer itself avoids providing access to `&Rc<T>` for malicious implementations, which would allow calling `Rc::get_mut`.

This is a breaking change necessary due to unsoundness, however the impact of it should be minimal.

This only fixes the issue with malicious `PartialEq` implementations, other `Pin` soundness issues are still here.

See <https://internals.rust-lang.org/t/unsoundness-in-pin/11311/73> for more details.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 5, 2019

☀️ Try build successful - checks-azure
Build commit: 70e8b7b (70e8b7bf8f1daf01a5793147964b5d1eb7f32585)

@mark-i-m

This comment has been minimized.

Copy link
Member

mark-i-m commented Dec 5, 2019

@comex

This comment has been minimized.

Copy link
Contributor

comex commented Dec 5, 2019

Seems good to me. I’d be interested to see crater results.

src/libcore/pin.rs Outdated Show resolved Hide resolved
src/libcore/pin.rs Outdated Show resolved Hide resolved
src/libcore/pin.rs Outdated Show resolved Hide resolved
@xfix xfix requested review from Centril and RalfJung Dec 5, 2019
src/libcore/pin.rs Outdated Show resolved Hide resolved
src/libcore/pin.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 5, 2019

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-12-05T09:53:03.1860311Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-05T09:53:03.2040215Z ##[command]git config gc.auto 0
2019-12-05T09:53:03.2121138Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-05T09:53:03.2177017Z ##[command]git config --get-all http.proxy
2019-12-05T09:53:03.2292094Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67039/merge:refs/remotes/pull/67039/merge
---
2019-12-05T09:57:52.3173030Z Checking rustdoc artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-05T09:57:52.3189641Z Checking std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-05T09:57:52.6677370Z    Compiling cc v1.0.47
2019-12-05T09:57:52.6677765Z     Checking core v0.0.0 (/checkout/src/libcore)
2019-12-05T09:57:59.6086923Z error[E0599]: no method named `partial_cmp` found for type `<P as ops::deref::Deref>::Target` in the current scope
2019-12-05T09:57:59.6088193Z    --> src/libcore/pin.rs:437:18
2019-12-05T09:57:59.6088882Z     |
2019-12-05T09:57:59.6089385Z 437 |         (**self).partial_cmp(other)
2019-12-05T09:57:59.6090335Z     |                  ^^^^^^^^^^^ method not found in `<P as ops::deref::Deref>::Target`
2019-12-05T09:57:59.6091555Z     = note: the method `partial_cmp` exists but the following trait bounds were not satisfied:
2019-12-05T09:57:59.6091555Z     = note: the method `partial_cmp` exists but the following trait bounds were not satisfied:
2019-12-05T09:57:59.6092033Z             `&mut <P as ops::deref::Deref>::Target : iter::traits::iterator::Iterator`
2019-12-05T09:57:59.6092525Z     = help: items from traits can only be used if the trait is implemented and in scope
2019-12-05T09:57:59.6093033Z     = note: the following traits define an item `partial_cmp`, perhaps you need to implement one of them:
2019-12-05T09:57:59.6093489Z             candidate #1: `cmp::PartialOrd`
2019-12-05T09:57:59.6093916Z             candidate #2: `iter::traits::iterator::Iterator`
2019-12-05T09:57:59.6094140Z 
2019-12-05T09:57:59.6137917Z error[E0369]: binary operation `<` cannot be applied to type `<P as ops::deref::Deref>::Target`
2019-12-05T09:57:59.6138530Z    --> src/libcore/pin.rs:441:16
2019-12-05T09:57:59.6138995Z     |
2019-12-05T09:57:59.6139452Z 441 |         **self < **other
2019-12-05T09:57:59.6140160Z     |         ------ ^ ------- <Q as ops::deref::Deref>::Target
2019-12-05T09:57:59.6140634Z     |         |
2019-12-05T09:57:59.6141089Z     |         <P as ops::deref::Deref>::Target
2019-12-05T09:57:59.6141520Z     |
2019-12-05T09:57:59.6142493Z     = note: an implementation of `std::cmp::PartialOrd` might be missing for `<P as ops::deref::Deref>::Target`
2019-12-05T09:57:59.6142728Z 
2019-12-05T09:57:59.6185436Z error[E0369]: binary operation `<=` cannot be applied to type `<P as ops::deref::Deref>::Target`
2019-12-05T09:57:59.6189681Z    --> src/libcore/pin.rs:445:16
2019-12-05T09:57:59.6190274Z     |
2019-12-05T09:57:59.6190741Z 445 |         **self <= **other
2019-12-05T09:57:59.6192199Z     |         ------ ^^ ------- <Q as ops::deref::Deref>::Target
2019-12-05T09:57:59.6193487Z     |         |
2019-12-05T09:57:59.6193981Z     |         <P as ops::deref::Deref>::Target
2019-12-05T09:57:59.6194401Z     |
2019-12-05T09:57:59.6194887Z     = note: an implementation of `std::cmp::PartialOrd` might be missing for `<P as ops::deref::Deref>::Target`
2019-12-05T09:57:59.6195386Z 
2019-12-05T09:57:59.6274654Z error[E0369]: binary operation `>` cannot be applied to type `<P as ops::deref::Deref>::Target`
2019-12-05T09:57:59.6275499Z    --> src/libcore/pin.rs:449:16
2019-12-05T09:57:59.6275882Z     |
2019-12-05T09:57:59.6276275Z 449 |         **self > **other
2019-12-05T09:57:59.6276926Z     |         ------ ^ ------- <Q as ops::deref::Deref>::Target
2019-12-05T09:57:59.6277688Z     |         |
2019-12-05T09:57:59.6281318Z     |         <P as ops::deref::Deref>::Target
2019-12-05T09:57:59.6281921Z     |
2019-12-05T09:57:59.6282344Z     = note: an implementation of `std::cmp::PartialOrd` might be missing for `<P as ops::deref::Deref>::Target`
2019-12-05T09:57:59.6282490Z 
2019-12-05T09:57:59.6286578Z error[E0369]: binary operation `>=` cannot be applied to type `<P as ops::deref::Deref>::Target`
2019-12-05T09:57:59.6287185Z    --> src/libcore/pin.rs:453:16
2019-12-05T09:57:59.6287518Z     |
2019-12-05T09:57:59.6288036Z 453 |         **self >= **other
2019-12-05T09:57:59.6288705Z     |         ------ ^^ ------- <Q as ops::deref::Deref>::Target
2019-12-05T09:57:59.6289085Z     |         |
2019-12-05T09:57:59.6289450Z     |         <P as ops::deref::Deref>::Target
2019-12-05T09:57:59.6289754Z     |
2019-12-05T09:57:59.6290181Z     = note: an implementation of `std::cmp::PartialOrd` might be missing for `<P as ops::deref::Deref>::Target`
2019-12-05T09:58:00.3020793Z    Compiling build_helper v0.1.0 (/checkout/src/build_helper)
2019-12-05T09:58:01.7475426Z    Compiling libc v0.2.64
2019-12-05T09:58:02.4883004Z error: aborting due to 5 previous errors
2019-12-05T09:58:02.4883196Z 
---
2019-12-05T09:58:02.5960635Z   local time: Thu Dec  5 09:58:02 UTC 2019
2019-12-05T09:58:02.8851231Z   network time: Thu, 05 Dec 2019 09:58:02 GMT
2019-12-05T09:58:02.8854952Z == end clock drift check ==
2019-12-05T09:58:16.0795886Z 
2019-12-05T09:58:16.0887342Z ##[error]Bash exited with code '1'.
2019-12-05T09:58:16.0911851Z ##[section]Starting: Checkout
2019-12-05T09:58:16.0913422Z ==============================================================================
2019-12-05T09:58:16.0913471Z Task         : Get sources
2019-12-05T09:58:16.0913509Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Dec 5, 2019

It would be good to add UI tests to this PR so that we have checks ensuring that the problematic pattern this is meant to catch cannot happen in the future.

@xfix xfix force-pushed the xfix:manually-implement-pin-traits branch 2 times, most recently from 8bee4ff to efaba2d Dec 5, 2019
@xfix

This comment has been minimized.

Copy link
Contributor Author

xfix commented Dec 5, 2019

@Centril Added UI test.

xfix added 3 commits Dec 4, 2019
Using deref target instead of pointer itself avoids providing access to
`&Rc<T>` for malicious implementations, which would allow calling
`Rc::get_mut`.

This is a breaking change necessary due to unsoundness, however
the impact of it should be minimal.

This only fixes the issue with malicious `PartialEq` implementations,
other `Pin` soundness issues are still here.

See <https://internals.rust-lang.org/t/unsoundness-in-pin/11311/73>
for more details.
@xfix xfix force-pushed the xfix:manually-implement-pin-traits branch from efaba2d to 6996ae1 Dec 5, 2019
@xfix

This comment has been minimized.

Copy link
Contributor Author

xfix commented Dec 5, 2019

Also, made amount of commits a bit smaller by merging some commits together.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Dec 6, 2019

big +1. these implementations were just unsound and this is the correct way to implement these traits for these types.

@nikomatsakis nikomatsakis self-assigned this Dec 7, 2019
@xfix xfix requested a review from RalfJung Dec 7, 2019
Copy link
Member

RalfJung left a comment

LGTM.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 9, 2019

@bors r+

I'm going to go ahead and r+ this without a crater run. It seems quite unlikely that we'll see any regressions here, and this is a soundness fix.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 9, 2019

📌 Commit 61d9c00 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 10, 2019

⌛️ Testing commit 61d9c00 with merge 883b6aa...

bors added a commit that referenced this pull request Dec 10, 2019
…akis

Use deref target in Pin trait implementations

Using deref target instead of pointer itself avoids providing access to `&Rc<T>` for malicious implementations, which would allow calling `Rc::get_mut`.

This is a breaking change necessary due to unsoundness, however the impact of it should be minimal.

This only fixes the issue with malicious `PartialEq` implementations, other `Pin` soundness issues are still here.

See <https://internals.rust-lang.org/t/unsoundness-in-pin/11311/73> for more details.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 10, 2019

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 883b6aa to master...

@bors bors added the merged-by-bors label Dec 10, 2019
@bors bors merged commit 61d9c00 into rust-lang:master Dec 10, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr #20191207.27 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.