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 certain suspicious trait object usage #264

Merged
merged 2 commits into from
Mar 23, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
11 changes: 11 additions & 0 deletions doc/src/config-lints.md
Original file line number Diff line number Diff line change
Expand Up @@ -230,3 +230,14 @@ impl std::marker::Unpin for Foo {}
```

As a workaround, in most cases, you should be able to use [`std::panic::AssertUnwindSafe`](https://doc.rust-lang.org/nightly/std/panic/struct.AssertUnwindSafe.html) instead of implementing one of the `UnwindSafe` traits, and Boxing your type can usually work around the need for `Unpin` (which should be rare in non-`async` code anyway).

### `plrust_suspicious_trait_object`

This lint forbids trait object use in turbofish and generic defaults. This is an effort to fix [certain soundness holes](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=764d78856996e1985ee88819b013c645) in the Rust language. More simply, the following patterns are disallowed:

```rs
// Trait object in turbofish
foo::<dyn SomeTrait>();
// Trait object in type default (enum, union, trait, and so on are all also forbidden)
struct SomeStruct<T = dyn SomeTrait>(...);
```
1 change: 1 addition & 0 deletions plrust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ const DEFAULT_LINTS: &'static str = "\
plrust_external_mod, \
plrust_print_macros, \
plrust_stdio, \
plrust_suspicious_trait_object, \
unsafe_code, \
deprecated, \
suspicious_auto_trait_impls, \
Expand Down
61 changes: 61 additions & 0 deletions plrustc/plrustc/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,65 @@ impl<'tcx> LateLintPass<'tcx> for PlrustAutoTraitImpls {
}
}

declare_plrust_lint!(
pub(crate) PLRUST_SUSPICIOUS_TRAIT_OBJECT,
"Disallow suspicious generic use of trait objects",
);

declare_lint_pass!(PlrustSuspiciousTraitObject => [PLRUST_SUSPICIOUS_TRAIT_OBJECT]);

impl<'tcx> LateLintPass<'tcx> for PlrustSuspiciousTraitObject {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &hir::Expr) {
let path_segments = match &expr.kind {
hir::ExprKind::Path(hir::QPath::Resolved(_, path)) => path.segments,
hir::ExprKind::Path(hir::QPath::TypeRelative(_, segment, ..))
| hir::ExprKind::MethodCall(segment, ..) => std::slice::from_ref(*segment),
_ => return,
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
};
for segment in path_segments {
let Some(args) = segment.args else {
continue;
};
for arg in args.args {
let hir::GenericArg::Type(ty) = arg else {
continue;
};
if let hir::TyKind::TraitObject(..) = &ty.kind {
cx.lint(
PLRUST_SUSPICIOUS_TRAIT_OBJECT,
"trait objects in turbofish are forbidden",
|b| b.set_span(expr.span),
);
}
}
}
}

fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) {
let generics = match &item.kind {
hir::ItemKind::TyAlias(_, generics) => *generics,
hir::ItemKind::Enum(_, generics) => *generics,
hir::ItemKind::Struct(_, generics) => *generics,
hir::ItemKind::Union(_, generics) => *generics,
hir::ItemKind::Trait(_, _, generics, ..) => *generics,
hir::ItemKind::Fn(_, generics, ..) => *generics,
_ => return,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remark as before.

Though, I'm surprised you can't leverage OpaqueTy somehow to induce this problem. Or can you and we already check that?

Copy link
Contributor Author

@thomcc thomcc Mar 22, 2023

Choose a reason for hiding this comment

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

OpaqueTy is unstable, it's TAIT stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather, using OpaqueTy in a turbofish or default type requires TAIT.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhh, I misread rustc's docs and lost the type Foo = part, thought for some reason that all impl Trait goes through it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh oh oh, so wait... wait... OpaqueTy is a threat but it's currently syntactically inaccessible in this position, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually not sure that struct/enum/type/... are a threat in the first place. They're easy to check for and it feels like I almost got it working, so I figured I'd check for them.

But yeah, TraitAlias is probably in the same position. I could put it here too, but this whole thing probably needs rethinking when those stabilize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've filed #270 about this kind of thing.

};
for param in generics.params {
let hir::GenericParamKind::Type { default: Some(ty), .. } = &param.kind else {
continue;
};
if let hir::TyKind::TraitObject(..) = &ty.kind {
cx.lint(
PLRUST_SUSPICIOUS_TRAIT_OBJECT,
"trait objects in generic defaults are forbidden",
|b| b.set_span(item.span),
);
}
}
}
}

declare_plrust_lint!(
pub(crate) PLRUST_LIFETIME_PARAMETERIZED_TRAITS,
"Disallow lifetime parameterized traits"
Expand Down Expand Up @@ -544,6 +603,7 @@ static PLRUST_LINTS: Lazy<Vec<&'static Lint>> = Lazy::new(|| {
PLRUST_LIFETIME_PARAMETERIZED_TRAITS,
PLRUST_PRINT_MACROS,
PLRUST_STDIO,
PLRUST_SUSPICIOUS_TRAIT_OBJECT,
];
if *INCLUDE_TEST_ONLY_LINTS {
let test_only_lints = [PLRUST_TEST_ONLY_FORCE_ICE];
Expand Down Expand Up @@ -579,6 +639,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(PlrustSuspiciousTraitObject));
store.register_late_pass(move |_| Box::new(PlrustAutoTraitImpls));
store.register_late_pass(move |_| Box::new(PlrustFnPointer));
store.register_late_pass(move |_| Box::new(PlrustLeaky));
Expand Down
34 changes: 34 additions & 0 deletions plrustc/plrustc/uitests/sus_trait_obj_items.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#![crate_type = "lib"]

pub trait Foo {}

pub trait Bar<T = dyn Foo>
where
T: ?Sized,
{
}

#[allow(invalid_type_param_default)] // not the lint we're interested in testing
pub fn sus_fn<T = dyn Foo>()
where
T: ?Sized,
{
}

pub struct SusStruct<T = dyn Foo>(pub Box<T>)
where
T: ?Sized;

pub enum SusEnum<T = dyn Foo>
where
T: ?Sized,
{
Something(Box<T>),
}

pub union SusUnion<T = dyn Foo>
where
T: ?Sized,
{
pub something: *const T,
}
54 changes: 54 additions & 0 deletions plrustc/plrustc/uitests/sus_trait_obj_items.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
error: trait objects in generic defaults are forbidden
--> $DIR/sus_trait_obj_items.rs:5:1
|
LL | / pub trait Bar<T = dyn Foo>
LL | | where
LL | | T: ?Sized,
LL | | {
LL | | }
| |_^
|
= note: `-F plrust-suspicious-trait-object` implied by `-F plrust-lints`

error: trait objects in generic defaults are forbidden
--> $DIR/sus_trait_obj_items.rs:12:1
|
LL | / pub fn sus_fn<T = dyn Foo>()
LL | | where
LL | | T: ?Sized,
LL | | {
LL | | }
| |_^

error: trait objects in generic defaults are forbidden
--> $DIR/sus_trait_obj_items.rs:18:1
|
LL | / pub struct SusStruct<T = dyn Foo>(pub Box<T>)
LL | | where
LL | | T: ?Sized;
| |______________^

error: trait objects in generic defaults are forbidden
--> $DIR/sus_trait_obj_items.rs:22:1
|
LL | / pub enum SusEnum<T = dyn Foo>
LL | | where
LL | | T: ?Sized,
LL | | {
LL | | Something(Box<T>),
LL | | }
| |_^

error: trait objects in generic defaults are forbidden
--> $DIR/sus_trait_obj_items.rs:29:1
|
LL | / pub union SusUnion<T = dyn Foo>
LL | | where
LL | | T: ?Sized,
LL | | {
LL | | pub something: *const T,
LL | | }
| |_^

error: aborting due to 5 previous errors

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

trait Object<U> {
type Output;
}

impl<T: ?Sized, U> Object<U> for T {
type Output = U;
}

fn foo<T: ?Sized, U>(x: <T as Object<U>>::Output) -> U {
x
}

pub fn transmute<T, U>(x: T) -> U {
foo::<dyn Object<U, Output = T>, U>(x)
}
10 changes: 10 additions & 0 deletions plrustc/plrustc/uitests/sus_trait_obj_transmute.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: trait objects in turbofish are forbidden
--> $DIR/sus_trait_obj_transmute.rs:16:5
|
LL | foo::<dyn Object<U, Output = T>, U>(x)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-F plrust-suspicious-trait-object` implied by `-F plrust-lints`

error: aborting due to previous error