diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f15b16b5c96..0309bb74be81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5620,6 +5620,7 @@ Released 2018-09-13 [`as_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_conversions [`as_pointer_underscore`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_pointer_underscore [`as_ptr_cast_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_ptr_cast_mut +[`as_ptr_in_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_ptr_in_map [`as_underscore`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_underscore [`assertions_on_constants`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants [`assertions_on_result_states`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_result_states diff --git a/clippy_lints/src/as_ptr_in_map.rs b/clippy_lints/src/as_ptr_in_map.rs new file mode 100644 index 000000000000..aed49d49c748 --- /dev/null +++ b/clippy_lints/src/as_ptr_in_map.rs @@ -0,0 +1,176 @@ +use std::ops::ControlFlow; + +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::ty::is_copy; +use clippy_utils::visitors::for_each_expr; +use rustc_hir as hir; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::declare_lint_pass; +use rustc_span::{Ident, Symbol}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for closure creating raw pointer from owned type and returning raw pointer + /// + /// ### Why is this bad? + /// It might create a dangling pointer becaused owned type in a closure are dropped after the call + /// + /// ### Example + /// ```no_run + /// let v_opt = Some(vec![1]); + /// let _v_ptr = v_opt.map(|v| v.as_ptr()); + /// ``` + /// Use instead: + /// ```no_run + /// let v_opt = Some(vec![1]); + /// let _v_ptr = v_opt.as_ref().map(|v| v.as_ptr()); + /// ``` + #[clippy::version = "1.89.0"] + pub AS_PTR_IN_MAP, + nursery, + "check raw pointer inside closure from owned type" +} +declare_lint_pass!(AsPtrInMap => [AS_PTR_IN_MAP]); + +impl<'tcx> LateLintPass<'tcx> for AsPtrInMap { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) { + if let hir::ExprKind::Closure(hir::Closure { body, .. }) = expr.kind { + let typeck = cx.typeck_results(); + let closure_output = if let ty::Closure(_, closure_subs) = typeck.expr_ty(expr).kind() { + let closure = closure_subs.as_closure(); + let closure_sig = cx + .tcx + .signature_unclosure(closure.sig(), hir::Safety::Safe) + .skip_binder(); + + closure_sig.output() + } else { + return; + }; + + // check if the return type of the closure contains a raw pointer + if ty_contains_raw_ptr(closure_output) { + let closure_body = cx.tcx.hir_body(*body); + + let mut idents_needs_drop: Vec<_> = closure_body + .params + .iter() + .flat_map(|param| get_ident_bindings(*param.pat)) + .filter(|(_, hir_id)| { + let ty = typeck.node_type(*hir_id); + !is_copy(cx, ty) && ty.needs_drop(cx.tcx, cx.typing_env()) + }) + .map(|(i, _)| (i, false)) + .collect(); + + // get usage of call that create raw pointer inside the closure + let _ = for_each_expr(cx, expr, |e| { + if let Some(receiver) = is_creating_raw_ptr(*e) { + for (ident, is_creating_raw_ptr) in &mut idents_needs_drop { + if ident_eq(*ident, receiver) { + *is_creating_raw_ptr = true; + } + } + } + if false { + return ControlFlow::Break(()); + } + ControlFlow::Continue(()) + }); + + let indents_make_raw_pointer = idents_needs_drop + .iter() + .filter_map(|(ident, is_creating_raw_ptr)| is_creating_raw_ptr.then_some(ident)) + .collect::>(); + + if !indents_make_raw_pointer.is_empty() { + span_lint_and_then( + cx, + AS_PTR_IN_MAP, + expr.span, + "this closure might return a dangling pointer", + |diag| { + for ident in indents_make_raw_pointer { + diag.span_note( + ident.span, + "this bindings is used to create a raw pointer that might be dangling", + ); + } + }, + ); + } + } + } + } +} + +fn ident_eq(name: Ident, path: hir::Expr<'_>) -> bool { + match path.kind { + hir::ExprKind::Path(hir::QPath::Resolved(None, path)) => { + path.segments.len() == 1 && path.segments[0].ident == name + }, + hir::ExprKind::AddrOf(_, _, expr) => ident_eq(name, *expr), + _ => false, + } +} + +fn ty_contains_raw_ptr(ty: ty::Ty<'_>) -> bool { + match ty.kind() { + #[allow(clippy::match_same_arms)] + ty::Adt(_, _) => false, // TODO: might contain raw pointer + ty::Array(ty, _) | ty::Slice(ty) => ty_contains_raw_ptr(*ty), + ty::RawPtr(_, _) => true, + ty::Tuple(ty_list) => ty_list.iter().any(|ty| ty_contains_raw_ptr(ty)), + _ => false, + } +} + +fn get_ident_bindings(pat: hir::Pat<'_>) -> Vec<(Ident, hir::HirId)> { + match pat.kind { + hir::PatKind::Binding(hir::BindingMode::NONE | hir::BindingMode::MUT, hir_id, ident, _) => { + vec![(ident, hir_id)] + }, + hir::PatKind::Struct(_, pat_fields, _) => pat_fields + .iter() + .flat_map(|pat_field| get_ident_bindings(*pat_field.pat)) + .collect(), + hir::PatKind::TupleStruct(_, pats, _) => pats.iter().flat_map(|pat| get_ident_bindings(*pat)).collect(), + hir::PatKind::Tuple(pats, _) => pats.iter().flat_map(|pat| get_ident_bindings(*pat)).collect(), + hir::PatKind::Box(pat) => get_ident_bindings(*pat), + hir::PatKind::Slice(pats1, pat_opt, pats2) => pats1 + .iter() + .flat_map(|pat| get_ident_bindings(*pat)) + .chain(pat_opt.iter().flat_map(|pat| get_ident_bindings(**pat))) + .chain(pats2.iter().flat_map(|pat| get_ident_bindings(*pat))) + .collect(), + _ => vec![], + } + // }); +} + +fn is_creating_raw_ptr(expr: hir::Expr<'_>) -> Option> { + match expr.kind { + hir::ExprKind::MethodCall(method, receiver, [], _) => { + if is_dangerous_ptr(method.ident.name) { + return Some(*receiver); + } + }, + hir::ExprKind::Call(function, [arg]) => { + let hir::ExprKind::Path(hir::QPath::TypeRelative(_, path_segment)) = function.kind else { + return None; + }; + + if is_dangerous_ptr(path_segment.ident.name) { + return Some(*arg); + } + }, + _ => (), + } + + None +} + +fn is_dangerous_ptr(s: Symbol) -> bool { + matches!(s.as_str(), "as_ptr" | "as_non_null" | "as_mut_ptr") +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index aac30d29a908..ad66ba2d02a9 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -9,6 +9,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::arbitrary_source_item_ordering::ARBITRARY_SOURCE_ITEM_ORDERING_INFO, crate::arc_with_non_send_sync::ARC_WITH_NON_SEND_SYNC_INFO, crate::as_conversions::AS_CONVERSIONS_INFO, + crate::as_ptr_in_map::AS_PTR_IN_MAP_INFO, crate::asm_syntax::INLINE_ASM_X86_ATT_SYNTAX_INFO, crate::asm_syntax::INLINE_ASM_X86_INTEL_SYNTAX_INFO, crate::assertions_on_constants::ASSERTIONS_ON_CONSTANTS_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 96a6dee58852..a8a731650f9a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -76,6 +76,7 @@ mod approx_const; mod arbitrary_source_item_ordering; mod arc_with_non_send_sync; mod as_conversions; +mod as_ptr_in_map; mod asm_syntax; mod assertions_on_constants; mod assertions_on_result_states; @@ -830,5 +831,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(cloned_ref_to_slice_refs::ClonedRefToSliceRefs::new(conf))); store.register_late_pass(|_| Box::new(infallible_try_from::InfallibleTryFrom)); store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny)); + store.register_late_pass(|_| Box::new(as_ptr_in_map::AsPtrInMap)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/as_ptr_in_map.rs b/tests/ui/as_ptr_in_map.rs new file mode 100644 index 000000000000..a6a5ce903450 --- /dev/null +++ b/tests/ui/as_ptr_in_map.rs @@ -0,0 +1,36 @@ +#![warn(clippy::as_ptr_in_map)] + +fn main() { + let v1_res = Ok(vec![1]); + let _v1_ptr: Result<_, ()> = v1_res.map(|v1| v1.as_ptr()); + //~^ as_ptr_in_map + + let v2_opt = Some((vec![2], 2)); + let _v2_ptr = v2_opt.map(|(v2, _x)| { + v2.as_ptr(); + 2 + }); + // this is fine + + let v3_opt = Some((vec![3], 3)); + let _v3_ptr = v3_opt.map(|(v3, x)| { + //~^ as_ptr_in_map + let _a = x + 2; + let p = v3.as_ptr(); + let _b = 6; + p + }); + + let v4_res = Ok(vec![4]); + let _v4_ptr: Result<_, &()> = v4_res.as_ref().map(|v4| v4.as_ptr()); + // this is fine + + let v5_opt = Some(vec![5]); + let _v5_ptr = v5_opt.map(|v5| std::vec::Vec::as_ptr(&v5)); + //~^ as_ptr_in_map + + let v6_res = Ok(vec![6]); + let v6_2_ptr = [6]; + let _v6_ptr: Result<_, ()> = v6_res.map(|_v6| v6_2_ptr.as_ptr()); + // this is fine +} diff --git a/tests/ui/as_ptr_in_map.stderr b/tests/ui/as_ptr_in_map.stderr new file mode 100644 index 000000000000..3c3bc84b91b1 --- /dev/null +++ b/tests/ui/as_ptr_in_map.stderr @@ -0,0 +1,47 @@ +error: this closure might return a dangling pointer + --> tests/ui/as_ptr_in_map.rs:5:45 + | +LL | let _v1_ptr: Result<_, ()> = v1_res.map(|v1| v1.as_ptr()); + | ^^^^^^^^^^^^^^^^ + | +note: this bindings is used to create a raw pointer that might be dangling + --> tests/ui/as_ptr_in_map.rs:5:46 + | +LL | let _v1_ptr: Result<_, ()> = v1_res.map(|v1| v1.as_ptr()); + | ^^ + = note: `-D clippy::as-ptr-in-map` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::as_ptr_in_map)]` + +error: this closure might return a dangling pointer + --> tests/ui/as_ptr_in_map.rs:16:30 + | +LL | let _v3_ptr = v3_opt.map(|(v3, x)| { + | ______________________________^ +LL | | +LL | | let _a = x + 2; +LL | | let p = v3.as_ptr(); +LL | | let _b = 6; +LL | | p +LL | | }); + | |_____^ + | +note: this bindings is used to create a raw pointer that might be dangling + --> tests/ui/as_ptr_in_map.rs:16:32 + | +LL | let _v3_ptr = v3_opt.map(|(v3, x)| { + | ^^ + +error: this closure might return a dangling pointer + --> tests/ui/as_ptr_in_map.rs:29:30 + | +LL | let _v5_ptr = v5_opt.map(|v5| std::vec::Vec::as_ptr(&v5)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: this bindings is used to create a raw pointer that might be dangling + --> tests/ui/as_ptr_in_map.rs:29:31 + | +LL | let _v5_ptr = v5_opt.map(|v5| std::vec::Vec::as_ptr(&v5)); + | ^^ + +error: aborting due to 3 previous errors +