Disallow subtyping between T and U in T: Unsize<U>. by eddyb · Pull Request #40319 · rust-lang/rust

eddyb · 2017-03-07T14:34:59Z

Because &mut T can be coerced to &mut U, T and U must be unified invariantly. Fixes#40288.
E.g. coercing &mut [&'a X; N] to &mut [&'b X] must require 'a be equal to 'b, otherwise you can convert between &'a X and &'b X (in either direction), potentially unsoundly lengthening lifetimes.

Subtyping here was introduced with Unsize in #24619 (landed in 1.1, original PR is #23785).

rust-highfive · 2017-03-07T14:35:12Z

r? @nrc

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

eddyb · 2017-03-07T14:35:25Z

(starting crater run now)

nikomatsakis · 2017-03-07T14:38:29Z

r? @nikomatsakis

nikomatsakis · 2017-03-07T14:39:29Z

The code looks good to me, curious to hear the results of the crater run. I'll try to do a quick skim through the code to convince myself that these are all the relevant cases though.

arielb1 · 2017-03-08T14:55:25Z

Don't you need this also on the raw ptr path? better to subtype source as one unit.

Subtype first, then resolve the variable, then reborrow from &mut and/or to raw pointers?

Aaaaah, that would also keep Rc working! I'll try to do it that way and add more testcases.

eddyb · 2017-03-08T20:27:18Z

Crater report shows 6 3 root regressions:

~~valico (failure)~~
mime_multipart (failure)
- 2 dependents (formdata and email-format)
  - email-format doesn't use it though?
- 0.3.5: 1 regression (pencil -> formdata)
- 0.4.0: 2 regressions (formdata and sharp_pencil -> formdata)
~~intervallum (failure)~~
~~crayon (failure)~~
multipart (failure)
- ~~similar/same crate as mime_multipart?~~
- only 0.9.* is affected (1 dependent, which regressed, tg_botapi)
yamlette (failure)

EDIT: some were false positives, see #40319 (comment)

Stebalien · 2017-03-08T21:10:54Z

similar/same crate as mime_multipart?

Different crates but the issue in both stems from the httparse bug (the original bug linked in the first post). Those are both legitimate UB (I can't tell with the other bugs).

eddyb · 2017-03-08T21:16:42Z

@Stebalien I hadn't even realized that was the original report! Alright, I'm more confident now.
~~I've only looked in depth at crayon (AFAICT there's some lifetime parameter order confusion).~~

Assuming only {mime_},multipart~~and valico~~ have any dependents, we should be able to release fixed patch versions of them (if it's possible at all without changing the API), and that will be that.

eddyb · 2017-03-08T21:51:29Z

So the crates that other crates depend on are:

@mikedilger's mime_multipart (needs fix and 0.3.6 and 0.4.1 released)
@abonander's multipart (needs fix and 0.9.1 released)

Everything else we can break without having a cascade of regressions.
I may attempt making PRs to some of the crates, but they're pretty tough to understand.

abonander · 2017-03-08T21:53:50Z

I've already got this fixed in multipart 0.10.1 so it's trivial to backport.

eddyb · 2017-03-08T21:56:27Z

@abonander (and everyone else) Ping me when you have the fix on a branch somewhere, or a patch in a gist, etc. if you want a confirmation that it indeed compiles with the changes in this PR.

abonander · 2017-03-08T22:11:04Z

@eddyb branch 0.9 has the fix backported. I'll publish once you confirm.

eddyb · 2017-03-08T22:13:58Z

@abonander It compiles 👍

abonander · 2017-03-08T22:20:29Z

@eddyb Published.

mikedilger · 2017-03-08T23:01:10Z

@eddyb Not sure I understand the bug very well, but I hope a simple sub-scope makes the borrow issue go away. Let me know if thats right or if I need some other fix. https://github.com/mikedilger/mime-multipart/tree/rust40288

eddyb · 2017-03-08T23:16:28Z

@mikedilger Almost! I was getting confused by not knowing what is actually keeping the borrow alive.
Turns out your change is in the right direction, you just have to also move header_memory in that block.
That should work on both 0.4.0 (master) and 0.3.5 (b118d606feb62c15d0dbd4e3280597a02e004190).

mikedilger · 2017-03-08T23:26:40Z

@eddyb Ok, I (evilly) force-pushed https://github.com/mikedilger/mime-multipart/tree/rust40288 and also created https://github.com/mikedilger/mime-multipart/tree/rust40288_03 for the 0.3 branch. Once they are confirmed to work I'll pull them in and publish new versions.

eddyb · 2017-03-08T23:34:17Z

@mikedilger Those do indeed work, thank you!

mikedilger · 2017-03-08T23:59:22Z

@eddyb Published.

eddyb · 2017-03-09T00:46:07Z

Not needing use_lub felt wrong, and it is! Reordering these twomatch arms "fixes" the false positive, but both orders should work identically.
I'm preparing an improved version that should not have this problem.

Rollup of 38 pull requests - Successful merges: #39202, #39820, #39918, #39921, #40092, #40146, #40199, #40225, #40239, #40257, #40259, #40261, #40277, #40278, #40287, #40297, #40311, #40315, #40319, #40324, #40336, #40340, #40344, #40345, #40367, #40369, #40372, #40373, #40379, #40385, #40386, #40389, #40400, #40404, #40410, #40422, #40423, #40424 - Failed merges: #40220, #40329, #40426

…r=nikomatsakis Disallow subtyping between T and U in T: Unsize<U>. Because `&mut T` can be coerced to `&mut U`, `T` and `U` must be unified invariantly. Fixesrust-lang#40288. E.g. coercing `&mut [&'a X; N]` to `&mut [&'b X]` must require `'a` be equal to `'b`, otherwise you can convert between `&'a X` and `&'b X` (in either direction), potentially unsoundly lengthening lifetimes. Subtyping here was introduced with `Unsize` in rust-lang#24619 (landed in 1.1, original PR is rust-lang#23785).

Rollup of 13 pull requests - Successful merges: #40146, #40299, #40315, #40319, #40344, #40345, #40367, #40372, #40373, #40385, #40400, #40404, #40431 - Failed merges:

…r=nikomatsakis Disallow subtyping between T and U in T: Unsize<U>. Because `&mut T` can be coerced to `&mut U`, `T` and `U` must be unified invariantly. Fixesrust-lang#40288. E.g. coercing `&mut [&'a X; N]` to `&mut [&'b X]` must require `'a` be equal to `'b`, otherwise you can convert between `&'a X` and `&'b X` (in either direction), potentially unsoundly lengthening lifetimes. Subtyping here was introduced with `Unsize` in rust-lang#24619 (landed in 1.1, original PR is rust-lang#23785).

Rollup of 12 pull requests - Successful merges: #40146, #40299, #40315, #40319, #40344, #40345, #40372, #40373, #40400, #40404, #40419, #40431 - Failed merges:

rust-highfive assigned nrc Mar 7, 2017

eddyb requested a review from nikomatsakis March 7, 2017 14:35

eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Mar 7, 2017

rust-highfive assigned nikomatsakis and unassigned nrc Mar 7, 2017

Disallow subtyping between T and U in T: Unsize<U>.
3198904

eddyb force-pushed the it's-"unsize"-not-"unsound" branch from 152f1c7 to 608d080Compare March 8, 2017 12:44

arielb1 reviewed Mar 8, 2017
View reviewed changes

eddyb force-pushed the it's-"unsize"-not-"unsound" branch from 608d080 to 721f5b8Compare March 8, 2017 19:04

eddyb commented Mar 9, 2017
View reviewed changes

eddyb mentioned this pull request Mar 9, 2017
Ref suggestion #37658
Merged

Overhaul coercion to use the lazy InferOk obligations passing.
74bc7fd

alexcrichton mentioned this pull request Mar 10, 2017
Rollup of 38 pull requests #40432
Merged

arielb1 mentioned this pull request Mar 11, 2017
Rollup of 13 pull requests #40440
Closed

bors added a commit that referenced this pull request Mar 11, 2017
Auto merge of#40440- arielb1:rollup, r=arielb1
9ca8831
Rollup of 13 pull requests - Successful merges: #40146, #40299, #40315, #40319, #40344, #40345, #40367, #40372, #40373, #40385, #40400, #40404, #40431 - Failed merges:

arielb1 mentioned this pull request Mar 11, 2017
Rollup of 12 pull requests #40446
Merged

bors added a commit that referenced this pull request Mar 12, 2017
Auto merge of#40446- arielb1:rollup, r=alexcrichton
744e696
Rollup of 12 pull requests - Successful merges: #40146, #40299, #40315, #40319, #40344, #40345, #40372, #40373, #40400, #40404, #40419, #40431 - Failed merges:

bors merged commit cfb41ae into rust-lang:masterMar 12, 2017

eddyb deleted the it's-"unsize"-not-"unsound" branch March 12, 2017 09:36

alexcrichton mentioned this pull request Mar 15, 2017
A recent change to beta & nightly channels broke the tokio-core build #40551
Closed

nikomatsakis mentioned this pull request Mar 31, 2017
Rust 1.17 regression - yamlette 0.0.2 #40964
Closed

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 4, 2017

arielb1 mentioned this pull request Apr 4, 2017
Rust 1.18 regression - mime_multipart-0.3.5 #40958
Closed

arielb1 mentioned this pull request Apr 13, 2017
[beta] backport fix for #40951 #41269
Closed

djzin mentioned this pull request Apr 28, 2017
1.17.0 regression: loop mutable vs immutable alias. #41599
Closed

qnighy added a commit to qnighy/rust that referenced this pull request Jun 29, 2017
Correct comments about unsizing regardingrust-lang#40319.
1fe43a5

qnighy added a commit to qnighy/rust that referenced this pull request Jun 29, 2017
Correct comments about unsizing regardingrust-lang#40319.
0812b33

qnighy added a commit to qnighy/rust that referenced this pull request Jun 29, 2017
Correct comments about unsizing regardingrust-lang#40319.
94862c6

varkor mentioned this pull request Apr 29, 2020
Unsoundness due to variance of trait objects WRT associated types #71550
Closed

Uh oh!

Uh oh!

Hidden character warning

Conversation

eddyb commented Mar 7, 2017

Uh oh!

rust-highfive commented Mar 7, 2017

Uh oh!

eddyb commented Mar 7, 2017

Uh oh!

nikomatsakis commented Mar 7, 2017

Uh oh!

nikomatsakis commented Mar 7, 2017

Uh oh!

arielb1Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

eddybMar 8, 2017

Choose a reason for hiding this comment

Uh oh!

eddybMar 8, 2017

Choose a reason for hiding this comment

Uh oh!

eddyb commented Mar 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Stebalien commented Mar 8, 2017

Uh oh!

eddyb commented Mar 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb commented Mar 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abonander commented Mar 8, 2017

Uh oh!

eddyb commented Mar 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abonander commented Mar 8, 2017

Uh oh!

eddyb commented Mar 8, 2017

Uh oh!

abonander commented Mar 8, 2017

Uh oh!

mikedilger commented Mar 8, 2017

Uh oh!

eddyb commented Mar 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikedilger commented Mar 8, 2017

Uh oh!

eddyb commented Mar 8, 2017

Uh oh!

mikedilger commented Mar 8, 2017

Uh oh!

eddybMar 9, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

eddyb commented Mar 8, 2017•
edited
Loading

eddyb commented Mar 8, 2017•
edited
Loading

eddyb commented Mar 8, 2017•
edited
Loading

eddyb commented Mar 8, 2017•
edited
Loading

eddyb commented Mar 8, 2017•
edited
Loading

eddybMar 9, 2017•
edited
Loading