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

borrowed referent of a &T sometimes incorrectly allowed #38899

Closed
nikomatsakis opened this issue Jan 7, 2017 · 38 comments · Fixed by #64221
Closed

borrowed referent of a &T sometimes incorrectly allowed #38899

nikomatsakis opened this issue Jan 7, 2017 · 38 comments · Fixed by #64221
Labels
A-borrow-checker Area: The borrow checker A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. fixed-by-NLL Bugs fixed, but only when NLL is enabled. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 7, 2017

@jorendorf asks on the users forum about a curious discrepancy around fields. It seems that implicit borrows sometimes seem to get overlooked in the borrow checker. This seems like a kind of bad bug, though it's exact scope is unclear until we investigate a bit more.

Here is a variant of @jorendorf's example which is pretty clearly wrong. Here, the block variable is mutably borrowed into x, so it should not be accessible via let p:

#![allow(dead_code)]
pub struct Block<'a> {
    current: &'a u8,
    unrelated: &'a u8,
}

fn bump<'a>(mut block: &mut Block<'a>) {
    let x = &mut block;
    let p: &'a u8 = &*block.current;
	// (use `x` and `p` so enabling NLL doesn't assign overly short lifetimes)
	drop(x);
	drop(p);
}

fn main() {}

I'm guessing that the problem has to do with the logic around borrowing the referent of an &T (in this case, we are borrowing *block.current). In particular, we deem that to be "safe" for the scope of 'a because the data is independently guaranteed to be valid that long (this is reasonable). But we still need to validate that block.current can be (instantaneously) read. It seems we are not doing that. But this is all a hypothesis: I've not dug into the code to validate it.

@nikomatsakis nikomatsakis added A-borrow-checker Area: The borrow checker I-nominated I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 7, 2017
@nikomatsakis nikomatsakis changed the title implicit coercions sometimes incorrectly allow reads from borrowed fields borrowed referent of a &T sometimes incorrectly allowed Jan 7, 2017
@nikomatsakis
Copy link
Contributor Author

I have a fix locally. I'm going to try and evaluate the impact.

@nikomatsakis
Copy link
Contributor Author

This is gonna' be bad unless we address the problem that makes vec.push(vec.len()) an error.

@arielb1
Copy link
Contributor

arielb1 commented Jan 8, 2017

How bad?

@nikomatsakis
Copy link
Contributor Author

@arielb1 I don't know. I'm working through the bootstrap process now, then I'll try to do some sort of crater run to get an idea.

As an aside, this bug dates back to Rust 1.0.0 from what I can tell.

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jan 9, 2017
The old code incorrectly assumed that the borrow would always generate a
loan. In that case, we will check if that loan conflicts with other
loans and hence get an error if the path we borrowed is mutably
borrowed. But this is not true if we are borrowing the referent of an
`&T` reference.

Fixes rust-lang#38899.
@nikomatsakis
Copy link
Contributor Author

Branch in my repo is nikomatsakis/issue-38899 btw. I did a crater run which showed 160 root regressions: https://gist.github.com/nikomatsakis/9279f3023ecbec3d1f09730671c7884b

@nikomatsakis
Copy link
Contributor Author

I don't know what percentage of those would be fixed by the method call temporary thing. Certainly not all (I just opened one at random and found it would not have been).

@aidanhs
Copy link
Member

aidanhs commented Jan 13, 2017

Not sure why ayzim is listed as a root regression when the log shows the culprit is lazy_static (same is true of a few other crates I clicked at random).

@nikomatsakis
Copy link
Contributor Author

@aidanhs yeah, it seems like it gets a bit goofy sometimes in that respect.

@arielb1
Copy link
Contributor

arielb1 commented Jan 14, 2017

@nikomatsakis

If only say 20% of these examples are fixed by method call temporaries, I don't see why we shouldn't do the normal lint -> hard error sequence here.

Sure it's sad to break basically every non-trivial rust project ever, but method call temporaries won't help/

@nikomatsakis
Copy link
Contributor Author

@arielb1 it seems worth trying to investigate a bit more deeply. However, the experience in rustc was quite different -- the majority of errors were fixed. I think I had only one problem (out of ... 10 or so?) that was fixed a different way.

@arielb1
Copy link
Contributor

arielb1 commented Jan 18, 2017

The lazy_static example looks like it's tracking a borrow of the dereference of an unsafe pointer *self.0 for some reason - that's a false positive.

In rustc, it seems that:
2 error can't be fixed by multiphase borrows (rustc_resolve::macros, getopts), of them the rustc_resolve::macros issue is a "hard-to-fix" false positive and the getopts fix is a "technically UB" case.
19 errors are of the foo.mutate(&*foo.field) kind.
1 errors will be fixed by making reborrows multiphase, but will not be fixed by changing order of evaluation (rustc_typeck::check::upvar)

@aidanhs
Copy link
Member

aidanhs commented Jan 18, 2017

@arielb1 isn't it tracking the mutable borrow of the pointer itself rather than its dereference? Which then conflicts with dereferencing the pointer later on.

@arielb1
Copy link
Contributor

arielb1 commented Jan 18, 2017

@aidanhs

After looking at it again, sure. It's just another case of #6393.

@nikomatsakis
Copy link
Contributor Author

@arielb1 right; so 19/23 (80%) seems pretty significant.

@Ms2ger
Copy link
Contributor

Ms2ger commented Mar 1, 2017

CC @jorendorff

@bstrie
Copy link
Contributor

bstrie commented Mar 9, 2017

This is gonna' be bad unless we address the problem that makes vec.push(vec.len()) an error.

@nikomatsakis does this imply that NLL would allow us to fix this without breakage?

@nikomatsakis
Copy link
Contributor Author

@bstrie

does this imply that NLL would allow us to fix this without breakage?

No, some things would still break.

@bstrie
Copy link
Contributor

bstrie commented Jun 20, 2017

@nikomatsakis I see that you removed your nomination from this bug without assigning it a priority, was that intentional?

@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 26, 2017
@arielb1
Copy link
Contributor

arielb1 commented Aug 14, 2017

@bstrie

This is "waiting for 2φB", which is waiting for an RFC + MIR borrowck.

@bstrie bstrie added the P-high High priority label Sep 14, 2017
@bstrie
Copy link
Contributor

bstrie commented Sep 14, 2017

This is gonna' be bad unless we address the problem that makes vec.push(vec.len()) an error.

The "Enable nested method calls" RFC (a.k.a. "two-phase borrows", which is what I presume @arielb1 meant by "2φB") was recently accepted: rust-lang/rfcs#2025 .

Even considering MIR borrowck as a blocker, given that that RFC seems to be a high priority for the forthcoming three-month impl period, is it safe to assume that we'll have something in nightly by December with which we can start doing cargobomb runs to determine exactly how painful this bug will be to fix?

Possibly overstepping my bounds my assigning this a P-high, but it really does concern me the most of all the open and in-stable soundness bugs, and I have a longstanding pet peeve concerning unprioritized soundness bugs. :P

@arielb1 arielb1 added the P-medium Medium priority label Sep 14, 2017
kennytm added a commit to kennytm/rust that referenced this issue Jan 15, 2018
Add NLL tests for rust-lang#46557 and rust-lang#38899

This adapts the sample code from the two issues into test code.

Closes rust-lang#46557
Closes rust-lang#38899

r? @nikomatsakis
@aidanhs
Copy link
Member

aidanhs commented Jan 15, 2018

Per #46557 (comment): I think it's reasonable to say this is a P-medium issue until the fix is in master and enabled by default (I'm open to disagreement though!).

@aidanhs aidanhs reopened this Jan 15, 2018
@nikomatsakis
Copy link
Contributor Author

@aidanhs and I discussed. Since there is a test and this is fixed in MIR borrow check, closing as duplicate of #47366.

@nikomatsakis
Copy link
Contributor Author

Er, perhaps that was premature. =)

@nikomatsakis nikomatsakis reopened this Jan 16, 2018
@pnkfelix
Copy link
Member

@nikomatsakis so wait, what is the policy then for #47366? Are we closing issues that are fixed by #![feature(nll)] as long as they have a test? Or are we just downgrading them to P-medium?

(And either way, should this bug be linked from checklist in #47366 description?)

@pnkfelix
Copy link
Member

Ah according to #46557 (comment) the policy is that we're removing WG-compiler-nll label from such issues? That seems ... reasonable. Since then the work queue (either in-progress or to-do) should be identifiable from that label?

@nikomatsakis
Copy link
Contributor Author

@pnkfelix yeah

@pnkfelix
Copy link
Member

NLL (migrate mode) is enabled in all editions as of PR #59114.

The only policy question is that, under migrate mode, we only emit a warning on this (unsound) test case. Therefore, I am not 100% sure whether we should close this until that warning has been turned into a hard-error under our (still in development) future-compatibility lint policy.

Centril added a commit to Centril/rust that referenced this issue Jul 28, 2019
Centril added a commit to Centril/rust that referenced this issue Jul 30, 2019
Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019
…hewjasper

 Rust 2015: No longer downgrade NLL errors

As per decision on a language team meeting as described in rust-lang#63565 (comment), in Rust 2015, we refuse to downgrade NLL errors, that AST borrowck accepts, into warnings and keep them as hard errors.

The remaining work to throw out AST borrowck and adjust some tests still remains after this PR.

Fixes rust-lang#38899
Fixes rust-lang#53432
Fixes rust-lang#45157
Fixes rust-lang#31567
Fixes rust-lang#27868
Fixes rust-lang#47366

r? @matthewjasper
Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019
…hewjasper

 Rust 2015: No longer downgrade NLL errors

As per decision on a language team meeting as described in rust-lang#63565 (comment), in Rust 2015, we refuse to downgrade NLL errors, that AST borrowck accepts, into warnings and keep them as hard errors.

The remaining work to throw out AST borrowck and adjust some tests still remains after this PR.

Fixes rust-lang#38899
Fixes rust-lang#53432
Fixes rust-lang#45157
Fixes rust-lang#31567
Fixes rust-lang#27868
Fixes rust-lang#47366

r? @matthewjasper
Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019
…hewjasper

 Rust 2015: No longer downgrade NLL errors

As per decision on a language team meeting as described in rust-lang#63565 (comment), in Rust 2015, we refuse to downgrade NLL errors, that AST borrowck accepts, into warnings and keep them as hard errors.

The remaining work to throw out AST borrowck and adjust some tests still remains after this PR.

Fixes rust-lang#38899
Fixes rust-lang#53432
Fixes rust-lang#45157
Fixes rust-lang#31567
Fixes rust-lang#27868
Fixes rust-lang#47366

r? @matthewjasper
@bors bors closed this as completed in 3b5fbb6 Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. fixed-by-NLL Bugs fixed, but only when NLL is enabled. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.