diff --git a/CHANGELOG.md b/CHANGELOG.md index 58ea0f9ab9d..df3fa2866c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2898,6 +2898,7 @@ Released 2018-09-13 [`no_effect`]: https://rust-lang.github.io/rust-clippy/master/index.html#no_effect [`non_ascii_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_ascii_literal [`non_octal_unix_permissions`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_octal_unix_permissions +[`non_send_field_in_send_ty`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_send_field_in_send_ty [`nonminimal_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool [`nonsensical_open_options`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonsensical_open_options [`nonstandard_macro_braces`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonstandard_macro_braces diff --git a/clippy_lints/src/lib.mods.rs b/clippy_lints/src/lib.mods.rs index 2718604f905..1d9d2be3443 100644 --- a/clippy_lints/src/lib.mods.rs +++ b/clippy_lints/src/lib.mods.rs @@ -149,6 +149,7 @@ mod no_effect; mod non_copy_const; mod non_expressive_names; mod non_octal_unix_permissions; +mod non_send_field_in_send_ty; mod nonstandard_macro_braces; mod open_options; mod option_env_unwrap; diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 1ad27870b1a..b1cd5aafe4c 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -366,6 +366,7 @@ store.register_lints(&[ non_expressive_names::MANY_SINGLE_CHAR_NAMES, non_expressive_names::SIMILAR_NAMES, non_octal_unix_permissions::NON_OCTAL_UNIX_PERMISSIONS, + non_send_field_in_send_ty::NON_SEND_FIELD_IN_SEND_TY, nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES, open_options::NONSENSICAL_OPEN_OPTIONS, option_env_unwrap::OPTION_ENV_UNWRAP, diff --git a/clippy_lints/src/lib.register_nursery.rs b/clippy_lints/src/lib.register_nursery.rs index b082f577a52..56b4f9d2391 100644 --- a/clippy_lints/src/lib.register_nursery.rs +++ b/clippy_lints/src/lib.register_nursery.rs @@ -16,6 +16,7 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![ LintId::of(missing_const_for_fn::MISSING_CONST_FOR_FN), LintId::of(mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL), LintId::of(mutex_atomic::MUTEX_INTEGER), + LintId::of(non_send_field_in_send_ty::NON_SEND_FIELD_IN_SEND_TY), LintId::of(nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES), LintId::of(option_if_let_else::OPTION_IF_LET_ELSE), LintId::of(path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 4e21b03217d..93b93bcba48 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -535,6 +535,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move || Box::new(feature_name::FeatureName)); store.register_late_pass(move || Box::new(iter_not_returning_iterator::IterNotReturningIterator)); store.register_late_pass(move || Box::new(if_then_panic::IfThenPanic)); + store.register_late_pass(|| Box::new(non_send_field_in_send_ty::NonSendFieldInSendTy)); } #[rustfmt::skip] diff --git a/clippy_lints/src/non_send_field_in_send_ty.rs b/clippy_lints/src/non_send_field_in_send_ty.rs new file mode 100644 index 00000000000..5d6501196cd --- /dev/null +++ b/clippy_lints/src/non_send_field_in_send_ty.rs @@ -0,0 +1,126 @@ +use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_note}; +use clippy_utils::ty::{implements_trait, is_copy}; +use rustc_ast::ImplPolarity; +use rustc_hir::{Item, ItemKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{self, subst::GenericArgKind, Ty}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::sym; + +declare_clippy_lint! { + /// ### What it does + /// Warns about a field in a `Send` struct that is neither `Send` nor `Copy`. + /// + /// ### Why is this bad? + /// Sending the struct to another thread and drops it there will also drop + /// the field in the new thread. This effectively changes the ownership of + /// the field type to the new thread and creates a soundness issue by + /// breaking breaks the non-`Send` invariant. + /// + /// ### Known Problems + /// Data structures that contain raw pointers may cause false positives. + /// They are sometimes safe to be sent across threads but do not implement + /// the `Send` trait. This lint has a heuristic to filter out basic cases + /// such as `Vec<*const T>`, but it's not perfect. + /// + /// ### Example + /// ```rust + /// use std::sync::Arc; + /// + /// // There is no `RC: Send` bound here + /// unsafe impl Send for ArcGuard {} + /// + /// #[derive(Debug, Clone)] + /// pub struct ArcGuard { + /// inner: T, + /// // Possibly drops `Arc` (and in turn `RC`) on a wrong thread + /// head: Arc + /// } + /// ``` + pub NON_SEND_FIELD_IN_SEND_TY, + nursery, + "a field in a `Send` struct does not implement `Send`" +} + +declare_lint_pass!(NonSendFieldInSendTy => [NON_SEND_FIELD_IN_SEND_TY]); + +impl<'tcx> LateLintPass<'tcx> for NonSendFieldInSendTy { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { + let send_trait = cx.tcx.get_diagnostic_item(sym::send_trait).unwrap(); + + // Check if we are in `Send` impl item + if_chain! { + if let ItemKind::Impl(hir_impl) = &item.kind; + if let Some(trait_ref) = &hir_impl.of_trait; + if let Some(trait_id) = trait_ref.trait_def_id(); + if send_trait == trait_id; + if let ImplPolarity::Positive = hir_impl.polarity; + if let Some(ty_trait_ref) = cx.tcx.impl_trait_ref(item.def_id); + if let self_ty = ty_trait_ref.self_ty(); + if let ty::Adt(adt_def, impl_trait_substs) = self_ty.kind(); + then { + for variant in &adt_def.variants { + for field in &variant.fields { + let field_ty = field.ty(cx.tcx, impl_trait_substs); + + // TODO: substs rebase_onto + + if raw_pointer_in_ty_def(cx, field_ty) + || implements_trait(cx, field_ty, send_trait, &[]) + || is_copy(cx, field_ty) + { + continue; + } + + if let Some(field_span) = cx.tcx.hir().span_if_local(field.did) { + if is_ty_param(field_ty) { + span_lint_and_help( + cx, + NON_SEND_FIELD_IN_SEND_TY, + field_span, + "a field in a `Send` struct does not implement `Send`", + Some(item.span), + &format!("add `{}: Send` in `Send` impl for `{}`", field_ty, self_ty), + ) + } else { + span_lint_and_note( + cx, + NON_SEND_FIELD_IN_SEND_TY, + field_span, + "a field in a `Send` struct does not implement `Send`", + Some(item.span), + &format!( + "type `{}` doesn't implement `Send` when `{}` is `Send`", + field_ty, self_ty + ), + ) + } + } + } + } + } + } + } +} + +/// Returns `true` if the type itself is a raw pointer or has a raw pointer as a +/// generic parameter, e.g., `Vec<*const u8>`. +/// Note that it does not look into enum variants or struct fields. +fn raw_pointer_in_ty_def<'tcx>(cx: &LateContext<'tcx>, target_ty: Ty<'tcx>) -> bool { + for ty_node in target_ty.walk(cx.tcx) { + if_chain! { + if let GenericArgKind::Type(inner_ty) = ty_node.unpack(); + if let ty::RawPtr(_) = inner_ty.kind(); + then { + return true; + } + } + } + + false +} + +/// Returns `true` if the type is a type parameter such as `T`. +fn is_ty_param<'tcx>(target_ty: Ty<'tcx>) -> bool { + matches!(target_ty.kind(), ty::Param(_)) +} diff --git a/tests/ui/non_send_field_in_send_ty.rs b/tests/ui/non_send_field_in_send_ty.rs new file mode 100644 index 00000000000..a0c574f8e59 --- /dev/null +++ b/tests/ui/non_send_field_in_send_ty.rs @@ -0,0 +1,94 @@ +#![warn(clippy::non_send_field_in_send_ty)] +#![feature(extern_types)] + +use std::cell::UnsafeCell; +use std::ptr::NonNull; +use std::sync::{Arc, Mutex}; + +// disrustor / RUSTSEC-2020-0150 +pub struct RingBuffer { + data: Vec>, + capacity: usize, + mask: usize, +} + +unsafe impl Send for RingBuffer {} + +// noise_search / RUSTSEC-2020-0141 +pub struct MvccRwLock { + raw: *const T, + lock: Mutex>, +} + +unsafe impl Send for MvccRwLock {} + +// async-coap / RUSTSEC-2020-0124 +pub struct ArcGuard { + inner: T, + head: Arc, +} + +unsafe impl Send for ArcGuard {} + +// rusb / RUSTSEC-2020-0098 +extern "C" { + type libusb_device_handle; +} + +pub trait UsbContext { + // some user trait that does not guarantee `Send` +} + +pub struct DeviceHandle { + context: T, + handle: NonNull, +} + +unsafe impl Send for DeviceHandle {} + +// Raw pointers are allowed +extern "C" { + type SomeFfiType; +} + +pub struct FpTest { + vec: Vec<*const SomeFfiType>, +} + +unsafe impl Send for FpTest {} + +// Check raw pointer false positive +#[allow(clippy::non_send_field_in_send_ty)] +pub struct AttrTest1(T); + +pub struct AttrTest2 { + #[allow(clippy::non_send_field_in_send_ty)] + field: T, +} + +pub enum AttrTest3 { + #[allow(clippy::non_send_field_in_send_ty)] + Enum1(T), + Enum2(T), +} + +unsafe impl Send for AttrTest1 {} +unsafe impl Send for AttrTest2 {} +unsafe impl Send for AttrTest3 {} + +pub struct MultiField { + field1: T, + field2: T, + field3: T, +} + +unsafe impl Send for MultiField {} + +pub enum MyOption { + MySome(T), + MyNone, +} + +unsafe impl Send for MyOption {} + +fn main() {}