Github Permit mutable references in all const contexts by oli-obk · Pull Request...

rust-highfive commented on Oct 31, 2020

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

This comment has been hidden.

oli-obk commented on Oct 31, 2020

r? @ecstatic-morse

RalfJung commented on Oct 31, 2020 •

This also allows (with feature(const_mut_ref)) mutable references in the final value of
immutable statics, which is not a problem, because
accessing a mutable static FOO works via *&FOO, so
the intermediate immutable reference blocks any mis-use.

But what about interior mutability? Also, what is even the problem against which this is defending? Assuming the lifetimes work out, I see no reason that an &mut in a static would cause any issue. The only concern is we need to make sure that we put it into mutable global memory whenever it can actually be mutated. Interning should already get this right, but we should have some tests to ensure that this is the case, but I am not sure what the best way is to test for this.

Constants prevent mutable references in their final value
via two separate means:

So, we are giving up on doing static checks here, and rely on (post-monomorphitation) "dynamic" checks?

oli-obk commented on Nov 1, 2020

But what about interior mutability?

We already permit that on stable and it's only possible through wrapper types with unsafe impl Sync, so the user said it's alright.

Also, what is even the problem against which this is defending?

Since we allow static FOO: &mut i32 = &mut 42; now, that could mean users can at any arbitrary point write

let x: &'static mut i32 = FOO;
let y: &'static mut i32 = FOO;

and then they have two mutable references to the same memory. BUT since we actually generate the above as

let x: &'static mut i32 = *&FOO;
let y: &'static mut i32 = *&FOO;

The user will get an error because you can't access a mutable reference behind an immutable referenc as mutable.

Assuming the lifetimes work out, I see no reason that an &mut in a static would cause any issue.

Not in the current way that MIR implements static accesses, and not in the way I think we want to ever use statics, but it still something we need to protect against.

The only concern is we need to make sure that we put it into mutable global memory whenever it can actually be mutated. Interning should already get this right, but we should have some tests to ensure that this is the case, but I am not sure what the best way is to test for this.

Why into mutable memory? You cannot safely access it mutably, and any access will go through an immutable reference, so unsafely accessing it mutably would be UB, right?

So, we are giving up on doing static checks here, and rely on (post-monomorphitation) "dynamic" checks?

hmm... indeed. I'll read through @eddyb 's suggestion in zulip again.

RalfJung commented on Nov 2, 2020 •

We already permit that on stable and it's only possible through wrapper types with unsafe impl Sync, so the user said it's alright.

What I meant is, RefCell<&mut T> should be okay... right? Your OP does not talk about this case at all.

Since we allow static FOO: &mut i32 = &mut 42; now, that could mean users can at any arbitrary point write

Ah, right. So even if we move away again from "statics as const pointers", we have to make sure they are still treated as an immutable place by the borrow checker.

Why into mutable memory? You cannot safely access it mutably, and any access will go through an immutable reference, so unsafely accessing it mutably would be UB, right?

I said "we need to make sure that we put it into mutable global memory whenever it can actually be mutated" (emphasis is new). Like for example in case of RefCell<&mut T>. I think our code does this right, but it is a case worth carefully documenting and thinking through (and testing), that's why I brought it up.

oli-obk commented on Nov 2, 2020

What I meant is, RefCell<&mut T> should be okay... right? Your OP does not talk about this case at all.

Well, not RefCell per se, but a custom Sync type could work from a type-system perspective. It will still not work though.

use core::cell::UnsafeCell;
struct NotAMutex<T>(UnsafeCell<T>);

unsafe impl<T> Sync for NotAMutex<T> {}

const FOO: NotAMutex<&i32> = NotAMutex(UnsafeCell::new(&42));

The above only works, because &42 is promoted. I think if you could move to &mut (like in this PR), you'd get borrow errors. You can emulate the effect on nightly with immutable references by using a block:

const FOO: NotAMutex<&i32> = NotAMutex(UnsafeCell::new(&{
    let x = 42;
    x
}));

I'll write appropriate tests and post my findings here.

oli-obk commented on Nov 2, 2020

I added a new test for your example, and as I expected, it is not possible to do that, so we do not need to check whether interning can handle this.

RalfJung commented on Nov 2, 2020

What about using &mut STATIC_MUT?

oli-obk commented on Nov 2, 2020

For constants that is not possible anyway (yes, this time it is really a static check ;) ).

For static items, it's not a problem, as the allocation behind the reference is not part of the static currently being interned. For all other concerns, accessing a mutable static requires unsafe, so the user is the one who has to prove correctness.

RalfJung commented on Nov 3, 2020 •

Hm, I am running out of ideas.^^ Would unleashing Miri let us disable some of these restrictions?

I feel like there probably are or will be other ways to get &mut, such as transmuting &UnsafeCell<T> to &mut T or Box::leak or so... I think our code should handle this all correctly, so I would much rather rely on the interner being correct than relying on it being impossible to write such code. You invoked several otherwise independent invariants to crush my counterexamples, which makes this all seem rather fragile.

In particular you invoked the const-may-not-point-to-static restriction, which only exists for the benefit of const-in-pattern and which rejects way more code that it would have to. If, hypothetically, we'd instead dynamically check this only when a const is actually used in a pattern, your scheme here does not work any more. I have a plan for how to make that restriction not load-beaing any more (when constructing a valtree, ensure we only read from immutable allocations; make patterns go through valtrees). I think we should be careful not to add anything else that would make this restriction load-bearing in different ways.

EDIT: Or, did you invoke that restriction? I am losing track of what all has to work together to make this sound, which is a bad sign. :/ But I think yes you did, "For constants that is not possible anyway" -- "that" being &mut STATIC_MUT. But why does the argument that applies to static not also apply to const here?

Interning prevents pointers to allocations except as part of references

As discussed in this PR before, I don't think this check should be relied upon for soundness.

So I propose that for static, the soundness argument is more along the lines of:

• all accesses go through shared references, so plain &mut cannot be used for mutation anyway
• when there is an UnsafeCell or "hidden pointers", we correctly intern allocations referenced inside there as mutable

For const, we disallow mutable references to show up directly, as you noted... but for "hidden pointers" the story is much less clear, unless we want to commit to never allowing those.

RalfJung commented on Nov 3, 2020 •

when there is an UnsafeCell or "hidden pointers", we correctly intern allocations referenced inside there as mutable

Btw, there is a funny corner case here where the same allocation is referenced multiple times, so it might end up immutable from the first reference we encounter to it and then later another reference wants it to be mutable... something like

#![feature(const_mut_refs)]
#![feature(const_raw_ptr_deref)]
static mut FOO: i32 = 0;

struct SyncRawPtr<T>(*mut T);
unsafe impl<T> Sync for SyncRawPtr<T> {}

static EVIL: (&i32, SyncRawPtr<i32>) = {
  let x: *mut i32 = unsafe { &mut FOO };
  (unsafe { &*x }, SyncRawPtr(x))
};

#![feature(const_mut_refs)]
#![feature(const_raw_ptr_deref)]
static mut FOO: i32 = 0;

struct SyncRawPtr<T>(*mut T);
unsafe impl<T> Sync for SyncRawPtr<T> {}

const fn make_evil() -> (&'static i32, SyncRawPtr<i32>) {
    let x: *mut i32 = unsafe { &mut FOO };
    (unsafe { &*x }, SyncRawPtr(x))
}

static EVIL: (&i32, SyncRawPtr<i32>) = {
    make_evil()
};

The latter seems to hinge only on "constant functions cannot refer to statics" (and one could imagine Box::leak(Box::new(0)) working some day to provide an alternative).

One could argue that if EVIL.0 is never used, writing to EVIL.1 is actually fine...

RalfJung commented on Nov 3, 2020 •

Sorry for all the rambling, but I feel very uneasy about this change.^^ I don't think the required soundness conditions and the checks that uphold them have been documented clearly enough. We shouldn't start with examples, we should start with "here is a list of (dynamic) conditions that, if they are all met, we agree are sufficient to make this all sound", and then we should go on with "here are some (static and/or dynamic) checks that ensure that all conditions are met", and then we can come up with testcases to try to beak them.

oli-obk commented on Nov 3, 2020

We shouldn't start with examples, we should start with "here is a list of (dynamic) conditions that, if they are all met, we agree are sufficient to make this all sound", and then we should go on with "here are some (static and/or dynamic) checks that ensure that all conditions are met", and then we can come up with testcases to try to beak them.

I think part of this is because, as you correctly noted, we are mixing a lot of different concerns. E.g. I consider it ok for the dynamic checks to permit const FOO: *mut i32 = &mut 42; since, that is not a soundness issue, just a footgun. Obviously the static checks must at least cover all dynamic checks, but we don't just need to cover "too hard to statically check" in addition, we can also add static checks to prevent footguns.

One could argue that if EVIL.0 is never used, writing to EVIL.1 is actually fine...

I thought we had to consider all references to be used, no matter if they actually are. I don't see how having a shared reference to some memory that is mutated through other means is fine, but a reference to dangling memory is not.

The only way that example seems doable to me is to use *const i32 instead of &i32, and then we can't intern immutably anyway.

RalfJung commented on Nov 3, 2020 •

I thought we had to consider all references to be used, no matter if they actually are. I don't see how having a shared reference to some memory that is mutated through other means is fine, but a reference to dangling memory is not.

Well, the following is allowed under Stacked Borrows in runtime code:

    let x: *mut i32 = unsafe { &mut FOO };
    let (_, x) = (unsafe { &*x }, SyncRawPtr(x));
    unsafe { *x = 1; }

Basically, the lifetime of the shared reference ends immediately, that's why this is okay. (It would also be permitted to dangle.) The guarantee of a shared ref is that between any two of its uses, nothing has been mutated, but if the shared ref is unused... well, this guarantee doesn't do anything. (The only special case is references passed as function parameters; that is the "protector" stuff in Stacked Borrows.)

I think to obtain the behavior you seem to expect, we'd need to somehow reflect the idea that "the 'static lifetime goes on forever" into Stacked Borrows, making sure the tag of the shared reference never gets popped... but we don't actually have any tags in compile-time computed pointers so this cannot really work.^^ Indeed having provenance somehow connect compile-time and run-time computations seems tricky (and strange), but if we make compile-time-computed pointers have no provenance, that automatically means there is no way to distinguish different compile-time computed pointers to the same data.

oli-obk commented on Nov 3, 2020

So... "must be unused" is actually not true here? We could alternately use the reference to read and the SyncRawPtr to write, as long as we don't keep a reference around while using SyncRawPtr? At least from a stacked borrows perspective.

oli-obk commented on Nov 3, 2020

Sorry, all of this is getting a bit off topic, but I really want to understand the details even if irrelevant to this PR. Should we take it to some other place?

I fully agree that we should not intern an allocation as immutable if some later place wants to intern it as mutable.

Though, in the case you showed, we won't intern anything anyway, as that is a pointer to a mutable static, which by itself is already a mutable allocation. But for hyptothetical heap allocations, this could definitely occur.

RalfJung commented on Nov 4, 2020

Right, sorry, I was somewhat distracted this morning and did not have enough time to reply properly. I still don't really have enough time...

So in terms of soundness, if all "inner" static allocations are interned mutable and hidden pointers are not allowed in const, I think I can agree that this is all sound. I just did not agree with the reasons you gave for soundness above, namely that how &mut can appear in static is restricted... that was never a soundness goal so far, so I do not think it should become one now. But I think it is not necessary, if interning does the right thing.

So my main soundness concerns are around

• Interning inner static allocations as immutable: we need to be really sure that this is indeed correct. I wonder if validity checking can help here... probably not because of raw pointers.
• Can we do this in a way that leaves the door open for "hidden pointers" in a const without being a huge footgun? A lint is likely to have many false positives, which is not great.

RalfJung commented on Nov 7, 2020

After thinking about this some more, I wonder if there isn't another approach -- given my inability to come up with an example that has mutable memory in an inner static allocation. We could say that all inner allocations of static (and static mut) are interned immutably. We'd have to add some tests to make sure that this holds up when with Miri-unleashed (and validation might be able to help by detecting UnsafeCell), but if it does it would let us remove all the type-based interning and instead just intern all inner allocations of everything immutably, which would be a great simplification.

@oli-obk almost convinced me that this could be sound for current CTFE, though I'd need to think about this some more. It would clearly not be sound with heap allocations during CTFE, but those are easily recognized and we could just always intern them mutably.

oli-obk commented on Nov 10, 2020

I can unconvince you in a single line that works on stable:

static mut FOO: &mut [u32] = &mut [5, 3, 4];

your mention of static mut reminded me that we have this special case for slices in mutable statics. Maybe I should finally create the future incompat lint removing static mut from the language?

RalfJung commented on Nov 10, 2020

I can unconvince you in a single line that works on stable:

Isn't that just because #75585 did not arrive on stable yet?

bors commented 14 days ago

The latest upstream changes (presumably #80942) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung commented 13 days ago

r? @RalfJung
@rustbot modify labels: -S-waiting-on-review +S-waiting-on-author
(There are some unresolved comments at the top of the discussion; as far as I can see they still apply.)

RalfJung commented 6 days ago

@bors r+

bors commented 6 days ago

Commit 819b008 has been approved by RalfJung