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

Under feature(capture_disjoint_fields), unsafe-checking has false positives under unsafe blocks #85561

Closed
pnkfelix opened this issue May 21, 2021 · 11 comments · Fixed by #85724
Closed
Assignees
Labels
C-bug Category: This is a bug. requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented May 21, 2021

Spawned off of issue #85435

I tried this code on nightly:

#![allow(incomplete_features)] // narrow diagnostic output
#![feature(capture_disjoint_fields)] // need this to witness the bug consistently in rustc's git history.

fn main() {
    let val: u8 = 5;
    let u8_ptr: *const u8 = &val;
    let _closure = || {
        unsafe {
            // Fails compilation with:                                                                                                                                                                                            
            // error[E0133]: dereference of raw pointer is unsafe and                                                                                                                                                             
            //               requires unsafe function or block                                                                                                                                                                    
            let tmp = *u8_ptr;
            tmp

            // Just dereferencing and returning directly compiles fine:                                                                                                                                                           
            // *u8_ptr
        }
    };
}

I expected to see this happen: Code accepted by compiler

Instead, this happened:

error[E0133]: dereference of raw pointer is unsafe and requires unsafe function or block
  --> src/main.rs:12:23
   |
12 |             let tmp = *u8_ptr;
   |                       ^^^^^^^ dereference of raw pointer
   |
   = note: raw pointers may be null, dangling or unaligned; they can violate aliasing rules and cause data races: all of these are undefined behavior

error: aborting due to previous error

For more information about this error, try `rustc --explain E0133`.

Note: This has same root cause as #85435. But, we're going to address #85435 with a targeted short-term fix (namely PR #85564), so that it won't hit stable. So I'm filing this bug to track fixing the root cause.

@pnkfelix pnkfelix added the C-bug Category: This is a bug. label May 21, 2021
pnkfelix added a commit to pnkfelix/rust that referenced this issue May 21, 2021
@pnkfelix
Copy link
Member Author

searched nightlies: from nightly-2021-03-04 to nightly-2021-05-21
regressed nightly: nightly-2021-03-17
searched commits: from 107896c to f5d8117
regressed commit: f5d8117

bisected with cargo-bisect-rustc v0.6.0

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --access github --preserve

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. requires-nightly This issue requires a nightly compiler in some way. labels May 21, 2021
@pnkfelix
Copy link
Member Author

cc @roxelo

@arora-aman
Copy link
Member

arora-aman commented May 21, 2021

So the solution to bug would could be to call the restrict capture precision functions in https://github.com/rust-lang/rust/blob/master/compiler/rustc_typeck/src/check/upvar.rs#L1589-L1593

I think this would not cause problems because:

  • The reason we needed the fake reads was because we were trying to convert a less precise place into a closure capture.
    Eg: let (a, _) = x and we'd try convert x into a closure capture even though x.0 was being captured. So we added fake reads for the entirety of x.
  • In the case of raw ptr dereference eg *(some_place), we are capturing some_place and matching on a more precise *(some_place) in the let. Restricting precision of the fake read shouldn't affect this scenario.

@pnkfelix
Copy link
Member Author

Before we jump into the approach outlined by @arora-aman : we are expecting to move the unsafe-checking from MIR to THIR, according to rust-lang/compiler-team#402 . Won't that shift also resolve this issue?

@arora-aman
Copy link
Member

arora-aman commented May 21, 2021

I'm not aware of that, I'll read through that. But I don't think we should be adding fake reads of raw pointers dereference anyway.

As I understand it, the fake reads are less precise closure captures to facilitate pattern matching (in both let and match) and we shouldn't be capturing raw pointer dereference.

@roxelo
Copy link
Member

roxelo commented May 21, 2021

It doesn't seem like rust-lang/compiler-team#402 is going to be done any time soon (and I am not entirely sure if that will fix the bug or not), wouldn't it be better for the time being to attempt @arora-aman's suggestion? It should be a quick fix

@LeSeulArtichaut
Copy link
Contributor

We are expecting to move the unsafe-checking from MIR to THIR, according to rust-lang/compiler-team#402 . Won't that shift also resolve this issue?

Since yesterday (#85555), dereferences of raw pointers is implemented in the THIR unsafety checker (gated under -Z thir-unsafeck), and it correctly does not report an error here.

It doesn't seem like rust-lang/compiler-team#402 is going to be done any time soon

I'm not sure how much time it will take to make the THIR unsafety checker the default once it is fully implemented, but the implementation itself is going rather fast (see rust-lang/project-thir-unsafeck#7).

@nikomatsakis
Copy link
Contributor

@arora-aman @roxelo can you spell out a bit more what is happening? What fake reads do we introduce?

@arora-aman
Copy link
Member

arora-aman commented May 24, 2021

So when we have something like

let raw_ptr: *i32;
let c = || { 
    let x  = *raw_ptr;
    ....
};

The MIR that will be generated will be something like

FakeRead(ForLet(span), *raw_ptr)  // This deref is unsafe
_X = create the closure ... 

The reason we have the fake read is to test that the right hand side of the let (and in other examples scrutinee of a match) have been initialized. Since the RHS of let/match scrutinee can be less precise than what a closure captures, we do a FakeRead them outside the closure so as to not capture them for the initialization test.

This means it's fine to do FakeRead an even less precise place, eg. in case of raw pointers don't FakeRead the deref projection.

So my proposal is to apply all the restrictions on closure captures around unsafe code (deref of raw ptr, reference into a repr packed struct) to the Fake Reads that are introduced for let/match on upvars in closure.

Updated MIR would then be:

FakeRead(ForLet(span), raw_ptr)  // Note: deref omitted
_X = create the closure 

@nikomatsakis
Copy link
Contributor

@rustbot assign @roxelo

@nikomatsakis
Copy link
Contributor

OK, I agree that adding the FakeRead is wrong.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 26, 2021
…disjoint-fields-gate, r=nikomatsakis

 readd capture disjoint fields gate

This readds a feature gate guard that was added in PR rust-lang#83521. (Basically, there were unintended consequences to the code exposed by removing the feature gate guard.)

The root bug still remains to be resolved, as discussed in issue rust-lang#85561. This is just a band-aid suitable for a beta backport.

Cc issue rust-lang#85435

Note that the latter issue is unfixed until we backport this (or another fix) to 1.53 beta
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 27, 2021
…disjoint-fields-gate, r=nikomatsakis

 readd capture disjoint fields gate

This readds a feature gate guard that was added in PR rust-lang#83521. (Basically, there were unintended consequences to the code exposed by removing the feature gate guard.)

The root bug still remains to be resolved, as discussed in issue rust-lang#85561. This is just a band-aid suitable for a beta backport.

Cc issue rust-lang#85435

Note that the latter issue is unfixed until we backport this (or another fix) to 1.53 beta
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 27, 2021
…disjoint-fields-gate, r=nikomatsakis

 readd capture disjoint fields gate

This readds a feature gate guard that was added in PR rust-lang#83521. (Basically, there were unintended consequences to the code exposed by removing the feature gate guard.)

The root bug still remains to be resolved, as discussed in issue rust-lang#85561. This is just a band-aid suitable for a beta backport.

Cc issue rust-lang#85435

Note that the latter issue is unfixed until we backport this (or another fix) to 1.53 beta
pnkfelix added a commit to pnkfelix/rust that referenced this issue Jun 1, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 3, 2021
…nikomatsakis

Fix issue 85435 by restricting Fake Read precision

This PR fixes the root bug of issue rust-lang#85435 by restricting Fake Read precision in closures and removing the feature gate introduced in PR rust-lang#85564. More info [here](rust-lang#85561 (comment)) and [here](rust-lang#85561 (comment)).

Closes rust-lang#85561

r? `@nikomatsakis`
@bors bors closed this as completed in 34f1275 Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. requires-nightly This issue requires a nightly compiler in some way. 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.

5 participants