-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Taint the type of ill-formed (unsized) statics #144226
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
Conversation
r? @davidtwco rustbot has assigned @davidtwco. Use |
Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri This PR changes a file inside Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE machinery |
@@ -243,6 +243,17 @@ impl<'tcx, Prov: Provenance> std::ops::Deref for ImmTy<'tcx, Prov> { | |||
} | |||
|
|||
impl<'tcx, Prov: Provenance> ImmTy<'tcx, Prov> { | |||
#[inline(always)] | |||
pub fn try_from_immediate(imm: Immediate<Prov>, layout: TyAndLayout<'tcx>) -> Option<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of these functions. The point of the assertions is that they are a last line of defense to detect defective callers. They are not exhaustive checks. If the caller can't ensure that the value has the right type, that can only be fixed in the caller.
IOW, matches_abi
here really is more of a maybe_matches_abi
. It is necessary, but not sufficient. And trying to make it sufficient is the wrong approach; the right approach is figuring out why someone is feeding bogus data into these functions.
|
||
use std::fmt::Debug; | ||
|
||
static STATIC_1: dyn Debug + Sync = *(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance, this is a clearly ill-formed static. Nothing should ever look at its MIR. Trying to make the MIR interpreter APIs resistant against bogus MIR is a pointless game of whack-a-mole.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea those cases are straight forward to prevent within type_of
, we don't even need to compute the layout, just doing a sizedness check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't quite work since we allow extern statics that have extern types, which are unsized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we can check the tail manually for slices and dyn trait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that requires unfolding the type which will trigger the same cycle error, won't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These all seem to be standard cases of "ill-formed MIR gets too far into the pipeline". We should never run any MIR passes on ill-formed MIR; it is a waste of effort to try and fix this inside every individual pass.
I agree, but fixing this is general is hard too. We have an A "cleaner" way would be to ensure we never produce a constant with mismatching ABI, be it as a |
These are statics failing the "it must be sized" check. It's not some impossible where bound. I don't see what is so hard about marking them as tainted so nothing else ever touches them again. It's the exact same thing as a static whose initializer body is plainly ill-typed. |
7e54047
to
a825a96
Compare
@RalfJung this latest version prevents creating the erroneous constants altogether. Do you agree with this version? |
This comment has been minimized.
This comment has been minimized.
a825a96
to
a250112
Compare
This comment has been minimized.
This comment has been minimized.
Yeah, that looks much better, thanks. I'm not familiar with MIR building so I can't approve this on my own, however. |
It's not ready yet. Just checking sizedness is not enough, as some non-sized types are OK too, like extern types. And using layout causes cycles. |
Extern types can only work with extern statics, right? And even then, that seems kind of cursed. Did we intentionally allow that or just forget to disallow it? |
Or inside pointers. And pointers to extern types are scalars.
It's intentional. We have a run-make test to verify this works. |
Pointers to extern types are sized so there is no problem with them, those should just work.
|
The failing test is this one: rust/tests/run-make/static-extern-type/use-foo.rs Lines 3 to 7 in a7a1618
As a foreign type Checking whether |
Yeah, that is a very odd test, I didn't think we'd allow extern statics with unknown size. Interestingly, we reject What is not entirely clear to me is why we'd care about whether |
All the issues referenced here seem to be about statics that are unsized-with-metadata. You can check for this without knowing the layout; just call This still has to normalize some things though, so that coroutine case could become interesting. But it's okay for that code to error, it just can't ICE. |
// Still, producing a single scalar constant would be inconsistent, as pointers to | ||
// non-`Sized` types are scalar pairs. Avoid an ICE by producing an error constant. | ||
let guar = | ||
tcx.dcx().span_delayed_bug(span, format!("static's type `{ty}` is not Sized")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tcx.dcx().span_delayed_bug(span, format!("static's type `{ty}` is not Sized")); | |
tcx.dcx().span_delayed_bug(span, format!("static's type `{pointee}` is not Sized")); |
Wouldn't an alternative be to change the type of the static itself to Cc @oli-obk |
a250112
to
1a23d74
Compare
1a23d74
to
8095c2d
Compare
This PR modifies |
The latest commit tries that. I think it's worse because it breaks passing tests. |
This comment was marked as outdated.
This comment was marked as outdated.
8095c2d
to
7c64961
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... I don't think I understand why this fails, but I guess that's how type_alias_impl_trait works...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, if we do use the "reveal opaque type to check the auto trait" logic, then this must cycle if we do it as a part of type_of. But if you are using a TAIT as the type of a static then it's not unreasonable to mark it as Sync, too.
Of course we could keep the Sync check out of the type_of code path and check it only in wfcheck like we did before this PR, but that'll be up to the lang team I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. Yeah Sync
can't influence layout so it's much less likely to ICE later parts of the compiler... but I'm fine either way.
@@ -17,6 +17,7 @@ impl<F: Future> Task<F> { | |||
} | |||
|
|||
pub type F = impl Future; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub type F = impl Future; | |
pub type F = impl Future + Sync; |
Needing an extra annotation for an opaque type in a static item seems fine to me
@@ -23,5 +22,6 @@ mod helper { | |||
|
|||
// Static queries the layout of the coroutine. | |||
static A: Option<helper::F> = None; | |||
//~^ ERROR cycle detected when computing type of `A` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here. Open an issue that this cycle diagnostic should propose adding the bound to the opaque. We'll fix that for TAITs but that shouldn't stop your PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, if we do use the "reveal opaque type to check the auto trait" logic, then this must cycle if we do it as a part of type_of. But if you are using a TAIT as the type of a static then it's not unreasonable to mark it as Sync, too.
Of course we could keep the Sync check out of the type_of code path and check it only in wfcheck like we did before this PR, but that'll be up to the lang team I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor comment nits, I can imagine this being quite confusing for someone who doesn't have the context we do right now.
@bors r-
(But we already got rolled up so this may have to be resolved in a follow-up PR.)
// Verify that here to avoid ill-formed MIR. | ||
match check_static_item(tcx, def_id, ty, false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Verify that here to avoid ill-formed MIR. | |
match check_static_item(tcx, def_id, ty, false) { | |
// Verify that here to avoid ill-formed MIR. | |
// We skip the `Sync` check to avoid cycles for type-alias-impl-trait, | |
// relying on the fact that non-Sync statics don't ICE the rest of the compiler. | |
match check_static_item(tcx, def_id, ty, /* should_check_for_sync */ false) { |
// Verify that here to avoid ill-formed MIR. | ||
match check_static_item(tcx, def_id, ty, false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Verify that here to avoid ill-formed MIR. | |
match check_static_item(tcx, def_id, ty, false) { | |
// Verify that here to avoid ill-formed MIR. | |
// We skip the `Sync` check to avoid cycles for type-alias-impl-trait, | |
// relying on the fact that non-Sync statics don't ICE the rest of the compiler. | |
match check_static_item(tcx, def_id, ty, /* should_check_for_sync */ false) { |
@@ -767,7 +767,8 @@ pub(crate) fn check_item_type(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Result<(), | |||
DefKind::Static { .. } => { | |||
check_static_inhabited(tcx, def_id); | |||
check_static_linkage(tcx, def_id); | |||
res = res.and(wfcheck::check_static_item(tcx, def_id)); | |||
let ty = tcx.type_of(def_id).instantiate_identity(); | |||
res = res.and(wfcheck::check_static_item(tcx, def_id, ty, true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res = res.and(wfcheck::check_static_item(tcx, def_id, ty, true)); | |
res = res.and(wfcheck::check_static_item(tcx, def_id, ty, /* should_check_for_sync */ true)); |
Rollup of 4 pull requests Successful merges: - #144226 (Do not assert layout in KnownPanicsLint.) - #144385 (Optimize performance by inline in macro hygiene system) - #144454 (move uefi test to run-make) - #144455 (Unify LLVM ctlz/cttz intrinsic generation) r? `@ghost` `@rustbot` modify labels: rollup
I'll submit a PR with the comments. |
…compiler-errors check_static_item: explain should_check_for_sync choices Follow-up to rust-lang#144226. r? `@oli-obk`
…compiler-errors check_static_item: explain should_check_for_sync choices Follow-up to rust-lang#144226. r? ``@oli-obk``
Fixes #121176
Fixes #129109
Fixes #130970
Fixes #131347
Fixes #139872
Fixes #140332