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

Use attributes for dangling_pointers_from_temporaries lint #132732

Merged
merged 6 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
5 changes: 5 additions & 0 deletions compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -875,6 +875,11 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
lang, Normal, template!(NameValueStr: "name"), DuplicatesOk, EncodeCrossCrate::No, lang_items,
"lang items are subject to change",
),
rustc_attr!(
rustc_as_ptr, Normal, template!(Word), ErrorFollowing,
EncodeCrossCrate::Yes,
"#[rustc_as_ptr] is used to mark functions returning pointers to their inner allocations."
),
rustc_attr!(
rustc_pass_by_value, Normal, template!(Word), ErrorFollowing,
EncodeCrossCrate::Yes,
Expand Down
27 changes: 13 additions & 14 deletions compiler/rustc_lint/src/dangling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,10 @@ declare_lint! {
}

/// 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:
/// 1. Ways to get a temporary that are not recognized:
/// - `owning_temporary.field`
/// - `owning_temporary[index]`
/// 3. No checks for ref-to-ptr conversions:
/// 2. No checks for ref-to-ptr conversions:
/// - `&raw [mut] temporary`
/// - `&temporary as *(const|mut) _`
/// - `ptr::from_ref(&temporary)` and friends
Expand Down Expand Up @@ -133,10 +130,11 @@ impl DanglingPointerSearcher<'_, '_> {

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)
&& let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
&& cx.tcx.has_attr(fn_id, sym::rustc_as_ptr)
&& is_temporary_rvalue(receiver)
&& let ty = cx.typeck_results().expr_ty(receiver)
&& is_interesting(cx.tcx, ty)
&& owns_allocation(cx.tcx, ty)
{
// FIXME: use `emit_node_lint` when `#[primary_span]` is added.
cx.tcx.emit_node_span_lint(
Expand Down Expand Up @@ -199,24 +197,25 @@ fn is_temporary_rvalue(expr: &Expr<'_>) -> bool {
}
}

// 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 {
// Array, Vec, String, CString, MaybeUninit, Cell, Box<[_]>, Box<str>, Box<CStr>, UnsafeCell,
// SyncUnsafeCell, or any of the above in arbitrary many nested Box'es.
fn owns_allocation(tcx: TyCtxt<'_>, ty: Ty<'_>) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

With the addition of the attribute we should able to simplify this owns_allocation method to a simple check of the type and it's generic arguments.

Something like this would be a good starting point (the check should probably be further refined):

    match ty.kind() {
        ty::Array(_, _) => true,
        ty::Adt(_def, args) => args.iter().filter_map(|g| g.as_type()).all(|ty| !ty.is_any_ptr()),
        _ => false,
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a Box<Box<&[T]>> would pass the check that you suggest, even though it should not

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we still need to take the derefs into account.

Probably something more like this:

    let adjs = cx.typeck_results().expr_adjustments(receiver);
    let origin_ty = cx.typeck_results().expr_ty(receiver);

    adjs.iter()
        .filter_map(|adj| match adj.kind {
            Adjust::Deref(_) => Some(adj.target),
            Adjust::NeverToAny
            | Adjust::Borrow(_)
            | Adjust::Pointer(_)
            | Adjust::ReborrowPin(_) => None,
        })
        .all(|ty| !ty.is_any_ptr())
        && match origin_ty.kind() {
            ty::Adt(_def, args) => {
                args.iter().filter_map(|g| g.as_type()).all(|ty| !ty.is_any_ptr())
            }
            _ => !origin_ty.is_any_ptr(),
        }

but, let's discussed that on the follow-up issue/pr.

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)
|| owns_allocation(tcx, inner)
} else if let Some(def) = ty.ty_adt_def() {
for lang_item in [LangItem::String, LangItem::MaybeUninit] {
for lang_item in [LangItem::String, LangItem::MaybeUninit, LangItem::UnsafeCell] {
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))
tcx.get_diagnostic_name(def.did()).is_some_and(|name| {
matches!(name, sym::cstring_type | sym::Vec | sym::Cell | sym::SyncUnsafeCell)
})
} else {
false
}
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_passes/src/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
self.check_rustc_std_internal_symbol(attr, span, target)
}
[sym::naked, ..] => self.check_naked(hir_id, attr, span, target, attrs),
[sym::rustc_as_ptr, ..] => {
self.check_applied_to_fn_or_method(hir_id, attr, span, target)
}
[sym::rustc_never_returns_null_ptr, ..] => {
self.check_applied_to_fn_or_method(hir_id, attr, span, target)
}
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ symbols! {
SubdiagMessage,
Subdiagnostic,
Sync,
SyncUnsafeCell,
T,
Target,
ToOwned,
Expand Down Expand Up @@ -410,7 +411,6 @@ symbols! {
arm,
arm_target_feature,
array,
as_mut_ptr,
as_ptr,
as_ref,
as_str,
Expand Down Expand Up @@ -1652,6 +1652,7 @@ symbols! {
rustc_allow_const_fn_unstable,
rustc_allow_incoherent_impl,
rustc_allowed_through_unstable_modules,
rustc_as_ptr,
rustc_attrs,
rustc_autodiff,
rustc_box,
Expand Down
2 changes: 2 additions & 0 deletions library/alloc/src/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1499,6 +1499,7 @@ impl<T: ?Sized, A: Allocator> Box<T, A> {
/// [`as_ptr`]: Self::as_ptr
#[unstable(feature = "box_as_ptr", issue = "129090")]
#[rustc_never_returns_null_ptr]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[inline]
pub fn as_mut_ptr(b: &mut Self) -> *mut T {
// This is a primitive deref, not going through `DerefMut`, and therefore not materializing
Expand Down Expand Up @@ -1547,6 +1548,7 @@ impl<T: ?Sized, A: Allocator> Box<T, A> {
/// [`as_ptr`]: Self::as_ptr
#[unstable(feature = "box_as_ptr", issue = "129090")]
#[rustc_never_returns_null_ptr]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[inline]
pub fn as_ptr(b: &Self) -> *const T {
// This is a primitive deref, not going through `DerefMut`, and therefore not materializing
Expand Down
2 changes: 2 additions & 0 deletions library/alloc/src/vec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1662,6 +1662,7 @@ impl<T, A: Allocator> Vec<T, A> {
#[stable(feature = "vec_as_ptr", since = "1.37.0")]
#[rustc_const_unstable(feature = "const_vec_string_slice", issue = "129041")]
#[rustc_never_returns_null_ptr]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[inline]
pub const fn as_ptr(&self) -> *const T {
// We shadow the slice method of the same name to avoid going through
Expand Down Expand Up @@ -1724,6 +1725,7 @@ impl<T, A: Allocator> Vec<T, A> {
#[stable(feature = "vec_as_ptr", since = "1.37.0")]
#[rustc_const_unstable(feature = "const_vec_string_slice", issue = "129041")]
#[rustc_never_returns_null_ptr]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[inline]
pub const fn as_mut_ptr(&mut self) -> *mut T {
// We shadow the slice method of the same name to avoid going through
Expand Down
5 changes: 5 additions & 0 deletions library/core/src/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,7 @@ impl<T: ?Sized> Cell<T> {
#[inline]
#[stable(feature = "cell_as_ptr", since = "1.12.0")]
#[rustc_const_stable(feature = "const_cell_as_ptr", since = "1.32.0")]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[rustc_never_returns_null_ptr]
pub const fn as_ptr(&self) -> *mut T {
self.value.get()
Expand Down Expand Up @@ -1149,6 +1150,7 @@ impl<T: ?Sized> RefCell<T> {
/// ```
#[inline]
#[stable(feature = "cell_as_ptr", since = "1.12.0")]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[rustc_never_returns_null_ptr]
pub fn as_ptr(&self) -> *mut T {
self.value.get()
Expand Down Expand Up @@ -2157,6 +2159,7 @@ impl<T: ?Sized> UnsafeCell<T> {
#[inline(always)]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_stable(feature = "const_unsafecell_get", since = "1.32.0")]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
Urgau marked this conversation as resolved.
Show resolved Hide resolved
#[rustc_never_returns_null_ptr]
pub const fn get(&self) -> *mut T {
// We can just cast the pointer from `UnsafeCell<T>` to `T` because of
Expand Down Expand Up @@ -2270,6 +2273,7 @@ impl<T: DispatchFromDyn<U>, U> DispatchFromDyn<UnsafeCell<U>> for UnsafeCell<T>
/// See [`UnsafeCell`] for details.
#[unstable(feature = "sync_unsafe_cell", issue = "95439")]
#[repr(transparent)]
#[rustc_diagnostic_item = "SyncUnsafeCell"]
#[rustc_pub_transparent]
pub struct SyncUnsafeCell<T: ?Sized> {
value: UnsafeCell<T>,
Expand Down Expand Up @@ -2303,6 +2307,7 @@ impl<T: ?Sized> SyncUnsafeCell<T> {
/// when casting to `&mut T`, and ensure that there are no mutations
/// or mutable aliases going on when casting to `&T`
#[inline]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[rustc_never_returns_null_ptr]
pub const fn get(&self) -> *mut T {
self.value.get()
Expand Down
1 change: 1 addition & 0 deletions library/core/src/ffi/c_str.rs
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,7 @@ impl CStr {
#[must_use]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_stable(feature = "const_str_as_ptr", since = "1.32.0")]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[rustc_never_returns_null_ptr]
pub const fn as_ptr(&self) -> *const c_char {
self.inner.as_ptr()
Expand Down
2 changes: 2 additions & 0 deletions library/core/src/mem/maybe_uninit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@ impl<T> MaybeUninit<T> {
/// until they are, it is advisable to avoid them.)
#[stable(feature = "maybe_uninit", since = "1.36.0")]
#[rustc_const_stable(feature = "const_maybe_uninit_as_ptr", since = "1.59.0")]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[inline(always)]
pub const fn as_ptr(&self) -> *const T {
// `MaybeUninit` and `ManuallyDrop` are both `repr(transparent)` so we can cast the pointer.
Expand Down Expand Up @@ -570,6 +571,7 @@ impl<T> MaybeUninit<T> {
/// until they are, it is advisable to avoid them.)
#[stable(feature = "maybe_uninit", since = "1.36.0")]
#[rustc_const_stable(feature = "const_maybe_uninit_as_mut_ptr", since = "1.83.0")]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[inline(always)]
pub const fn as_mut_ptr(&mut self) -> *mut T {
// `MaybeUninit` and `ManuallyDrop` are both `repr(transparent)` so we can cast the pointer.
Expand Down
2 changes: 2 additions & 0 deletions library/core/src/slice/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,7 @@ impl<T> [T] {
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_stable(feature = "const_slice_as_ptr", since = "1.32.0")]
#[rustc_never_returns_null_ptr]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[inline(always)]
#[must_use]
pub const fn as_ptr(&self) -> *const T {
Expand Down Expand Up @@ -766,6 +767,7 @@ impl<T> [T] {
#[rustc_const_stable(feature = "const_ptr_offset", since = "1.61.0")]
#[rustc_allow_const_fn_unstable(const_mut_refs)]
#[rustc_never_returns_null_ptr]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[inline(always)]
#[must_use]
pub const fn as_mut_ptr(&mut self) -> *mut T {
Expand Down
2 changes: 2 additions & 0 deletions library/core/src/str/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,7 @@ impl str {
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_stable(feature = "rustc_str_as_ptr", since = "1.32.0")]
#[rustc_never_returns_null_ptr]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[must_use]
#[inline(always)]
pub const fn as_ptr(&self) -> *const u8 {
Expand All @@ -388,6 +389,7 @@ impl str {
#[stable(feature = "str_as_mut_ptr", since = "1.36.0")]
#[rustc_const_stable(feature = "const_str_as_mut", since = "1.83.0")]
#[rustc_never_returns_null_ptr]
#[cfg_attr(not(bootstrap), rustc_as_ptr)]
#[must_use]
#[inline(always)]
pub const fn as_mut_ptr(&mut self) -> *mut u8 {
Expand Down
7 changes: 6 additions & 1 deletion tests/ui/lint/dangling-pointers-from-temporaries/types.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#![deny(dangling_pointers_from_temporaries)]
#![feature(sync_unsafe_cell)]

use std::cell::Cell;
use std::cell::{Cell, SyncUnsafeCell, UnsafeCell};
use std::ffi::{CStr, CString};
use std::mem::MaybeUninit;

Expand Down Expand Up @@ -47,6 +48,10 @@ fn main() {
//~^ ERROR a dangling pointer will be produced because the temporary `MaybeUninit<u8>` will be dropped
declval::<Vec<AsPtrFake>>().as_ptr();
//~^ ERROR a dangling pointer will be produced because the temporary `Vec<AsPtrFake>` will be dropped
declval::<UnsafeCell<u8>>().get();
//~^ ERROR a dangling pointer will be produced because the temporary `UnsafeCell<u8>` will be dropped
declval::<SyncUnsafeCell<u8>>().get();
//~^ ERROR a dangling pointer will be produced because the temporary `SyncUnsafeCell<u8>` will be dropped
declval::<Box<AsPtrFake>>().as_ptr();
declval::<AsPtrFake>().as_ptr();
}
Loading
Loading