-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of #128985 - GrigorenkoPV:instantly-dangling-pointer, r=Urgau
Lint against getting pointers from immediately dropped temporaries Fixes #123613 ## Changes: 1. New lint: `dangling_pointers_from_temporaries`. Is a generalization of `temporary_cstring_as_ptr` for more types and more ways to get a temporary. 2. `temporary_cstring_as_ptr` is removed and marked as renamed to `dangling_pointers_from_temporaries`. 3. `clippy::temporary_cstring_as_ptr` is marked as renamed to `dangling_pointers_from_temporaries`. 4. Fixed a false positive[^fp] for when the pointer is not actually dangling because of lifetime extension for function/method call arguments. 5. `core::cell::Cell` is now `rustc_diagnostic_item = "Cell"` ## Questions: - [ ] Instead of manually checking for a list of known methods and diagnostic items, maybe add some sort of annotation to those methods in library and check for the presence of that annotation? #128985 (comment) ## Known limitations: ### False negatives[^fn]: See the comments in `compiler/rustc_lint/src/dangling.rs` 1. Method calls that are not checked for: - `temporary_unsafe_cell.get()` - `temporary_sync_unsafe_cell.get()` 2. Ways to get a temporary that are not recognized: - `owning_temporary.field` - `owning_temporary[index]` 3. No checks for ref-to-ptr conversions: - `&raw [mut] temporary` - `&temporary as *(const|mut) _` - `ptr::from_ref(&temporary)` and friends [^fn]: lint **should** be emitted, but **is not** [^fp]: lint **should not** be emitted, but **is**
- Loading branch information
Showing
33 changed files
with
1,093 additions
and
128 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,223 @@ | ||
| use rustc_ast::visit::{visit_opt, walk_list}; | ||
| use rustc_hir::def_id::LocalDefId; | ||
| use rustc_hir::intravisit::{FnKind, Visitor, walk_expr}; | ||
| use rustc_hir::{Block, Body, Expr, ExprKind, FnDecl, LangItem}; | ||
| use rustc_middle::ty::{Ty, TyCtxt}; | ||
| use rustc_session::{declare_lint, impl_lint_pass}; | ||
| use rustc_span::Span; | ||
| use rustc_span::symbol::sym; | ||
|
|
||
| use crate::lints::DanglingPointersFromTemporaries; | ||
| use crate::{LateContext, LateLintPass}; | ||
|
|
||
| declare_lint! { | ||
| /// The `dangling_pointers_from_temporaries` lint detects getting a pointer to data | ||
| /// of a temporary that will immediately get dropped. | ||
| /// | ||
| /// ### Example | ||
| /// | ||
| /// ```rust | ||
| /// # #![allow(unused)] | ||
| /// # unsafe fn use_data(ptr: *const u8) { } | ||
| /// fn gather_and_use(bytes: impl Iterator<Item = u8>) { | ||
| /// let x: *const u8 = bytes.collect::<Vec<u8>>().as_ptr(); | ||
| /// unsafe { use_data(x) } | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// {{produces}} | ||
| /// | ||
| /// ### Explanation | ||
| /// | ||
| /// Getting a pointer from a temporary value will not prolong its lifetime, | ||
| /// which means that the value can be dropped and the allocation freed | ||
| /// while the pointer still exists, making the pointer dangling. | ||
| /// This is not an error (as far as the type system is concerned) | ||
| /// but probably is not what the user intended either. | ||
| /// | ||
| /// If you need stronger guarantees, consider using references instead, | ||
| /// as they are statically verified by the borrow-checker to never dangle. | ||
| pub DANGLING_POINTERS_FROM_TEMPORARIES, | ||
| Warn, | ||
| "detects getting a pointer from a temporary" | ||
| } | ||
|
|
||
| /// FIXME: false negatives (i.e. the lint is not emitted when it should be) | ||
| /// 1. Method calls that are not checked for: | ||
| /// - [`temporary_unsafe_cell.get()`][`core::cell::UnsafeCell::get()`] | ||
| /// - [`temporary_sync_unsafe_cell.get()`][`core::cell::SyncUnsafeCell::get()`] | ||
| /// 2. Ways to get a temporary that are not recognized: | ||
| /// - `owning_temporary.field` | ||
| /// - `owning_temporary[index]` | ||
| /// 3. No checks for ref-to-ptr conversions: | ||
| /// - `&raw [mut] temporary` | ||
| /// - `&temporary as *(const|mut) _` | ||
| /// - `ptr::from_ref(&temporary)` and friends | ||
| #[derive(Clone, Copy, Default)] | ||
| pub(crate) struct DanglingPointers; | ||
|
|
||
| impl_lint_pass!(DanglingPointers => [DANGLING_POINTERS_FROM_TEMPORARIES]); | ||
|
|
||
| // This skips over const blocks, but they cannot use or return a dangling pointer anyways. | ||
| impl<'tcx> LateLintPass<'tcx> for DanglingPointers { | ||
| fn check_fn( | ||
| &mut self, | ||
| cx: &LateContext<'tcx>, | ||
| _: FnKind<'tcx>, | ||
| _: &'tcx FnDecl<'tcx>, | ||
| body: &'tcx Body<'tcx>, | ||
| _: Span, | ||
| _: LocalDefId, | ||
| ) { | ||
| DanglingPointerSearcher { cx, inside_call_args: false }.visit_body(body) | ||
| } | ||
| } | ||
|
|
||
| /// This produces a dangling pointer: | ||
| /// ```ignore (example) | ||
| /// let ptr = CString::new("hello").unwrap().as_ptr(); | ||
| /// foo(ptr) | ||
| /// ``` | ||
| /// | ||
| /// But this does not: | ||
| /// ```ignore (example) | ||
| /// foo(CString::new("hello").unwrap().as_ptr()) | ||
| /// ``` | ||
| /// | ||
| /// But this does: | ||
| /// ```ignore (example) | ||
| /// foo({ let ptr = CString::new("hello").unwrap().as_ptr(); ptr }) | ||
| /// ``` | ||
| /// | ||
| /// So we have to keep track of when we are inside of a function/method call argument. | ||
| struct DanglingPointerSearcher<'lcx, 'tcx> { | ||
| cx: &'lcx LateContext<'tcx>, | ||
| /// Keeps track of whether we are inside of function/method call arguments, | ||
| /// where this lint should not be emitted. | ||
| /// | ||
| /// See [the main doc][`Self`] for examples. | ||
| inside_call_args: bool, | ||
| } | ||
|
|
||
| impl Visitor<'_> for DanglingPointerSearcher<'_, '_> { | ||
| fn visit_expr(&mut self, expr: &Expr<'_>) -> Self::Result { | ||
| if !self.inside_call_args { | ||
| lint_expr(self.cx, expr) | ||
| } | ||
| match expr.kind { | ||
| ExprKind::Call(lhs, args) | ExprKind::MethodCall(_, lhs, args, _) => { | ||
| self.visit_expr(lhs); | ||
| self.with_inside_call_args(true, |this| walk_list!(this, visit_expr, args)) | ||
| } | ||
| ExprKind::Block(&Block { stmts, expr, .. }, _) => { | ||
| self.with_inside_call_args(false, |this| walk_list!(this, visit_stmt, stmts)); | ||
| visit_opt!(self, visit_expr, expr) | ||
| } | ||
| _ => walk_expr(self, expr), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl DanglingPointerSearcher<'_, '_> { | ||
| fn with_inside_call_args<R>( | ||
| &mut self, | ||
| inside_call_args: bool, | ||
| callback: impl FnOnce(&mut Self) -> R, | ||
| ) -> R { | ||
| let old = core::mem::replace(&mut self.inside_call_args, inside_call_args); | ||
| let result = callback(self); | ||
| self.inside_call_args = old; | ||
| result | ||
| } | ||
| } | ||
|
|
||
| fn lint_expr(cx: &LateContext<'_>, expr: &Expr<'_>) { | ||
| if let ExprKind::MethodCall(method, receiver, _args, _span) = expr.kind | ||
| && matches!(method.ident.name, sym::as_ptr | sym::as_mut_ptr) | ||
| && is_temporary_rvalue(receiver) | ||
| && let ty = cx.typeck_results().expr_ty(receiver) | ||
| && is_interesting(cx.tcx, ty) | ||
| { | ||
| // FIXME: use `emit_node_lint` when `#[primary_span]` is added. | ||
| cx.tcx.emit_node_span_lint( | ||
| DANGLING_POINTERS_FROM_TEMPORARIES, | ||
| expr.hir_id, | ||
| method.ident.span, | ||
| DanglingPointersFromTemporaries { | ||
| callee: method.ident.name, | ||
| ty, | ||
| ptr_span: method.ident.span, | ||
| temporary_span: receiver.span, | ||
| }, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| fn is_temporary_rvalue(expr: &Expr<'_>) -> bool { | ||
| match expr.kind { | ||
| // Const is not temporary. | ||
| ExprKind::ConstBlock(..) | ExprKind::Repeat(..) | ExprKind::Lit(..) => false, | ||
|
|
||
| // This is literally lvalue. | ||
| ExprKind::Path(..) => false, | ||
|
|
||
| // Calls return rvalues. | ||
| ExprKind::Call(..) | ExprKind::MethodCall(..) | ExprKind::Binary(..) => true, | ||
|
|
||
| // Inner blocks are rvalues. | ||
| ExprKind::If(..) | ExprKind::Loop(..) | ExprKind::Match(..) | ExprKind::Block(..) => true, | ||
|
|
||
| // FIXME: these should probably recurse and typecheck along the way. | ||
| // Some false negatives are possible for now. | ||
| ExprKind::Index(..) | ExprKind::Field(..) | ExprKind::Unary(..) => false, | ||
|
|
||
| ExprKind::Struct(..) => true, | ||
|
|
||
| // FIXME: this has false negatives, but I do not want to deal with 'static/const promotion just yet. | ||
| ExprKind::Array(..) => false, | ||
|
|
||
| // These typecheck to `!` | ||
| ExprKind::Break(..) | ExprKind::Continue(..) | ExprKind::Ret(..) | ExprKind::Become(..) => { | ||
| false | ||
| } | ||
|
|
||
| // These typecheck to `()` | ||
| ExprKind::Assign(..) | ExprKind::AssignOp(..) | ExprKind::Yield(..) => false, | ||
|
|
||
| // Compiler-magic macros | ||
| ExprKind::AddrOf(..) | ExprKind::OffsetOf(..) | ExprKind::InlineAsm(..) => false, | ||
|
|
||
| // We are not interested in these | ||
| ExprKind::Cast(..) | ||
| | ExprKind::Closure(..) | ||
| | ExprKind::Tup(..) | ||
| | ExprKind::DropTemps(..) | ||
| | ExprKind::Let(..) => false, | ||
|
|
||
| // Not applicable | ||
| ExprKind::Type(..) | ExprKind::Err(..) => false, | ||
| } | ||
| } | ||
|
|
||
| // Array, Vec, String, CString, MaybeUninit, Cell, Box<[_]>, Box<str>, Box<CStr>, | ||
| // or any of the above in arbitrary many nested Box'es. | ||
| fn is_interesting(tcx: TyCtxt<'_>, ty: Ty<'_>) -> bool { | ||
| if ty.is_array() { | ||
| true | ||
| } else if let Some(inner) = ty.boxed_ty() { | ||
| inner.is_slice() | ||
| || inner.is_str() | ||
| || inner.ty_adt_def().is_some_and(|def| tcx.is_lang_item(def.did(), LangItem::CStr)) | ||
| || is_interesting(tcx, inner) | ||
| } else if let Some(def) = ty.ty_adt_def() { | ||
| for lang_item in [LangItem::String, LangItem::MaybeUninit] { | ||
| if tcx.is_lang_item(def.did(), lang_item) { | ||
| return true; | ||
| } | ||
| } | ||
| tcx.get_diagnostic_name(def.did()) | ||
| .is_some_and(|name| matches!(name, sym::cstring_type | sym::Vec | sym::Cell)) | ||
| } else { | ||
| false | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.