Promotion introduces UB into otherwise well-defined code

[RalfJung]

While investigating rust-lang/rust#121610, I noticed something problematic... we currently accept code like this:

let x: &'static Option<Cell<i32>> = &None;

Promotion kicks in despite interior mutability, because it sees the None variant and considers that one variant to not have interior mutability.

But also, both Stacked Borrows and Tree Borrows consider this to be legal:

    let o: Option<Cell<i32>> = None;
    let x = &o;
    ptr::from_ref(x).cast_mut().write(Some(Cell::new(0)));

They allow this because we avoid making "do we allow mutation" value-dependent. This is for two reasons: (a) we don't even want to require the reference to point to a valid value, but if the memory might store any sequence of bytes then the concept of value-dependency doesn't even make sense, and (b) checking the value stored behind the reference would introduce something very close to a cyclic argument into the memory model. So when creating a reference to o, we can't know whether o is None or Some, and therefore we just conservatively assume that interior mutability is allowed.

It should follow that this is also legal:

    let x: &Option<Cell<i32>> = &None;
    ptr::from_ref(x).cast_mut().write(Some(Cell::new(0)));

But this is UB, due to promotion putting the None into read-only memory. That's Not Good, promotion is introducing UB!

This is related to #236 but seems worth a separate sub-issue.
See rust-lang/rust#122789 for a crater run of at attempt to fix this.
Cc @oli-obk

[chorman0773]

This even has issues with the weaker vertical model I proposed, since you could do:

let offset = {
       let x = Some(Cell::new(0i32));
       let p = match &x{
              Some(y) => y as *const _,
              None => unreachable!()
        };
        p.byte_offset_from(addr_of!(x))
};
let x: &Option<Cell<i32>> = &None;
ptr::from_ref(x).cast_mut().byte_offset(offset).cast::<i32>().write(0);

I'm assuming we can't not promote there b/c it's valid on stable with &'static.

I think there's two options here, both bad:

• We adopt the variant-granular model for enums*
• We accept that const promotion can create UB

I don't think that it would be reasonable to lift read-only status, because that poses issues with (de-)duplication.

[RalfJung]

This even has issues with the weaker vertical model I proposed, since you could do:

Yeah, this needs full variant-awareness to be justified.

I'm assuming we can't not promote there b/c it's valid on stable with &'static.

Just because it's stable doesn't mean we can't try to slowly change it, maybe not promote it over an edition, whatever.

But I worry this might be extremely widely used. And until inline const finally stabilizes, we often don't have any alternative we can offer. And any kind of migration for promotion-related things is very hard to do since it is basically impossible to have a lint for "you are relying on X being promoted, but in the future this will not be promoted any more, use inline const instead".

We adopt the variant-granular model for enums*

What does the * mean?

[chorman0773]

I'm wondering if even that is sufficient.
I'm not sure I can come up with a counterexample, though.

[RalfJung]

Well, even with inline const it is somewhat questionable to make &const { None::<Cell<i32>> } point to constant memory...

[glaebhoerl]

IIUC, there are something like one-and-a-half issues here:

1. Promotion can cause programs to exhibit UB where they otherwise wouldn't
   2. Stacked Borrows and Tree Borrows aren't equipped to even detect this UB

For (2), instead of considering the value behind the reference, could this be addressed by distinguishing between read-only and mutable provenances, where the former is used for static read-only memory, and is "stronger than" interior mutability? Of course this might be a heavy hammer for dealing with a single edge case like this.

(I've been casually following the provenance and aliasing model discussions, but amn't fully in the loop, so feel free to dismiss this as a silly idea if it is one.)

[RalfJung]

Stacked Borrows and Tree Borrows aren't equipped to even detect this UB

Not sure what you mean, Miri detects this UB just fine.

[CAD97]

The case for item or inline const is certainly a footgun, but not model-breaking; "just" give all references to const data frozen/immutable provenance independent of the pointee type when created. This at least means any reliance on "infectious" mutability cannot be sound API even if well-defined in controlled environments.

IIUC, it's not plausible to only do const promotion when necessary to satisfy outlives requirements (scope-bound temporary lifetime extension when sufficient), as codegen decisions based on region inference is a no-go. It would certainly be a huge and undesirable footgun if we did it, but it would partially resolve the issue, in that code WF absent implicit const promotion would remain WF. (The same applies for a region-inference dependent lint.)

I see three options, roughly matching Connor's:

• Stop doing implicit const promotion for types which are not Freeze; require those usages to do manual promotion with const. (breaking)
• Always do implicit const promotion of candidate¹ lifetime-extended temporarys. Accept that this can introduce UB into code otherwise WF with nonconst lifetime extension.
  • Variant: attempt to lint problem cases, somehow.
  • Variant: attempt to skip const promotion when possible in order to mitigate UB.
• Adopt more granular shared mutability rules that inspect the enum discriminant, making the example always UB instead of conditionally WF.

If union literals are ever eligible for const promotion (they don't appear to be currently), they would encounter the same footgun and lack the option to granularly restrict shared mutability based on discriminant.

If we're going to in any way constrain const promotion from what's currently eligible in the future (i.e. over an edition) for any reason, it's probably a good idea to also constrain it to Freeze pointee types at the same time. Only with cross-edition "nobody notices" cleanup of const promotion, though, the potential footgun will remain in not-that-future edition code.

Personally, I don't think it's too bad — code relying on "adjacent" mutability is already a bad idea and only ever tenuously sound — but it is unfortunate that promotion — conceptually a "more things just work" feature — would restrict what code is WF. But not annoying enough to justify discriminant dependent retagging.

At a bare minimum, a clippy restriction lint against const promotion of ?Freeze types makes sense (once inline const exists).

Footnotes

1. The straightforward rule for candidate expressions I've seen is essentially just literal expressions, with adhoc bonus inclusions for the few specially tagged std functions. This naturally excludes any shared mutability since that transitively requires non-pattern construction (UnsafeCell::new or a transmutation source). ↩

[RalfJung]

If union literals are ever eligible for const promotion (they don't appear to be currently), they would encounter the same footgun and lack the option to granularly restrict shared mutability based on discriminant.

AFAIK unions get promoted but only if they are Freeze. We don't do value-based reasoning for unions, and rust-lang/rust#121893 adds a test ensuring it stays that way.

[RalfJung]

Fixing this the way I'd like to (by not doing value-based reasoning for interior mutability) would leak to 4k crater regressions (see rust-lang/rust#122789).

Not sure how to best proceed here. I think the best possible outcome at this point is to do this promotion change over an edition that tells people to use const { ... } instead, and say that inline consts are immutable even if the same expression at runtime would allow some mutability. (Option 1 in CAD's list above.) Ironically, if today we removed value-based reasoning from promotion but kept it for the const outer scope rule (to make const { &None } work but reject the fully implicit case), const { &None::<Cell<i32>> } would trigger rust-lang/rust#122153...

it is unfortunate that promotion — conceptually a "more things just work" feature — would restrict what code is WF. But not annoying enough to justify discriminant dependent retagging.

Agreed.

[RalfJung]

I've opened a separate issue for discussing the proposal of having the "const transition" implicitly make all pointers immutable: #502. (@CAD97 only mentioned references but I think it has to also include raw pointers.)