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.)

[Diggsey]

I think promotion was clearly intended to be "transparent" to users - ie. the only visible side effect should be the extended lifetime. If it was meant to have other side effects then it would have been introduced with an explicit syntax for promotion, since Rust generally places a lot of importance on explicitness.

Given that, I think the proposed solution of removing implicit promotion for !Freeze types is the best option.

I think that even having promotion for !Freeze types inside const {...} blocks is quite magical. What about requiring manual promotion (ie. you must declare a static variable of the !Freeze type) for this kind of promotion to work:

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

AIUI, cell would not even be placed in read-only memory, and so the mutations would be completely legal (assuming appropriate synchronization).

[RalfJung]

I think that even having promotion for !Freeze types inside const {...} blocks is quite magical.

Yes, it is. OTOH, if users don't write unsafe, it will actually do exactly what they want. So there's a question here whether it's worth requiring the extra syntactic overhead of something even more explicit (and making that even possible with generics), given that most code won't care and given that we have 10 years worth of code relying on this happening fully implicitly.

I am not aware of cases where this caused issues -- we won't see all those cases, obviously, but writing to such a promoted would fairly reliably segfault since they are stored in read-only memory, so if people were surprised by that I suspect they would tell us.

What about requiring manual promotion (ie. you must declare a static variable of the !Freeze type) for this kind of promotion to work:

This doesn't work with generics so we'll probably not be able to offer good migration for some amount of the existing code -- leaving aside the ergonomic questions.

[Diggsey]

given that most code won't care and given that we have 10 years worth of code relying on this happening fully implicitly.

We've established that this would require an edition change anyway, so if a more explicit syntax is better, then we should change to that syntax, rather than compromising for no reason. While the amount of breakage is enough to require an edition change, in the grand scheme of things a tiny amount of code would be affected (and thus require the longer syntax).

This doesn't work with generics so we'll probably not be able to offer good migration for some amount of the existing code -- leaving aside the ergonomic questions.

Ah... Still, there are other more explicit options.

[RalfJung]

Ah... Still, there are other more explicit options.

Such as? I don't think there are.

[Diggsey]

Such as? I don't think there are.

I mean you could add any syntax you want to explicitly call it out, but the obvious one is to use a const instead of a static as I originally suggested. Generic consts are unstable but don't suffer from the implementation problems of generic statics.
rust-lang/rust#113521

[RalfJung]

I have no idea what you mean. If you want to reject const { &None::<Cell<T>> } then clearly we should also reject

const MY_CONST: &'static Cell<i32> = &None;

so we can't use consts instead of statics either.

Put differently, we already have a syntax for

{
    const cell: Option<Cell<i32>> = &None;
    cell
}

and it's const { &None }.

Now I realize your desugaring put the & outside the static, but that will work even less because then we'd still have to implicitly promote an interior mutable value: &CONST relies on promotion to get lifetime 'static since CONST is a value. (In contrast, &STATIC always has 'static lifetime since STATIC is a place, not a value.)

IOW, this is still implicit promotion causing UB in what looks like well-defined code and should be rejected, so we can't have it desugar to this:

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

So, if you think const { &None } should still error then I don't think there is any kind of desugaring in terms of existing or proposed language mechanisms that we can use. We'd need an entirely new mechanism. I think the end result would be more confusing than using const { ... } for this.

[RalfJung]

I think fixing this can only really happen as part of rust-lang/rust#124328.