Prohibit cycles behind references while static initialization #149973
Conversation
rustbot
added
S-waiting-on-review
| if !tcx.is_foreign_item(static_def_id) { | ||
| // Fire the query to detect cycles. We cannot restrict this to only when | ||
| // evaluating statics, since static reference cycles can also be formed | ||
| // through consts, especially promoted ones. |
There was a problem hiding this comment.
Not skiping firing the query when machine.static_root_ids.is_none() here further prohibits some currently compiled codes such as
static FOO: i32 = {
let x = &BAR;
42
};
const BAR: i32 = {
let x = &FOO;
42
};but I guess we might have to refute const cases as well because the following codes slightly modified from the original issue still make problems with it:
enum Never {}
// &&X creates a promoted const
static X: &Never = weird(&&X);
const fn weird(a: &&&Never) -> &'static Never {
// SAFETY: our argument type has an unsatisfiable
// library invariant; therefore, this code is unreachable.
unsafe { std::hint::unreachable_unchecked() };
}// for privacy
mod mrow {
pub struct InBoundsIndex<const N: usize>(());
impl<const N: usize> InBoundsIndex<N> {
pub const fn new() -> Option<InBoundsIndex<N>> {
if N < 32 {
Some(Self(()))
} else {
None
}
}
}
}
use mrow::InBoundsIndex;
static IDX: InBoundsIndex<64> = {
FOO;
// THIS SHOULD PANIC!!! but we do UB first on the line above
InBoundsIndex::<64>::new().unwrap()
};
const FOO: () = {
let _x = index([0; 32], &IDX);
};
const fn index<const N: usize>(arr: [u8; 32], _: &InBoundsIndex<N>) -> u8 {
// SAFETY: InBoundsIndex can only be created by its new, which ensures N is < 32
unsafe { arr.as_ptr().add(N).read() }
}
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Oh, this already breaks things as hell like in: static POSTFIX: [(Input, Action); 10] = [
(Keyword("as"), SetState(&[(ExpectType, SetState(&POSTFIX))])),
... |
traviscross
added
the
I-lang-radar
|
@bors try |
Prohibit cycles behind references while static initialization
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 352f3f3 failed: CI. Failed jobs:
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| self.layout_of_local(self.frame(), local, None)?.ty.is_ref() | ||
| } else { | ||
| false | ||
| }; |
There was a problem hiding this comment.
This looks like it will be false for &(*local_ref).field, which is not what we want.
Generally, I am not convinced by the implementation approach. This adds a very syntactic check to one specific operation that constructs references, but it's far from clear that this is enough. For instance, the code could take a raw pointer to a static, store it in a [*const T; 1], and transmute that to [&T; 1] -- then you would never see a (re)borrow of a reference but we still have the same underlying unsoundness.
There was a problem hiding this comment.
For instance,
I guess we'd say that the unsafe code needed to construct references this way is unsound... hm. Maybe that is fair but it needs good documentation.
Also, what you implemented is somewhat different from what I sketched, not sure if that is deliberate. You seem to be trying to catch all &expr where expr evaluates to a pointer that points to a static. The original plan was just to catch all &STATIC, i.e. it really syntactically has to have that shape.
In particular, your approach would seem to reject this:
static X: &Never = weird(unsafe { &*&raw const X });I'm not sure that's a good idea. It's unprincipled: it's still possible to write unsafe code that evades your checks, so yo haven't gained any new guarantees. I would rather have a very clear line for which code gets accepted and which gets rejected.
There was a problem hiding this comment.
Yeah, I’m not a fan of this implementation either (I’ll elaborate a bit more in my next comment 😅).
But this doesn't reject static X: &Never = weird(unsafe { &*&raw const X }); because is_reborrow_of_ref is false for that case.
But everything else is exactly as you described - e.g., field projections - so I’ll try the other approach.
| // - they may be references to some other legitimate static reference | ||
| // (e.g. via a raw pointer), and | ||
| // - if they originate from an illegal static reference, that illegal | ||
| // reference must already appear in the body and will be checked there. |
There was a problem hiding this comment.
I don't quite understand what you mean by this. However, it all seems an artifact of trying to catch cases where the reference points to a static, rather than just the specific syntactic pattern &STATIC.
|
Also I should say, thanks for tackling this. :) |
|
Thanks for the detailed feedback! Do you think this would be better implemented somewhere else, like in a late lint pass or during MIR construction? BTW, since it looks like I need to implement the non-denying lint first for FCW - and triggering a query cycle won’t work for that - I’ll have to explore a different approach anyway 😅 |
rustbot
added
S-waiting-on-author
CTFE gives you all the syntactic information that exists in MIR. ;) And strangely, even if the user wrote I think we'll have to change that. To make it easier for later stages of the compiler to distinguish ref-to-static from raw-ptr-to-static, we should adjust the type of the const we generate for that (during MIR building) accordingly.
Yes, I was wondering what your plans were here.^^ I honestly don't have any immediately good ideas. |
Yeah, I encountered this and most of the fuss above started from trying to discern those two cases.
You're right. I'll try working on both parts 😄
I'm thinking of something like the following for now 🤔
But maybe I could find a better way 😅 |
|
Some other cases that needs to be detected and prohibited, if they aren't handled already: struct Weird(&'static Weird);
static X: Weird = {
let ref a = X;
Weird(a)
};struct Weird(&'static Weird);
impl Weird {
const fn by_ref(&self) -> &Self { self }
}
static X: Weird = Weird(X.by_ref()); // autorefstruct Weird(&'static Weird);
static X: (Weird, i32) = (Weird(&X.0), 1); // reference to part of the static |
|
I'm not sure on adjustments into raw ptr cases: enum Never {}
static X: &Never = weird(&X);
const fn weird(a: *const &Never) -> &'static Never {
todo!("Reachable, perhaps?")
} |
|
I think we should reject that, "&X" is a reference after all.
|
|
Another case to be prohibited: struct Weird(&'static Weird);
static X: Weird = 'a: {
match X {
y if break 'a Weird(&y) => {},
_ => {},
};
unreachable!()
}; |
7 participants
Fixes #143047