-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Consider deprecation of UB-happy static mut
#53639
Comments
I don't know at all whether we should do this or not atm. But to make this decision I have an... ...Idea: We should do a crater run to scrutinize how many legitimate usages there are and how many ones there are that have UB. |
cc @rust-lang/wg-unsafe-code-guidelines -- we have this group just for this sort of thing =) |
I'm 100% behind getting rid of |
We should clearly document the undefined behavior, but I don't think we should remove the ability to have (unsafe) global mutable locations directly, without any layer of indirection. That would make it more difficult to build certain types of lock-free synchronization, for instance. |
@joshtriplett But you can always use |
Can you give an example of what you mean? You can't do much more with |
Yeah, and that's just as unsafe. So TBH I do not see the point. |
@RalfJung No, it's not, and I've explained why. With EDIT: Now if "references exist" cannot be UB, ever, then this is not a problem, but IIUC, they do. |
It's not actually different from an |
@eddyb Ah okay I see -- that has nothing to with with I agree |
@RalfJung Okay I should make it clearer that the |
If we could weaken ... tough people would probably still turn them into references ASAP. |
How would they instantiate their custom type in stable Rust? User To me this sounds like it would reduce what you can do in the 2018 edition. In the 2015 edition you can create |
@japaric Okay, that's a good point. I removed the bound from the Note that you can use an associated constant if you don't need arguments in your constructor (e.g. Depending on what you're doing you might be able to make your fields public, but as was in the case of cc @Centril @oli-obk Do we want to force people to replace their "custom synchronization abstractions" involving
What kinds of types?
Hmm, Given I wish the |
@rfcbot poll @rust-lang/libs @rust-lang/lang Should we add #[repr(transparent)]
pub struct RacyUnsafeCell<T>(UnsafeCell<T>);
unsafe impl<T: Sync> Sync for RacyUnsafeCell<T> {}
impl<T> RacyUnsafeCell<T> {
pub const fn new(x: T) -> Self {
RacyUnsafeCell(UnsafeCell::new(x))
}
pub fn get(&self) -> *mut T {
self.0.get()
}
} |
Team member @eddyb has asked teams: T-lang, T-libs, for consensus on:
|
@RalfJung What we want is That is, the user of the abstraction should be outside of the "unsafe zone" and be able to use only safe code to interact with the abstraction, otherwise it's not really a good abstraction. |
In embedded we use // we generate this function using macros
#[export_name = "SOME_KNOWN_NAME_TO_HAVE_THE_LINKER_PUT_THIS_IN_THE_RIGHT_PLACE"]
pub unsafe extern "C" fn interrupt_handler_that_has_some_random_name_unknown_to_the_user() {
static mut STATE: usize = 0; // initial value provided by the user
on_button_pressed(&mut STATE);
}
// the user provides this function along with the initial value for the state
fn on_button_pressed(count: &mut usize) {
*count += 1;
println!("Button has been pressed {} times", *count);
} This way the end user indirectly uses As there are no newtypes involved the user can use primitives and types I'm not against this feature as long as the above pattern continues to work on stable Rust 2018. |
If there's an API that @japaric and libs are happy with, I'm happy to remove |
Personally, I think we should just deprecate |
@japaric Ah, if you're doing code generation, you can have your own version of There's a more powerful pattern I prefer using in those situations: // Lifetime-invariant ZST token. Probably doesn't even need the invariance.
#[repr(transparent)]
#[derive(Copy, Clone)]
pub struct InterruptsDisabled<'a>(PhantomData<&'a mut &'a ()>);
// Public field type to get around the lack of stable `const fn`.
pub struct InterruptLock<T: ?Sized + Send>(pub UnsafeCell<T>);
// AFAIK this should behave like Mutex.
unsafe impl<T: ?Sized + Send> Sync for InterruptLock<T> {}
impl<T: ?Sized + Send> InterruptLock<T> {
// This gives access to the `T` that's `Send` but maybe not `!Sync`,
// for *at most* the duration that the interrupts are disabled for.
// Note: I wanted to implement `Index` but that can't constrain lifetimes.
fn get<'a>(&'a self, _: InterruptsDisabled<'a>) -> &'a T {
unsafe { &*self.0.get() }
}
}
// Note that you bake any of these into `InterruptLock` without `const fn`,
// because you would need a private field to ensure nobody can touch it.
// OTOH, `UnsafeCell<T>` makes *only constructing* the field public.
pub type InterruptCell<T: ?Sized + Send> = InterruptLock<Cell<T>>;
pub type InterruptRefCell<T: ?Sized + Send> = InterruptLock<RefCell<T>>; #[export_name = "SOME_KNOWN_NAME_TO_HAVE_THE_LINKER_PUT_THIS_IN_THE_RIGHT_PLACE"]
pub unsafe extern "C" fn interrupt_handler_that_has_some_random_name_unknown_to_the_user(
token: InterruptsDisabled,
) {
on_button_pressed(token);
}
static COUNT: InterruptCell<usize> = InterruptLock(UnsafeCell::new(
Cell::new(0),
));
fn on_button_pressed(token: InterruptsDisabled) {
let count = COUNT.get(token);
count.set(count.get() + 1);
println!("Button has been pressed {} times", count.get());
} There's another variation on this, where you make the token non- #[export_name = "SOME_KNOWN_NAME_TO_HAVE_THE_LINKER_PUT_THIS_IN_THE_RIGHT_PLACE"]
pub unsafe extern "C" fn interrupt_handler_that_has_some_random_name_unknown_to_the_user(
token: InterruptsDisabled,
) {
on_button_pressed(token);
}
static COUNT: InterruptLock<usize> = InterruptLock(UnsafeCell::new(
0,
));
fn on_button_pressed(mut token: InterruptsDisabled) {
// This mutable borrow of `token` prevents using it for
// accessing any `InterruptLock`s, including `COUNT` itself.
// The previous version was like `&token` from this one.
let count = COUNT.get_mut(&mut token);
*count += 1;
println!("Button has been pressed {} times", *count);
} (I didn't write the implementation details for the second version for brevity's sake, if desired I can edit this later) You can even allow creating a Theoretically you can even create a IMO a safe abstraction like this is a good long-term investment, since it can handle any sort of cc @RalfJung Has anything like this been proven safe? |
@eddyb note that the non-reentrancy that @japaric describes is for that particular interrupt handler (and any lower priority, but that’s hard to do anything useful with). Interrupts are not globally disabled during an interrupt handler on architectures like ARMv6-M. It might be possible to use a more complicated token scheme where each interrupt has its own token type, but I don’t know if anyone has looked into some way to make that actually usable. |
@Nemo157 Ah, that's a very interesting detail! Yeah you can just have one token type per level and use Another idea that I came up with while discussing with @arielb1 on IRC: We could only allow private That, combined with deprecation of private |
IIRC, AVR actually allows for recursive interrupt handlers:
It's up to the programmer to choose (and thus appropriately handle) this case. |
On every architecture I'm aware of, you can explicitly set the interrupt flag after entering a handler; this is not AVR-specific. (On architectures with interrupt prioritization, like Cortex-M, you'll also need to manipulate priorities.) But the point here is that this needs to be done explicitly, with unsafe code; the architecture guarantees that you wouldn't reenter the ISR if you don't specifically request that. So this is perfectly in line with the usual safety rules. |
Some C APIs expose mutable globals that my Rust code right now access via: extern "C" {
pub static mut FOO: BAR;
} Some C APIs require their users to define mutable globals, that the C library then accesses. Right now my Rust code interfaces with those using: #[export(name = "foo")]
pub static mut FOO: BAR = ..initializer..; How do I access mutable globals from C libraries, and how do I provide mutable globals to C libraries, without An example of a C library that uses both is |
I don’t like this. It’s already unsafe. That’s good enough. |
const interning: decide about mutability purely based on the kind of interning, not the types we see r? `@oli-obk` this is what I meant on Zulip. For now I left the type visitor in the code; removing it and switching to a simple interning loop will mean we accept code that we currently reject, such as this ```rust const CONST_RAW: *const Vec<i32> = &Vec::new() as *const _; ``` I see no reason for us to reject such code, but accepting it should go through t-lang FCP, so I want to do that in a follow-up PR. This PR does change behavior in the following situations: 1. Shared references inside `static mut` are no longer put in read-only memory. This affects for instance `static mut FOO: &i32 = &0;`. We never *promised* that this would be read-only, and `static mut` is [an anti-pattern anyway](rust-lang#53639), so I think this is fine. If you want read-only memory, write this as `static INNER: i32 = 0; static mut FOO: &i32 = &INNER;`. 2. Potentially, mutable things in a `static` are now marked read-only. That would be a problem. But I am not sure if that can happen? The code mentions `static FOO: *const AtomicUsize = &AtomicUsize::new(42)`, but that is rejected for being non-`Sync`. [This variant](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=112e930ae1b3ef285812ab404ca296fa) also gets rejected, and same for [this one](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0dac8d173a2b3099b9c2854fdad7a87c). I think we should reject all cases where a `static` introduces mutable state, except for the outermost allocation itself which can have interior mutability (and which is the one allocation where we have fully reliable type information). What I still want to do in this PR before it is ready for review it is ensure we detect situations where `&mut` or `&UnsafeCell` points to immutable allocations. That should detect if we have any instance of case (2). That check should be part of the regular type validity check though, not part of interning.
See discussion in rust-lang/rust#53639. This came up during an internal review. Signed-off-by: Christian Blichmann <[email protected]> (cherry picked from commit 88f3537) Signed-off-by: Bo Chen <[email protected]>
See discussion in rust-lang/rust#53639. This came up during an internal review. Signed-off-by: Christian Blichmann <[email protected]> (cherry picked from commit 88f3537) Signed-off-by: Bo Chen <[email protected]>
…g/rust#53639 (Also removes std::ops::Index bounds checking)
Interesting minor detail I wasn't aware of prior — However, I still agree that deprecating Although, silly (probably bad) idea: instead of a separate |
Yes, the only concern I have about dropping (The thread-ID check has to be separate to the |
@uazu Idea being that |
Excerpt from the docs of
For the |
I think there may be issues with ThreadBound. If I call I'm trying to do things as efficiently as you'd do it in C, i.e. just use globals but promising not to use them from any other thread, but formalizing that in Rust so there is a genuine guarantee that all Rust's rules are kept. The cost is one small check once, and then you can keep cloning some zero-size zero-cost type, so effectively the result of that check is only known to the compiler and doesn't appear in the final executable. In the machine code it looks just like it would in C. |
That is already UB at the moment you create the |
The ThreadBound code provided in the example requires you to get a In my code I am not using ThreadBound. I have a The problem is that all these globals are effectively private globals for a wrapper type (DeferrerAux). That is the type that makes everything sound. Each individual piece if viewed in isolation is not necessarily sound. So I can't put |
How about putting an |
Keeping |
It seems an awkward solution, i.e. an UnsafeCell containing the thread-ID and another UnsafeCell, but with a backdoor that allows the check to be bypassed if the wrapper "knows" that the check has already been done. I agree that it meets the requirements, though, and looks like it won't break any rules. If the decision is made to go ahead and deprecate |
With the lint being tracked in #128794, I think we should close this issue: Nominating for discussion by @rust-lang/lang: should we close this in favor of #128794 or are there still plans to entirely deprecate |
Agreed. Given the degree to which @rfcbot fcp close |
Team member @traviscross has proposed to close this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
(@RalfJung suggested I mention my opinion here when I brought it up on Zulip. Disclaimer: I haven't read the entire history of this specific issue; I've been aware of other discussions of I claim that Currently, newcomers often choose Keeping |
I want to second @kpreid's sentiment here: I think we should still eventually remove In my opinion, removing |
Sometimes, you need global mutable state. We already had several cases of people being quite confused and concerned by the lints we started emitting, and it turns out that what they were doing is completely fine and we basically told them to add a bit of noise to their code to silence the lint. Those people will be even more confused if we now tell them that Many people that use So overall I am not convinced that the churn caused on existing code, and the overhead caused for people that know what they are doing, is balanced by a sufficient risk reduction for people that don't know what they are doing. |
I'm happy for the foreseeable future about the linting plan we've arrived at. With a time machine I agree I'd just nuke @rfcbot reviewed Maybe in another 5 years we might revisit something like this again, but I don't think it's something we need to keep open. |
static mut
is almost impossible to use correctly, see rust-lang-nursery/lazy-static.rs#117 for an example in the widely-usedlazy-static
.You must be able to show that every borrow of the
static mut
is not reentrant (as opposed to regular interior mutability, which only requires reentrance-freedom when accessing data), which is almost entirely impossible in real-world scenarios.We have a chance at removing it from Rust2018 and force people to use a proper synchronization abstraction (e.g.
lazy_static!
+Mutex
), or in lieu of one,thread_local!
/scoped_thread_local!
.If they were using
static mut
with custom synchronization logic, they should do this:And then use
CustomSynchronizingAbstraction
with regularstatic
s, safely.This matches the "soundness boundary" of Rust APIs, whereas
static mut
is more like C.cc @RalfJung @rust-lang/compiler @rust-lang/lang
2023-08 Note from triage, many years later: there's now https://doc.rust-lang.org/1.71.0/std/cell/struct.SyncUnsafeCell.html in the library, added by #95438, which can be used instead of
static mut
. Butstatic mut
is not even soft-deprecated currently.The text was updated successfully, but these errors were encountered: