borrowed referent of a `&T` sometimes incorrectly allowed

nikomatsakis commented 7 years ago

@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 commented 7 years ago

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

nikomatsakis commented 7 years ago

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

arielb1 commented 7 years ago

How bad?

nikomatsakis commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

arielb1 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@aidanhs

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

nikomatsakis commented 7 years ago

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

Ms2ger commented 7 years ago

CC @jorendorff

bstrie commented 7 years ago

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 commented 7 years ago

@bstrie

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

No, some things would still break.

bstrie commented 7 years ago

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

arielb1 commented 7 years ago

@bstrie

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

bstrie commented 7 years ago

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: https://github.com/rust-lang/rfcs/pull/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 commented 7 years ago

@bstrie

There's no point prioritizing this bug. It will be fixed by 2-phase borrows, and not beforehand. And I'm quite sure we decided to wait on 2-phase borrows until MIR borrowck.

briansmith commented 7 years ago

@arielb1 How can l-unsound bugs be anything other than high priority? As a matter of policy it shouldn't take over a year (or, IMO, more than one release) for l-unsound bugs to be fixed.

bstrie commented 7 years ago

How can l-unsound bugs be anything other than high priority?

@briansmith:

Some soundness bugs exist only via unstable features on nightly (e.g. https://github.com/rust-lang/rust/issues/44367)
Not all soundness bugs affect all platforms (https://github.com/rust-lang/rust/issues/43241)
Some soundness bugs are vague and unreproducible (https://github.com/rust-lang/rust/issues/18072)
Some soundness bugs are because of LLVM bugs that people are loathe to work around rather than waiting on LLVM to properly address the issue (https://github.com/rust-lang/rust/issues/28728)
Not all soundness bugs present backcompat hazards (e.g. https://github.com/rust-lang/rust/issues/10389)
Some soundness bugs are simply contrived enough that nobody has prioritized plugging them yet (https://github.com/rust-lang/rust/issues/41799)

(Many of the open soundness bugs fall into multiple of the above categories, too.)

That said, I think that this particular bug falls into none of those categories, hence my earlier statement that this "really does concern me the most of all the open and in-stable soundness bugs". However, P-high tends to mean "there is someone actively taking steps to address this", which in this case is technically true (someone's working on MIR borrowck, which is a prereq for two-phase borrows, which is a prereq for this bug) but I get the impression that arielb1 simply disagrees that completing those orthogonal tasks count as "working on" this bug.

As a matter of policy it shouldn't take over a year (or, IMO, more than one release) for l-unsound bugs to be fixed.

I'm hopeful that such a policy will eventually manifest, once we have both MIR borrowck and Chalk (most of the interesting open soundness bugs are waiting on these two initiatives, AFAICT: https://github.com/rust-lang/rust/issues/27282, https://github.com/rust-lang/rust/issues/27868, https://github.com/rust-lang/rust/issues/29723, https://github.com/rust-lang/rust/issues/25860, https://github.com/rust-lang/rust/issues/44454, https://github.com/rust-lang/rust/issues/27675, plus this issue itself), especially now that Rust code is about to be in stable Firefox. But especially with LLVM bugs we don't always have the combination of manpower and LLVM expertise to make this a strict rule.

arielb1 commented 7 years ago

As a matter of policy it shouldn't take over a year (or, IMO, more than one release) for l-unsound bugs to be fixed.

This is generally the case. Except that rustc's borrowck is fatally broken in several ways, and we're working on a replacement.

erickt commented 7 years ago

FYI this bug was exploited by @aidanhs in his entry for the 2016 underhanded rust contest.

Ixrec commented 7 years ago

Is this one of the borrow checking soundness bugs that's likely to get fixed "for free" or close-to-free by the move to MIR borrowck, two-phase borrows and/or NLL?

bstrie commented 7 years ago

@Ixrec It seems that the plan is that NLL will get us most of the way towards fixing this bug.

Manishearth commented 7 years ago

Given that NLL happened, where are we on this bug?

arielb1 commented 7 years ago

@Manishearth

MIR borrowck doesn't have this bug. When it will be enabled by default, this bug will be fixed.

nikomatsakis commented 6 years ago

I propose that we add a test file (with #![feature(nll)]) documenting that this is fixed, so I've labeled this as E-needstest.

nikomatsakis commented 6 years ago

The following test correctly errors with #![feature(nll)] but passes without it.

aidanhs commented 6 years ago

Per https://github.com/rust-lang/rust/issues/46557#issuecomment-357762606: 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!).

nikomatsakis commented 6 years ago

@aidanhs and I discussed. Since there is a test and this is fixed in MIR borrow check, closing as duplicate of https://github.com/rust-lang/rust/issues/47366.

nikomatsakis commented 6 years ago

Er, perhaps that was premature. =)

pnkfelix commented 6 years ago

@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 commented 6 years ago

Ah according to https://github.com/rust-lang/rust/issues/46557#issuecomment-358652258 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 commented 6 years ago

@pnkfelix yeah

pnkfelix commented 5 years ago

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.

rust-lang / rust

borrowed referent of a `&T` sometimes incorrectly allowed #38899