Add a lint for static items with large type alignment #8593

Jaic1 · 2022-03-27T09:58:58Z

The goal of this PR is to warn the usage of static items which have type alignment larger than page size (typically 4KB).

Due to some known unsound issues, the type alignment may not be fulfilled.
For more information, see rust-lang/rust#70022 and rust-lang/rust#70143.

changelog: add lint [static_items_large_align]

rust-highfive · 2022-03-27T09:59:00Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

Please see the contribution instructions for more information.

oli-obk · 2022-03-28T10:16:38Z

clippy_lints/src/static_items_large_align.rs

+        if_chain! {
+            if let ItemKind::Static(hir_ty, _, _) = item.kind;
+            let ty = hir_ty_to_ty(cx.tcx, hir_ty);
+            if let Some(adt_ref) = self.check_ty_alignment(cx.tcx, ty);


I did not think about references at all... that's a really good catch. I'm wondering if we'll miss anything with such an approach. I guess we could use the const validator to look at the actual allocations, but not sure we'll have other problems due to that.

Either way, your function looks good and we can add more types to recurse on later.

One thing your PR made me realize is that promoteds suffer from the same issue, but those can even be generic... not sure how to best handle them. Maybe with a mir visitor looking at the types of all promoteds. Doesn't have to be in this PR tho

I guess we could use the const validator to look at the actual allocations

Could you provide more context on const validator and allocations?

One thing your PR made me realize is that promoteds suffer from the same issue, but those can even be generic...

Your mention of generic reminds me to think more about it when collecting the spans... As for promoteds, I guess I will take a peek at const eval.

The actual memory as we will pass it to llvm can be fetched with https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.eval_static_initializer

This gives you access to an Allocation which has an align field. It also gives you a list of relocations, which are ids of other allocations referenced by this static (essentially pointers to other memory). You can then use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.get_global_alloc to fetch the allocations themselves and recurse. This will not give us any information about the spans or similar, but it will make sure we don't miss anything.

We could as a next step employ https://doc.rust-lang.org/nightly/nightly-rustc/rustc_const_eval/interpret/visitor/index.html but that is a bit more involved

oli-obk · 2022-03-28T10:18:11Z

clippy_lints/src/static_items_large_align.rs

+    /// 2. check if its (or its inner fields') alignment are larger than page size
+    /// 3. return one of them;
+    ///    otherwise return None
+    fn check_ty_alignment<'tcx>(&self, tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Option<AdtDef<'tcx>> {


I wonder if we can do a bit better on the spans and also collect the spans of all fields of intermediate structs. Not sure how to get a struct field's span though.

How about just report the intermediate types in mir but without the spans (from hir)? The difficult part in collecting the spans is dealing with the generics in hir.

A simple example:

#[repr(align(0x100000))] struct Aligned(u8); struct AlignedWrapper { f: u8, g: Aligned, } struct AlignedGeneric<T>(T); static XG: AlignedGeneric<AlignedWrapper> = ...;

Then the intermediate types in mir would be:

AlignedGeneric<AlignedWrapper> AlignedWrapper Aligned

But in hir we can only get:

AlignedGeneric<T>(T) // T needs to be resolved to AlignedWrapper // and it is not straightforward to do it in hir ...

As we get more involved in connecting the mir and hir (the solution I have been trying), it is likely the lint/checking would become unmanageable and be unsound (i.e., having false negative).

Here is a more involved example (as a reference):

#[repr(align(0x100000))] struct AlignedA(u8); #[repr(align(0x100000))] struct AlignedB(u8); struct FnPtr<T>(fn() -> Box<T>); struct AG<T>(T); type AGAlias<T> = AG<T>; struct AGWithArgs<A, B> { not_aligned: FnPtr<A>, aligned: AGAlias<B>, } static XG: AGWithArgs<AlignedA, AlignedB> = AGWithArgs { not_aligned: FnPtr(box_aligned), aligned: AG(AlignedB(0)), }; fn box_aligned() -> Box<AlignedA> { Box::new(AlignedA(0)) }

The report in mir would be straightforward (since it is more easy to mono mir types):

AGWithArgs<AlignedA, AlignedB> AG(AlignedB(0)) AlignedB

But it is difficult to report the relevant spans (from hir) like this:

struct AGWithArgs<A, B> { not_aligned: FnPtr<A>, aligned: AGAlias<B>, ^^^^^^^^^^^^^^^^^^^ } type AGAlias<T> = AG<T>; ^^^^^ struct AG<T>(T); ^^^ AlignedB

Or maybe there is a good way to work with (e.g., some subst mechanisms) generics in hir I am missing? And now I also start to wonder how rustc deal with generics when reporting (type) errors to user.

Yea, that seems reasonable. You can use tcx.def_span to get the span of the structs.

bors · 2022-03-30T17:15:30Z

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

oli-obk · 2022-04-01T11:27:17Z

All my comments are for follow up improvements. IMO we can merge this as is

bors · 2022-04-04T07:52:55Z

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

Jarcho · 2022-08-30T00:42:08Z

Ping @Jaic1. Are you willing to continue working on this? It doesn't seem like there are any major blockers for merging.

Jaic1 · 2022-08-30T04:25:14Z

@Jarcho Sure, I can resolve the conflicts at the moment. For the potential improvements mentioned above, maybe I can pick it up some day.

oli-obk · 2022-09-08T06:14:38Z

Please rebase instead of merge. When you rebase, the merge commit gets removed automatically without you needing to do anything

flip1995

There are a few changes left to do before we can merge this.

flip1995 · 2022-09-08T08:19:52Z

clippy_lints/src/static_items_large_align.rs

+    /// ```
+    #[clippy::version = "1.61.0"]
+    pub STATIC_ITEMS_LARGE_ALIGN,
+    pedantic,


I think this would be a good candidate for correctness or at least the suspicious group. Or do you have concerns about drastic false positives this lint might trigger?

flip1995 · 2022-09-08T08:20:30Z

clippy_lints/src/static_items_large_align.rs

+    ///     println!("{:#x}", Box::into_raw(b) as usize);
+    /// }
+    /// ```
+    #[clippy::version = "1.61.0"]


Suggested change

#[clippy::version = "1.61.0"]

#[clippy::version = "1.66.0"]

This will most likely be merged in the 1.66 cycle.

flip1995 · 2022-09-08T08:22:04Z

clippy_lints/src/static_items_large_align.rs

+    /// }
+    /// ```
+    #[clippy::version = "1.61.0"]
+    pub STATIC_ITEMS_LARGE_ALIGN,


The lint name sounds a bit weird to me. Maybe static_items_with_large_alignment but then again, that's a bit wordy... Maybe the current lint name is a good compromise.

flip1995 · 2022-09-08T08:25:09Z

clippy_lints/src/static_items_large_align.rs

+                    "this static item (itself or its subfield) has a type alignment,\n\
+                    which is larger than page size and may not be fulfilled,\n\
+                    for more information, see <https://github.com/rust-lang/rust/issues/70022>.",


Suggested change

"this static item (itself or its subfield) has a type alignment,\n\

which is larger than page size and may not be fulfilled,\n\

for more information, see <https://github.com/rust-lang/rust/issues/70022>.",

"static item with large type alignment",

The lint message is not meant to explain the whole world :P Clippy will add a link to its documentation to the first lint emission anyway, so adding links is not necessary.

flip1995 · 2022-09-08T08:29:20Z

clippy_lints/src/static_items_large_align.rs

+                "{} has alignment {:#x}, which is larger than {:#x},\n\
+                if you know what you are doing, config the default page size clippy uses in clippy.toml",


There shouldn't be any line breaks (or full stops) in a help message. The help message should just be: "{} has alignment {:#x}, which is larger than {:#x}"

The clippy.toml doesn't have to be mentioned here, because the config option will be listed in the lint documentation, which is linked to by the first lint emission.

flip1995 · 2022-09-08T08:29:49Z

clippy_lints/src/static_items_large_align.rs

+                        "{} with substitutions {:#?},\n\
+                        contains an inner type with large alignment",


Suggested change

"{} with substitutions {:#?},\n\

contains an inner type with large alignment",

"{} with substitutions {:#?}, contains an inner type with large alignment",

no newline.

flip1995 · 2022-09-08T08:30:23Z

clippy_lints/src/static_items_large_align.rs

+                if substs_ref.is_empty() {
+                    format!("{} contains an inner type with large alignment", ty.sort_string(cx.tcx))
+                } else {
+                    // TODO - can we use :#?


Can we? Do you have a test for this? This TODO comment should be resolved before merging.

flip1995 · 2022-09-08T08:35:18Z

clippy_lints/src/static_items_large_align.rs

+    /// 4. Check if its (or its inner fields') alignment are larger than page size.
+    /// 5. Return one of them;
+    ///     otherwise pop the current checked type and return `None`.
+    fn check_ty_alignment<'tcx>(


Doesn't rustc have a query that just gives you the type alignment? 🤔 Implementing type alignment calculation in Clippy feels a bit wrong to me.

bors · 2022-09-27T18:30:13Z

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

blyxyas · 2023-10-19T15:55:18Z

@Jaic1 I'm closing this for inactivity. It seems like you're not active in the OSS ecosystem anymore, so, have luck in whatever else you're doing! ❤️.

If you'd like to continue with this PR, don't hesitate to comment and I'll reopen it! Thanks for trying ~(=^‥^)/.

Edit: Similar to rust-lang/rust#116520

rust-highfive assigned flip1995 Mar 27, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 27, 2022

oli-obk reviewed Mar 28, 2022

View reviewed changes

Jaic1 force-pushed the static_items_large_align branch from 575cd03 to 69c07e0 Compare April 2, 2022 08:06

Jaic1 added 4 commits September 8, 2022 15:39

First submit

0f69824

Fix dogfood failure

216e89d

Report the intermediate spans

d07511e

Fix test failure

53d917b

Jaic1 force-pushed the static_items_large_align branch from 3476528 to 53d917b Compare September 8, 2022 07:47

flip1995 requested changes Sep 8, 2022

View reviewed changes

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 8, 2022

pnkfelix mentioned this pull request Dec 9, 2022

Locals aligned to greater than page size can cause unsound behavior rust-lang/rust#70143

Open

blyxyas closed this Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a lint for static items with large type alignment #8593

Add a lint for static items with large type alignment #8593

Jaic1 commented Mar 27, 2022

rust-highfive commented Mar 27, 2022

oli-obk Mar 28, 2022

Jaic1 Apr 1, 2022

oli-obk Apr 1, 2022

oli-obk Mar 28, 2022

Jaic1 Apr 1, 2022

oli-obk Apr 1, 2022

bors commented Mar 30, 2022

oli-obk commented Apr 1, 2022

bors commented Apr 4, 2022

Jarcho commented Aug 30, 2022

Jaic1 commented Aug 30, 2022

oli-obk commented Sep 8, 2022

flip1995 left a comment

flip1995 Sep 8, 2022

flip1995 Sep 8, 2022

flip1995 Sep 8, 2022

flip1995 Sep 8, 2022

flip1995 Sep 8, 2022

flip1995 Sep 8, 2022

flip1995 Sep 8, 2022

flip1995 Sep 8, 2022

bors commented Sep 27, 2022

blyxyas commented Oct 19, 2023 •

edited

Loading

		"{} has alignment {:#x}, which is larger than {:#x},\n\
		if you know what you are doing, config the default page size clippy uses in clippy.toml",

		"{} with substitutions {:#?},\n\
		contains an inner type with large alignment",

	"{} with substitutions {:#?},\n\
	contains an inner type with large alignment",
	"{} with substitutions {:#?}, contains an inner type with large alignment",

Add a lint for static items with large type alignment #8593

Add a lint for static items with large type alignment #8593

Conversation

Jaic1 commented Mar 27, 2022

rust-highfive commented Mar 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 30, 2022

oli-obk commented Apr 1, 2022

bors commented Apr 4, 2022

Jarcho commented Aug 30, 2022

Jaic1 commented Aug 30, 2022

oli-obk commented Sep 8, 2022

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Sep 27, 2022

blyxyas commented Oct 19, 2023 • edited Loading

blyxyas commented Oct 19, 2023 •

edited

Loading