Skip to content

Commit a9d1762

Browse files
committed
Auto merge of rust-lang#128985 - GrigorenkoPV:instantly-dangling-pointer, r=Urgau
Lint against getting pointers from immediately dropped temporaries Fixes rust-lang#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? rust-lang#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**
2 parents 3f1be1e + c69894e commit a9d1762

33 files changed

+1093
-128
lines changed

compiler/rustc_lint/messages.ftl

+6-6
Original file line numberDiff line numberDiff line change
@@ -203,14 +203,14 @@ lint_confusable_identifier_pair = found both `{$existing_sym}` and `{$sym}` as i
203203
.current_use = this identifier can be confused with `{$existing_sym}`
204204
.other_use = other identifier used here
205205
206-
lint_cstring_ptr = getting the inner pointer of a temporary `CString`
207-
.as_ptr_label = this pointer will be invalid
208-
.unwrap_label = this `CString` is deallocated at the end of the statement, bind it to a variable to extend its lifetime
209-
.note = pointers do not have a lifetime; when calling `as_ptr` the `CString` will be deallocated at the end of the statement because nothing is referencing it as far as the type system is concerned
210-
.help = for more information, see https://doc.rust-lang.org/reference/destructors.html
211-
212206
lint_custom_inner_attribute_unstable = custom inner attributes are unstable
213207
208+
lint_dangling_pointers_from_temporaries = a dangling pointer will be produced because the temporary `{$ty}` will be dropped
209+
.label_ptr = this pointer will immediately be invalid
210+
.label_temporary = this `{$ty}` is deallocated at the end of the statement, bind it to a variable to extend its lifetime
211+
.note = pointers do not have a lifetime; when calling `{$callee}` the `{$ty}` will be deallocated at the end of the statement because nothing is referencing it as far as the type system is concerned
212+
.help = for more information, see <https://doc.rust-lang.org/reference/destructors.html>
213+
214214
lint_default_hash_types = prefer `{$preferred}` over `{$used}`, it has better performance
215215
.note = a `use rustc_data_structures::fx::{$preferred}` may be necessary
216216

compiler/rustc_lint/src/dangling.rs

+223
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
use rustc_ast::visit::{visit_opt, walk_list};
2+
use rustc_hir::def_id::LocalDefId;
3+
use rustc_hir::intravisit::{FnKind, Visitor, walk_expr};
4+
use rustc_hir::{Block, Body, Expr, ExprKind, FnDecl, LangItem};
5+
use rustc_middle::ty::{Ty, TyCtxt};
6+
use rustc_session::{declare_lint, impl_lint_pass};
7+
use rustc_span::Span;
8+
use rustc_span::symbol::sym;
9+
10+
use crate::lints::DanglingPointersFromTemporaries;
11+
use crate::{LateContext, LateLintPass};
12+
13+
declare_lint! {
14+
/// The `dangling_pointers_from_temporaries` lint detects getting a pointer to data
15+
/// of a temporary that will immediately get dropped.
16+
///
17+
/// ### Example
18+
///
19+
/// ```rust
20+
/// # #![allow(unused)]
21+
/// # unsafe fn use_data(ptr: *const u8) { }
22+
/// fn gather_and_use(bytes: impl Iterator<Item = u8>) {
23+
/// let x: *const u8 = bytes.collect::<Vec<u8>>().as_ptr();
24+
/// unsafe { use_data(x) }
25+
/// }
26+
/// ```
27+
///
28+
/// {{produces}}
29+
///
30+
/// ### Explanation
31+
///
32+
/// Getting a pointer from a temporary value will not prolong its lifetime,
33+
/// which means that the value can be dropped and the allocation freed
34+
/// while the pointer still exists, making the pointer dangling.
35+
/// This is not an error (as far as the type system is concerned)
36+
/// but probably is not what the user intended either.
37+
///
38+
/// If you need stronger guarantees, consider using references instead,
39+
/// as they are statically verified by the borrow-checker to never dangle.
40+
pub DANGLING_POINTERS_FROM_TEMPORARIES,
41+
Warn,
42+
"detects getting a pointer from a temporary"
43+
}
44+
45+
/// FIXME: false negatives (i.e. the lint is not emitted when it should be)
46+
/// 1. Method calls that are not checked for:
47+
/// - [`temporary_unsafe_cell.get()`][`core::cell::UnsafeCell::get()`]
48+
/// - [`temporary_sync_unsafe_cell.get()`][`core::cell::SyncUnsafeCell::get()`]
49+
/// 2. Ways to get a temporary that are not recognized:
50+
/// - `owning_temporary.field`
51+
/// - `owning_temporary[index]`
52+
/// 3. No checks for ref-to-ptr conversions:
53+
/// - `&raw [mut] temporary`
54+
/// - `&temporary as *(const|mut) _`
55+
/// - `ptr::from_ref(&temporary)` and friends
56+
#[derive(Clone, Copy, Default)]
57+
pub(crate) struct DanglingPointers;
58+
59+
impl_lint_pass!(DanglingPointers => [DANGLING_POINTERS_FROM_TEMPORARIES]);
60+
61+
// This skips over const blocks, but they cannot use or return a dangling pointer anyways.
62+
impl<'tcx> LateLintPass<'tcx> for DanglingPointers {
63+
fn check_fn(
64+
&mut self,
65+
cx: &LateContext<'tcx>,
66+
_: FnKind<'tcx>,
67+
_: &'tcx FnDecl<'tcx>,
68+
body: &'tcx Body<'tcx>,
69+
_: Span,
70+
_: LocalDefId,
71+
) {
72+
DanglingPointerSearcher { cx, inside_call_args: false }.visit_body(body)
73+
}
74+
}
75+
76+
/// This produces a dangling pointer:
77+
/// ```ignore (example)
78+
/// let ptr = CString::new("hello").unwrap().as_ptr();
79+
/// foo(ptr)
80+
/// ```
81+
///
82+
/// But this does not:
83+
/// ```ignore (example)
84+
/// foo(CString::new("hello").unwrap().as_ptr())
85+
/// ```
86+
///
87+
/// But this does:
88+
/// ```ignore (example)
89+
/// foo({ let ptr = CString::new("hello").unwrap().as_ptr(); ptr })
90+
/// ```
91+
///
92+
/// So we have to keep track of when we are inside of a function/method call argument.
93+
struct DanglingPointerSearcher<'lcx, 'tcx> {
94+
cx: &'lcx LateContext<'tcx>,
95+
/// Keeps track of whether we are inside of function/method call arguments,
96+
/// where this lint should not be emitted.
97+
///
98+
/// See [the main doc][`Self`] for examples.
99+
inside_call_args: bool,
100+
}
101+
102+
impl Visitor<'_> for DanglingPointerSearcher<'_, '_> {
103+
fn visit_expr(&mut self, expr: &Expr<'_>) -> Self::Result {
104+
if !self.inside_call_args {
105+
lint_expr(self.cx, expr)
106+
}
107+
match expr.kind {
108+
ExprKind::Call(lhs, args) | ExprKind::MethodCall(_, lhs, args, _) => {
109+
self.visit_expr(lhs);
110+
self.with_inside_call_args(true, |this| walk_list!(this, visit_expr, args))
111+
}
112+
ExprKind::Block(&Block { stmts, expr, .. }, _) => {
113+
self.with_inside_call_args(false, |this| walk_list!(this, visit_stmt, stmts));
114+
visit_opt!(self, visit_expr, expr)
115+
}
116+
_ => walk_expr(self, expr),
117+
}
118+
}
119+
}
120+
121+
impl DanglingPointerSearcher<'_, '_> {
122+
fn with_inside_call_args<R>(
123+
&mut self,
124+
inside_call_args: bool,
125+
callback: impl FnOnce(&mut Self) -> R,
126+
) -> R {
127+
let old = core::mem::replace(&mut self.inside_call_args, inside_call_args);
128+
let result = callback(self);
129+
self.inside_call_args = old;
130+
result
131+
}
132+
}
133+
134+
fn lint_expr(cx: &LateContext<'_>, expr: &Expr<'_>) {
135+
if let ExprKind::MethodCall(method, receiver, _args, _span) = expr.kind
136+
&& matches!(method.ident.name, sym::as_ptr | sym::as_mut_ptr)
137+
&& is_temporary_rvalue(receiver)
138+
&& let ty = cx.typeck_results().expr_ty(receiver)
139+
&& is_interesting(cx.tcx, ty)
140+
{
141+
// FIXME: use `emit_node_lint` when `#[primary_span]` is added.
142+
cx.tcx.emit_node_span_lint(
143+
DANGLING_POINTERS_FROM_TEMPORARIES,
144+
expr.hir_id,
145+
method.ident.span,
146+
DanglingPointersFromTemporaries {
147+
callee: method.ident.name,
148+
ty,
149+
ptr_span: method.ident.span,
150+
temporary_span: receiver.span,
151+
},
152+
)
153+
}
154+
}
155+
156+
fn is_temporary_rvalue(expr: &Expr<'_>) -> bool {
157+
match expr.kind {
158+
// Const is not temporary.
159+
ExprKind::ConstBlock(..) | ExprKind::Repeat(..) | ExprKind::Lit(..) => false,
160+
161+
// This is literally lvalue.
162+
ExprKind::Path(..) => false,
163+
164+
// Calls return rvalues.
165+
ExprKind::Call(..) | ExprKind::MethodCall(..) | ExprKind::Binary(..) => true,
166+
167+
// Inner blocks are rvalues.
168+
ExprKind::If(..) | ExprKind::Loop(..) | ExprKind::Match(..) | ExprKind::Block(..) => true,
169+
170+
// FIXME: these should probably recurse and typecheck along the way.
171+
// Some false negatives are possible for now.
172+
ExprKind::Index(..) | ExprKind::Field(..) | ExprKind::Unary(..) => false,
173+
174+
ExprKind::Struct(..) => true,
175+
176+
// FIXME: this has false negatives, but I do not want to deal with 'static/const promotion just yet.
177+
ExprKind::Array(..) => false,
178+
179+
// These typecheck to `!`
180+
ExprKind::Break(..) | ExprKind::Continue(..) | ExprKind::Ret(..) | ExprKind::Become(..) => {
181+
false
182+
}
183+
184+
// These typecheck to `()`
185+
ExprKind::Assign(..) | ExprKind::AssignOp(..) | ExprKind::Yield(..) => false,
186+
187+
// Compiler-magic macros
188+
ExprKind::AddrOf(..) | ExprKind::OffsetOf(..) | ExprKind::InlineAsm(..) => false,
189+
190+
// We are not interested in these
191+
ExprKind::Cast(..)
192+
| ExprKind::Closure(..)
193+
| ExprKind::Tup(..)
194+
| ExprKind::DropTemps(..)
195+
| ExprKind::Let(..) => false,
196+
197+
// Not applicable
198+
ExprKind::Type(..) | ExprKind::Err(..) => false,
199+
}
200+
}
201+
202+
// Array, Vec, String, CString, MaybeUninit, Cell, Box<[_]>, Box<str>, Box<CStr>,
203+
// or any of the above in arbitrary many nested Box'es.
204+
fn is_interesting(tcx: TyCtxt<'_>, ty: Ty<'_>) -> bool {
205+
if ty.is_array() {
206+
true
207+
} else if let Some(inner) = ty.boxed_ty() {
208+
inner.is_slice()
209+
|| inner.is_str()
210+
|| inner.ty_adt_def().is_some_and(|def| tcx.is_lang_item(def.did(), LangItem::CStr))
211+
|| is_interesting(tcx, inner)
212+
} else if let Some(def) = ty.ty_adt_def() {
213+
for lang_item in [LangItem::String, LangItem::MaybeUninit] {
214+
if tcx.is_lang_item(def.did(), lang_item) {
215+
return true;
216+
}
217+
}
218+
tcx.get_diagnostic_name(def.did())
219+
.is_some_and(|name| matches!(name, sym::cstring_type | sym::Vec | sym::Cell))
220+
} else {
221+
false
222+
}
223+
}

compiler/rustc_lint/src/lib.rs

+4-3
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ mod async_closures;
4040
mod async_fn_in_trait;
4141
pub mod builtin;
4242
mod context;
43+
mod dangling;
4344
mod deref_into_dyn_supertrait;
4445
mod drop_forget_useless;
4546
mod early;
@@ -59,7 +60,6 @@ mod levels;
5960
mod lints;
6061
mod macro_expr_fragment_specifier_2024_migration;
6162
mod map_unit_fn;
62-
mod methods;
6363
mod multiple_supertrait_upcastable;
6464
mod non_ascii_idents;
6565
mod non_fmt_panic;
@@ -85,6 +85,7 @@ mod unused;
8585
use async_closures::AsyncClosureUsage;
8686
use async_fn_in_trait::AsyncFnInTrait;
8787
use builtin::*;
88+
use dangling::*;
8889
use deref_into_dyn_supertrait::*;
8990
use drop_forget_useless::*;
9091
use enum_intrinsics_non_enums::EnumIntrinsicsNonEnums;
@@ -97,7 +98,6 @@ use invalid_from_utf8::*;
9798
use let_underscore::*;
9899
use macro_expr_fragment_specifier_2024_migration::*;
99100
use map_unit_fn::*;
100-
use methods::*;
101101
use multiple_supertrait_upcastable::*;
102102
use non_ascii_idents::*;
103103
use non_fmt_panic::NonPanicFmt;
@@ -225,7 +225,7 @@ late_lint_methods!(
225225
UngatedAsyncFnTrackCaller: UngatedAsyncFnTrackCaller,
226226
ShadowedIntoIter: ShadowedIntoIter,
227227
DropTraitConstraints: DropTraitConstraints,
228-
TemporaryCStringAsPtr: TemporaryCStringAsPtr,
228+
DanglingPointers: DanglingPointers,
229229
NonPanicFmt: NonPanicFmt,
230230
NoopMethodCall: NoopMethodCall,
231231
EnumIntrinsicsNonEnums: EnumIntrinsicsNonEnums,
@@ -350,6 +350,7 @@ fn register_builtins(store: &mut LintStore) {
350350
store.register_renamed("non_fmt_panic", "non_fmt_panics");
351351
store.register_renamed("unused_tuple_struct_fields", "dead_code");
352352
store.register_renamed("static_mut_ref", "static_mut_refs");
353+
store.register_renamed("temporary_cstring_as_ptr", "dangling_pointers_from_temporaries");
353354

354355
// These were moved to tool lints, but rustc still sees them when compiling normally, before
355356
// tool lints are registered, so `check_tool_name_for_backwards_compat` doesn't work. Use

compiler/rustc_lint/src/lints.rs

+10-7
Original file line numberDiff line numberDiff line change
@@ -1137,16 +1137,19 @@ pub(crate) struct IgnoredUnlessCrateSpecified<'a> {
11371137
pub name: Symbol,
11381138
}
11391139

1140-
// methods.rs
1140+
// dangling.rs
11411141
#[derive(LintDiagnostic)]
1142-
#[diag(lint_cstring_ptr)]
1142+
#[diag(lint_dangling_pointers_from_temporaries)]
11431143
#[note]
11441144
#[help]
1145-
pub(crate) struct CStringPtr {
1146-
#[label(lint_as_ptr_label)]
1147-
pub as_ptr: Span,
1148-
#[label(lint_unwrap_label)]
1149-
pub unwrap: Span,
1145+
// FIXME: put #[primary_span] on `ptr_span` once it does not cause conflicts
1146+
pub(crate) struct DanglingPointersFromTemporaries<'tcx> {
1147+
pub callee: Symbol,
1148+
pub ty: Ty<'tcx>,
1149+
#[label(lint_label_ptr)]
1150+
pub ptr_span: Span,
1151+
#[label(lint_label_temporary)]
1152+
pub temporary_span: Span,
11501153
}
11511154

11521155
// multiple_supertrait_upcastable.rs

compiler/rustc_lint/src/methods.rs

-69
This file was deleted.

0 commit comments

Comments
 (0)