Skip to content
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

Add a lint for 'static impl shenanigans #265

Merged
merged 5 commits into from
Mar 23, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions doc/src/config-lints.md
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,23 @@ std::io::stderr().write_all(b"foobar").unwrap();
let _stdin_is_forbidden_too = std::io::stdin();
```

### `plrust_static_impls`

This lint forbids certain `impl` blocks for types containing `&'static` references. The precise details are somewhat obscure, but can usually be avoided by making a custom struct to contain your static reference, which avoids the particular soundness hole we're concerned with. For example:

```rust
// This is forbidden:
impl SomeTrait for (&'static Foo, Bar) {
// ...
}

// Instead, do this:
struct MyType(&'static Foo, Bar);
impl SomeTrait for MyType {
// ...
}
```

### `plrust_autotrait_impls`

This lint forbids explicit implementations of the safe auto traits, as a workaround for various soundness holes around these. It may be relaxed in the future if those are fixed.
Expand Down
1 change: 1 addition & 0 deletions plrust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ const DEFAULT_LINTS: &'static str = "\
plrust_lifetime_parameterized_traits, \
implied_bounds_entailment, \
plrust_autotrait_impls, \
plrust_static_impls, \
plrust_fn_pointers, \
plrust_filesystem_macros, \
plrust_env_macros, \
Expand Down
90 changes: 90 additions & 0 deletions plrustc/plrustc/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,94 @@ impl<'tcx> LateLintPass<'tcx> for NoExternBlockPass {
}
}

declare_plrust_lint!(
pub(crate) PLRUST_STATIC_IMPLS,
"Disallow impl blocks for types containing `'static` references"
);

declare_lint_pass!(PlrustStaticImpls => [PLRUST_STATIC_IMPLS]);

impl<'tcx> LateLintPass<'tcx> for PlrustStaticImpls {
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) {
let hir::ItemKind::Impl(imp) = &item.kind else {
return;
};
if self.has_static(imp.self_ty) {
cx.lint(
PLRUST_STATIC_IMPLS,
"`impl` blocks for types containing `'static` references are not allowed",
|b| b.set_span(imp.self_ty.span),
)
}
}
}

impl PlrustStaticImpls {
/// This is pretty naïve and designed to match the specific patterns that
/// trip up https://github.com/rust-lang/rust/issues/104005...
///
/// Also, I feel like I should be able to use `hir::intravisit::walk_ty`
/// here instead, but it doesn't seem to let be know the lifetime of refs,
/// so... not sure...
///
/// It's a method on `self` mostly to discourage use in other contexts,
/// since it's probably wrong for them.
fn has_static(&self, t: &hir::Ty) -> bool {
use hir::{Lifetime, LifetimeName::Static, MutTy, TyKind};
match &t.kind {
TyKind::Infer
| TyKind::Err
// Doesn't exist
| TyKind::Typeof(..)
// Not strictly correct but we forbid this elsewhere anyway.
| TyKind::BareFn(..)
// TAIT stuff, still unstable at the moment, seems very hard to
// prevent this for...
| TyKind::OpaqueDef(..)
| TyKind::Never => false,
// Found one!
TyKind::Rptr(Lifetime { res: Static, .. }, _) | TyKind::TraitObject(_, Lifetime { res: Static, .. }, _) => true,
// Need to keep looking.
TyKind::Rptr(_, MutTy { ty, .. })
Comment on lines +111 to +113
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Rptr" is a very confusing way for rustc to say "reference", honestly.

| TyKind::Ptr(MutTy { ty, .. })
| TyKind::Array(ty, _)
| TyKind::Slice(ty) => self.has_static(*ty),

TyKind::Tup(types) => types.iter().any(|t| self.has_static(t)),

TyKind::TraitObject(polytrait, ..) => {
polytrait.iter().any(|poly| {
self.segments_have_static(poly.trait_ref.path.segments)
})
}
// Something like `Vec<T>` or `Option<T>`. Need to look inside...
TyKind::Path(qpath) => {
let segs = match qpath {
hir::QPath::Resolved(Some(maybe_ty), _) if self.has_static(maybe_ty) => return true,
hir::QPath::TypeRelative(t, _) if self.has_static(*t) => return true,
hir::QPath::LangItem(..) => return false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is this why you mentioned the Box<Option<&'static T>> thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but I think it only happens if the path refers to a function.

hir::QPath::Resolved(_, path) => path.segments,
hir::QPath::TypeRelative(_, seg) => std::slice::from_ref(*seg),
};
self.segments_have_static(segs)
}
}
}

fn segments_have_static(&self, segs: &[hir::PathSegment]) -> bool {
segs.iter().any(|seg| {
seg.args().args.iter().any(|arg| match arg {
hir::GenericArg::Lifetime(hir::Lifetime {
res: hir::LifetimeName::Static,
..
}) => true,
hir::GenericArg::Type(t) => self.has_static(t),
_ => false,
thomcc marked this conversation as resolved.
Show resolved Hide resolved
})
})
}
}

declare_plrust_lint!(
pub(crate) PLRUST_AUTOTRAIT_IMPLS,
"Disallow impls of auto traits",
Expand Down Expand Up @@ -535,6 +623,7 @@ static PLRUST_LINTS: Lazy<Vec<&'static Lint>> = Lazy::new(|| {
let mut v = vec![
PLRUST_ASYNC,
PLRUST_AUTOTRAIT_IMPLS,
PLRUST_STATIC_IMPLS,
PLRUST_EXTERN_BLOCKS,
PLRUST_EXTERNAL_MOD,
PLRUST_FILESYSTEM_MACROS,
Expand Down Expand Up @@ -580,6 +669,7 @@ pub fn register(store: &mut LintStore, _sess: &Session) {
store.register_early_pass(move || Box::new(PlrustAsync));
store.register_early_pass(move || Box::new(PlrustExternalMod));
store.register_late_pass(move |_| Box::new(PlrustAutoTraitImpls));
store.register_late_pass(move |_| Box::new(PlrustStaticImpls));
store.register_late_pass(move |_| Box::new(PlrustFnPointer));
store.register_late_pass(move |_| Box::new(PlrustLeaky));
store.register_late_pass(move |_| Box::new(PlrustBuiltinMacros));
Expand Down
18 changes: 18 additions & 0 deletions plrustc/plrustc/uitests/static_impl_etc.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#![crate_type = "lib"]

trait Foo {}
struct Bar<A, B>(core::marker::PhantomData<(A, B)>);
struct Baz<'a, A>(core::marker::PhantomData<&'a A>);
trait Quux<A> {}

impl<T> Foo for &'static T {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these are generic because technically this is only (known to be) unsound in a case involving generics. I have not been able to trigger the unsound situation in cases with concrete types.

That said, given that we unload functions in create or replace situations, doing too much of anything with statics isn't exactly ideal, and just because I couldn't repro it doesn't mean it can't be done.

impl<T> Foo for &'static mut T {}
impl<T> Foo for [&'static T] {}
impl<T> Foo for &[&'static T] {}
impl<T> Foo for (i32, [&'static T]) {}
impl<T> Foo for (i32, [&'static T; 1]) {}
impl<T> Foo for *const &'static T {}
impl<T> Foo for Bar<i32, &'static T> {}
impl<T> Foo for Baz<'static, T> {}
impl<T> Foo for dyn Quux<&'static T> {}
impl<T> Foo for &'static dyn Quux<T> {}
70 changes: 70 additions & 0 deletions plrustc/plrustc/uitests/static_impl_etc.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
error: `impl` blocks for types containing `'static` references are not allowed
--> $DIR/static_impl_etc.rs:8:17
|
LL | impl<T> Foo for &'static T {}
| ^^^^^^^^^^
|
= note: `-F plrust-static-impls` implied by `-F plrust-lints`

error: `impl` blocks for types containing `'static` references are not allowed
--> $DIR/static_impl_etc.rs:9:17
|
LL | impl<T> Foo for &'static mut T {}
| ^^^^^^^^^^^^^^

error: `impl` blocks for types containing `'static` references are not allowed
--> $DIR/static_impl_etc.rs:10:17
|
LL | impl<T> Foo for [&'static T] {}
| ^^^^^^^^^^^^

error: `impl` blocks for types containing `'static` references are not allowed
--> $DIR/static_impl_etc.rs:11:17
|
LL | impl<T> Foo for &[&'static T] {}
| ^^^^^^^^^^^^^

error: `impl` blocks for types containing `'static` references are not allowed
--> $DIR/static_impl_etc.rs:12:17
|
LL | impl<T> Foo for (i32, [&'static T]) {}
| ^^^^^^^^^^^^^^^^^^^

error: `impl` blocks for types containing `'static` references are not allowed
--> $DIR/static_impl_etc.rs:13:17
|
LL | impl<T> Foo for (i32, [&'static T; 1]) {}
| ^^^^^^^^^^^^^^^^^^^^^^

error: `impl` blocks for types containing `'static` references are not allowed
--> $DIR/static_impl_etc.rs:14:17
|
LL | impl<T> Foo for *const &'static T {}
| ^^^^^^^^^^^^^^^^^

error: `impl` blocks for types containing `'static` references are not allowed
--> $DIR/static_impl_etc.rs:15:17
|
LL | impl<T> Foo for Bar<i32, &'static T> {}
| ^^^^^^^^^^^^^^^^^^^^

error: `impl` blocks for types containing `'static` references are not allowed
--> $DIR/static_impl_etc.rs:16:17
|
LL | impl<T> Foo for Baz<'static, T> {}
| ^^^^^^^^^^^^^^^

error: `impl` blocks for types containing `'static` references are not allowed
--> $DIR/static_impl_etc.rs:17:17
|
LL | impl<T> Foo for dyn Quux<&'static T> {}
| ^^^^^^^^^^^^^^^^^^^^

error: `impl` blocks for types containing `'static` references are not allowed
--> $DIR/static_impl_etc.rs:18:17
|
LL | impl<T> Foo for &'static dyn Quux<T> {}
| ^^^^^^^^^^^^^^^^^^^^

error: aborting due to 11 previous errors
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that there are 11 impl lines in the test file, and we want each of them to trigger.


27 changes: 27 additions & 0 deletions plrustc/plrustc/uitests/static_impl_unsound.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#![crate_type = "lib"]

use std::fmt::Display;

trait Displayable {
fn display(self) -> Box<dyn Display>;
}

// This is more complex than the one in the issue, to make sure the `Box`'s
// lang_item status doesn't bite us.
impl<T: Display> Displayable for (T, Box<Option<&'static T>>) {
Comment on lines +9 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is making me wonder: Box<T> is a lang item, but more than that, it is #[fundamental]. Should we have any special concerns about other #[fundamental] types here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not as far as I know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more concrete, we are looking somewhat blindly for 'static through almost everything, so fundamental or not, it doesn't make a huge difference.

fn display(self) -> Box<dyn Display> {
Box::new(self.0)
}
}

fn extend_lt<T, U>(val: T) -> Box<dyn Display>
where
(T, Box<Option<U>>): Displayable,
{
Displayable::display((val, Box::new(None)))
}

pub fn get_garbage(s: &str) -> String {
let val = extend_lt(&String::from(s));
val.to_string()
}
10 changes: 10 additions & 0 deletions plrustc/plrustc/uitests/static_impl_unsound.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: `impl` blocks for types containing `'static` references are not allowed
--> $DIR/static_impl_unsound.rs:11:34
|
LL | impl<T: Display> Displayable for (T, Box<Option<&'static T>>) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-F plrust-static-impls` implied by `-F plrust-lints`

error: aborting due to previous error