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

Raw pointer deref of uninhabited type allowed in const fn #77694

Closed
DutchGhost opened this issue Oct 8, 2020 · 13 comments · Fixed by #80199
Closed

Raw pointer deref of uninhabited type allowed in const fn #77694

DutchGhost opened this issue Oct 8, 2020 · 13 comments · Fixed by #80199
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. A-raw-pointers Area: raw pointers, MaybeUninit, NonNull A-stability Area: issues related to #[stable] and #[unstable] attributes themselves. C-bug Category: This is a bug. P-high High priority

Comments

@DutchGhost
Copy link
Contributor

DutchGhost commented Oct 8, 2020

The code below compiles as of 1.46, but I'm not sure it should, since it dereferences a raw pointer in a const block, which is only allowed in nightly with the #![feature(const_raw_ptr_deref)] flag:

const unsafe fn unreachable() -> ! {
    use core::convert::Infallible;
    
    const INFALLIBLE: *const Infallible = [].as_ptr();
    match *INFALLIBLE {}
}

fn main() {
}
@ohsayan
Copy link
Contributor

ohsayan commented Oct 8, 2020

@rustbot modify labels to +A-raw-pointers +A-const-fn

@rustbot rustbot added A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. A-raw-pointers Area: raw pointers, MaybeUninit, NonNull labels Oct 8, 2020
@jonas-schievink jonas-schievink added A-stability Area: issues related to #[stable] and #[unstable] attributes themselves. C-bug Category: This is a bug. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 8, 2020
@jonas-schievink
Copy link
Contributor

cc @rust-lang/wg-const-eval

@RalfJung
Copy link
Member

RalfJung commented Oct 8, 2020

Ah, that looks like the const version of the old "uninhabited match" problem...

The MIR for unreachable is

fn unreachable() -> ! {
    let mut _0: !;                       // return place in scope 0 at src/main.rs:1:34: 1:35

    bb0: {
        unreachable;                     // scope 0 at src/main.rs:5:11: 5:22
    }
}

So somehow the deref already vanished, and hence the const analysis cannot catch it.

But doesn't unsafety checking also happen on the MIR? Why does that work?

@DutchGhost
Copy link
Contributor Author

It seems that whenever the dereference is not bound to a variable, it compiles, but whenever bound to a variable, it fails to compile:

const unsafe fn const_deref() {
    use core::convert::Infallible;
    const INFALLIBLE: *const Infallible = [].as_ptr();
    
    match *INFALLIBLE {
        n => {}, // <--- rightfully claims I need #![feature(const_raw_ptr_deref)]
        //_ => {}, // <--- compiles fine
    };
}

@LeSeulArtichaut LeSeulArtichaut added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Oct 8, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Oct 8, 2020

I don't believe this is an unsoundness. Unsafe code is required to cause anything here.

The MIR for unreachable is

That's the final MIR, since without unsafe fn this will fail to compile, it must be there for unsafety checking, thus it must also be there for const checking. Not sure why we fail to catch this.

@RalfJung
Copy link
Member

RalfJung commented Oct 8, 2020

I don't believe this is an unsoundness. Unsafe code is required to cause anything here.

Agreed. This is a case of "oops a nightly-only feature is available on stable", which is bad, but it is not an unsoundness.

@RalfJung RalfJung removed the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Oct 8, 2020
@camelid
Copy link
Member

camelid commented Oct 8, 2020

Assigning P-high and removing I-prioritize as discussed in the prioritization working group.

@camelid camelid added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 8, 2020
@RalfJung
Copy link
Member

RalfJung commented Dec 7, 2020

The initial MIR looks like this:

fn unreachable() -> ! {
    let mut _0: !;                       // return place in scope 0 at test2.rs:1:34: 1:35
    let mut _1: !;                       // in scope 0 at test2.rs:1:36: 6:2
    let mut _2: !;                       // in scope 0 at test2.rs:5:5: 5:25
    let mut _3: *const std::convert::Infallible; // in scope 0 at test2.rs:5:12: 5:22

    bb0: {
        StorageLive(_1);                 // scope 0 at test2.rs:1:36: 6:2
        StorageLive(_2);                 // scope 0 at test2.rs:5:5: 5:25
        StorageLive(_3);                 // scope 0 at test2.rs:5:12: 5:22
        _3 = const INFALLIBLE;           // scope 0 at test2.rs:5:12: 5:22
                                         // ty::Const
                                         // + ty: *const std::convert::Infallible
                                         // + val: Unevaluated(WithOptConstParam { did: DefId(0:5 ~ test2[317d]::unreachable::INFALLIBLE), const_param_did: None }, [], None)
                                         // mir::Constant
                                         // + span: test2.rs:5:12: 5:22
                                         // + literal: Const { ty: *const std::convert::Infallible, val: Unevaluated(WithOptConstParam { did: DefId(0:5 ~ test2[317d]::unreachable::INFALLIBLE), const_param_did: None }, [], None) }
        FakeRead(ForMatchedPlace, (*_3)); // scope 0 at test2.rs:5:11: 5:22
        unreachable;                     // scope 0 at test2.rs:5:11: 5:22
    }

    bb1: {
        unreachable;                     // scope 0 at test2.rs:5:5: 5:25
    }

    bb2: {
        StorageDead(_2);                 // scope 0 at test2.rs:5:24: 5:25
        unreachable;                     // scope 0 at test2.rs:1:36: 6:2
    }

    bb3: {
        StorageDead(_3);                 // scope 0 at test2.rs:6:1: 6:2
        StorageDead(_1);                 // scope 0 at test2.rs:6:1: 6:2
        return;                          // scope 0 at test2.rs:6:2: 6:2
    }
}

So probably the issue is that const-checking ignores FakeRead?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 7, 2020

oh... yea.. we ignored it on purpose... "because it doesn't actually do anything" 🙃

I guess we should visit all places within statements that we otherwise ignore during const-checking?

@RalfJung
Copy link
Member

RalfJung commented Dec 7, 2020

Well, at least we need to visit FakeRead. ;)

@RalfJung
Copy link
Member

oh... yea.. we ignored it on purpose... "because it doesn't actually do anything" upside_down_face

Do we really? The Deref check is done in visit_projection_elem, which should be called even for the projection in the place in the fake read.

@RalfJung
Copy link
Member

Oh wait, we don't call super_statement unconditionally! That seems like... a problem.^^

@RalfJung
Copy link
Member

Proposed a fix: #80199

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 21, 2020
also const-check FakeRead

We need to const-check all statements, including `FakeRead`, to avoid issues like rust-lang#77694.

Fixes rust-lang#77694.
r? `@oli-obk`
@bors bors closed this as completed in 000c516 Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. A-raw-pointers Area: raw pointers, MaybeUninit, NonNull A-stability Area: issues related to #[stable] and #[unstable] attributes themselves. C-bug Category: This is a bug. P-high High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants