incorrect UB when when a ! place is constructed (but not loaded!)

[RalfJung]

This code shouldn't have UB but Miri reports UB and rustc generates a SIGILL:

#![feature(never_type)]
fn main() {
    unsafe {
        let x = 3u8;
        let x: *const ! = &x as *const u8 as *const _;
        let _ = *x;
    }
}

Thanks to @Nadrieril for finding this.

Zulip discussion

[RalfJung]

Labeling as "unsound" since under the rules that @rust-lang/opsem decided for *, and how we've been treating _ in general, this shouldn't SIGILL.

[Nadrieril]

Note that this triggers for ! but not enum Void{} so it's likely a type-based special case

[RalfJung]

This is already wrong in the initially generated MIR:

        _6 = &_2;
        _5 = &raw const (*_6);
        _4 = move _5 as *const ! (PtrToPtr);
        AscribeUserType(_4, o, UserTypeProjection { base: UserType(1), projs: [] });
        _3 = _4;
        StorageDead(_5);
        FakeRead(ForLet(None), _3);
        AscribeUserType(_3, o, UserTypeProjection { base: UserType(2), projs: [] });
        StorageDead(_6);
        StorageDead(_4);
        StorageLive(_7);
        StorageLive(_8);
        _8 = (*_3); // an actual load from the place!

If we use bool instead we get just a PlaceMention, as we should

        _5 = &_1;
        _4 = &raw const (*_5);
        _3 = move _4 as *const bool (PtrToPtr);
        AscribeUserType(_3, o, UserTypeProjection { base: UserType(1), projs: [] });
        _2 = _3;
        StorageDead(_4);
        FakeRead(ForLet(None), _2);
        AscribeUserType(_2, o, UserTypeProjection { base: UserType(2), projs: [] });
        StorageDead(_5);
        StorageDead(_3);
        PlaceMention((*_2)); // no load

@cjgillot any idea why we don't just get a PlaceMention for !?

[bjorn3]

-Zunpretty=thir-tree shows a NeverToAny expression. Maybe this has something to do with it?

[Nadrieril]

Oh, then this looks like fallback behavior: if I annotate let _: ! = *x; I only get a "entering unreachable code" error

[RalfJung]

Yeah that's also wrong, but different.^^ MIR for that case:

        _6 = &_2;
        _5 = &raw const (*_6);
        _4 = move _5 as *const ! (PtrToPtr);
        AscribeUserType(_4, o, UserTypeProjection { base: UserType(1), projs: [] });
        _3 = _4;
        StorageDead(_5);
        FakeRead(ForLet(None), _3);
        AscribeUserType(_3, o, UserTypeProjection { base: UserType(2), projs: [] });
        StorageDead(_6);
        StorageDead(_4);
        PlaceMention((*_3));
        AscribeUserType((*_3), +, UserTypeProjection { base: UserType(4), projs: [] });
        StorageDead(_3);
        StorageDead(_2);
        unreachable;

So we get a PlaceMention but then we also get a unreachable...

[RalfJung]

-Zunpretty=thir-tree shows a NeverToAny expression. Maybe this has something to do with it?

Yeah that's probably the right trail. MIR building might not even be at fault, the THIR is already wrong since it thinks there's a never value being constructed here. These THIR nodes seem to be constructed from Adjust::NeverToAny; no idea if there's any way to dump those.

Who are the right people to ping for THIR issues...?

[Jules-Bertholet]

@rustbot label F-never_type

[asquared31415]

Note that stable workarounds to access the never type, or cases where the type might be inferred still produce identical behavior, so this does not require the feature.

trait Extractor {
    type Output;
}

impl<T> Extractor for fn() -> T {
    type Output = T;
}

type RealNever = <fn() -> ! as Extractor>::Output;

fn main() {
    unsafe {
        let x = 3u8;
        let x: *const RealNever = &x as *const u8 as *const _;
        let _ = *x;
    }
}

[asquared31415]

#79735 mentions some similar cases that ! is special in places, though the primary problem (that deref into a let _ was allowed without unsafe) has since been resolved.

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

[matthewjasper]

This compiles, so this would also need some changes to type checking rules.

#![feature(never_type)]
fn make_string() -> String {
    unsafe {
        let x = 3u8;
        let x: *const ! = &x as *const u8 as *const _;
        let _ = *x; // Makes the unsafe block diverge, same as writing return String::new(); or let _ = panic!(); would
    }
}

[RalfJung]

Uff, yeah that should never have compiled...
Cc @rust-lang/lang

[Noratrieb]

The reason this compiles is probably

rust/compiler/rustc_hir_typeck/src/expr.rs

Lines 251 to 254 in 5777f2c

	// Any expression that produces a value of type `!` must have diverged
	if ty.is_never() {
	self.diverges.set(self.diverges.get() | Diverges::always(expr.span));
	}

HIR typeck thinks that an expression of type ! cannot possibly not diverge.. which is a reasonable assumption tbh. So I think we need to somehow special case that in HIR typeck... the let _ = *x semantics make sense at the MIR level but are pretty inconsistent for HIR

[RalfJung]

Can HIR tell apart places and values? A value expression of type ! obviously must diverge, but place expressions are quite different.

The comment is accurate, "Any expression that produces a value of type ! must have diverged". However, let _ = *x; does not produce a value of any type, it only produces a place.

[traviscross]

@rustbot labels -I-lang-nominated

We had an extensive discussion of this in the 2023-12-06 T-lang planning meeting last week.

The consensus¹ was general agreement with @RalfJung:

...that places of type ! should not be subject to never-to-any coercions (the coercion should apply only to values, matching the comment that justifies the coercion in the first place).

Specifically, we felt that it was 1) OK to have a place of ! type but insta-UB to ever coerce that place to a value, and 2) OK to not do never-to-any coercions on places.

Anywhere the compiler can statically determine that insta-UB is happening it would be desirable (but not required) to reject the code, preferably with a deny-by-default lint, but if it's more straightforward to emit a hard error, that's OK too.

Since we're agreeing to not do never-to-any coercions on places, anywhere that such a coercion would be required would now be a hard error due to the resulting type mismatch.

To the degree that changes to the behavior here break uses of the "never type hack", that's probably OK.

---

In the meeting, we settled on the general principles above, but did not go through every example. Following the logic of what we agreed, here's how this author would annotate the examples:

#![feature(never_type)]
use core::ptr::addr_of;
#[cfg(False)]
fn deref_never<T>() {
    unsafe {
        let x = 3u8;
        let x: *const ! = &x as *const u8 as *const _;
        //
        // In all cases, we do not enforce validity invariants on
        // places, and we do not perform never-to-any coercion on
        // places.
        //
        // `addr_of!` does not covert a place to a value.
        let _val = addr_of!(*x); //~ OK.
        //
        // The `_` binding does not convert a place to a value.
        let _ = *x; //~ OK.
        let _: ! = *x; //~ OK.
        //
        // Parentheses do not convert a place to a value.
        let _ = (*x); //~ OK.
        //
        // Since these do not convert a place to a value and
        // never-to-any coercion is not done on places, these do not
        // type check and are hard errors.
        let _: () = *x; //~ ERROR type mismatch
        let _: String = *x; //~ ERROR type mismatch
        let _: T = *x; //~ ERROR type mismatch
        //
        // In all cases, the compiler is free, on a best-effort basis,
        // to reject code that it can prove is insta-UB.  Preferably
        // it would do this as a deny-by-default lint, but it's also
        // OK to emit a hard error if that is much more
        // straightforward to implement.
        //
        // If a place of `!` type is converted to a value, that is
        // always insta-UB.
        //
        // The block expression converts a place to a value.
        let _ = { *x }; //~ Insta-UB.
        //
        // The tuple constructor converts a place to a value.
        let _ = (*x,); //~ Insta-UB.
        let (_,) = (*x,); //~ Insta-UB.
        //
        // Binding to a variable converts a place to a value.  Because
        // of never-to-any coercion on values these type check.
        let _val = *x; //~ Insta-UB.
        let _val: () = *x; //~ Insta-UB.
        let _val: String = *x; //~ Insta-UB.
        let _val: T = *x; //~ Insta-UB.
        //
        // Even if the resulting type of the value is explicitly `!`,
        // it's still insta-UB to convert a place of `!` type to a
        // value, since a value of type `!` is always insta-UB in
        // Rust.
        let _val: ! = *x; //~ Insta-UB.
        let _: ! = { *x }; //~ Insta-UB.
        let (_,): (!,) = (*x,); //~ Insta-UB.
    }
}

(Thanks to @WaffleLapkin for joining the meeting and for helpful discussions on this topic and to @scottmcm for staying on to the bitter end of the call to hammer out the details.)

Footnotes

1. Some members dropped off the call on the path to this consensus but asserted their trust in those that remained to work out the proper solution. ↩

[RalfJung]

@traviscross thanks for the detailed summary!

So... looks like the next step would be to actually implement these semantics. Since this occurs on the HIR layer I'm somewhat out of my depth here. Who are the HIR experts that we can ping to ask for help here? :)

[LunarLambda]

Why did this get marked "requires-nightly"? It reproduces on 1.79.0.

Also, I found a particularly funny version where codegen places the ud2 at the end of the function, allowing fn main to continue past the addr_of, then explode:

https://godbolt.org/z/fhdfK6jT1

trait Type { type T; }
impl<T> Type for fn() -> T { type T = T; }

// Make `!` nameable on stable
type Never = <fn() -> ! as Type>::T;

pub fn main() {
     let x = &();
     let y = (x as *const ()).cast::<Never>();

    // Should be sound (doesn't produce a value of `!`, only a place, so does not diverge)
    let z = unsafe { std::ptr::addr_of!(*y) };

    println!("huh: {z:p}");
}

If you change let z = unsafe { std::ptr::addr_of(*y) }; to let _ = unsafe { *y }; the println! gets destroyed.
let _ = unsafe { str::ptr::addr_of!(*y) }; also keeps the print. So compilation differs between addr_of!(*y) and *y, even when assigning to _.

rust-lang/unsafe-code-guidelines#261 (comment) Seems to suggest that let _ = *x; and let _ = addr_of!(*x); should be semantically identical, but they currently compile to very different MIR.

[WaffleLapkin]

I'd call <fn() -> ! as Type>::T a hack to purposefully avoid stability checks. While you can use this hack on stable I think that the "requires nightly" label is correct.

(in the same way as using RUSTC_BPOTSTRAP to avoid the checks does not mean that the issue does not require nightly)

[asquared31415]

isn't an empty enum equivalent to ! for the purposes of this place discussion?

[LunarLambda]

No this bug is specific to !. Equivalent code works fine for empty enums

[RalfJung]

This is now blocking #129248: making addr_of!(*ptr) a safe operation.

[compiler-errors]

Well, I can put up a hack to unblock that specifically. As @Nadrieril pointed out, this is actually two different "bugs" in HIR typeck:

#![feature(never_type)]
fn main() {
    unsafe {
        let x = 3u8;
        let x: *const ! = &x as *const u8 as *const _;
        let _ /* (1.) */ = *x;
        /* (2.) */
    }
}

In the above code:

1. At (1.), we actually do never type fallback to (). Coercing the ! to () does constitute a read, and therefore is UB.
2. At (2.), HIR typeck considers the fact that *x evalauted to ! to mean that all following code diverges, and therefore counts the block's type as ! as well.

Only (2.) actually matters for #129248, and if we were to land that PR right now, it would be unsound:

#![feature(never_type)]

fn make_up_a_value<T>() -> T {
    let x: *const ! = 0 as _;
    &raw const *x;
    // Since `*x` is `!`, HIR typeck thinks it diverges and allows the block to coerce to any value.
}

I will probably just put up a hack for (2.), since it's basically the only expression¹ where this matters. This doesn't fix the let _: ! = *x; case, but that should remain unsafe at least, and therefore just regular UB and not unsound.

Footnotes

1. Other combinations of HIR, such as match scrutinees that have no reads should also ideally be fixed... but that's really hard to fix I fear. ↩

[RalfJung]

I would say it is unsound in the sense that the reference doesn't say that this is UB. Admittedly the reference isn't clear on the entire point about let _ only evaluating the place expression, but that's what we've been consistently communicating for a while now and it has informed other decisions as well (around match, for instance).

But fixing one of two bugs is still progress, so that'd still be a great first step. :)