Skip to content

Update from_raw_with_void_ptr to support types other than Box #9700

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

Merged
merged 1 commit into from
Oct 25, 2022
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
48 changes: 34 additions & 14 deletions clippy_lints/src/from_raw_with_void_ptr.rs
Original file line number Diff line number Diff line change
@@ -1,25 +1,22 @@
use clippy_utils::diagnostics::span_lint_and_help;
use clippy_utils::path_def_id;
use clippy_utils::ty::is_c_void;
use clippy_utils::{match_def_path, path_def_id, paths};
use rustc_hir::def_id::DefId;
use rustc_hir::{Expr, ExprKind, QPath};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::RawPtr;
use rustc_middle::ty::TypeAndMut;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::sym;

declare_clippy_lint! {
/// ### What it does
/// Checks if we're passing a `c_void` raw pointer to `Box::from_raw(_)`
/// Checks if we're passing a `c_void` raw pointer to `{Box,Rc,Arc,Weak}::from_raw(_)`
///
/// ### Why is this bad?
/// However, it is easy to run into the pitfall of calling from_raw with the c_void pointer.
/// Note that the definition of, say, Box::from_raw is:
///
/// `pub unsafe fn from_raw(raw: *mut T) -> Box<T>`
///
/// meaning that if you pass a *mut c_void you will get a Box<c_void>.
/// Per the safety requirements in the documentation, for this to be safe,
/// c_void would need to have the same memory layout as the original type, which is often not the case.
/// When dealing with `c_void` raw pointers in FFI, it is easy to run into the pitfall of calling `from_raw` with the `c_void` pointer.
/// The type signature of `Box::from_raw` is `fn from_raw(raw: *mut T) -> Box<T>`, so if you pass a `*mut c_void` you will get a `Box<c_void>` (and similarly for `Rc`, `Arc` and `Weak`).
/// For this to be safe, `c_void` would need to have the same memory layout as the original type, which is often not the case.
///
/// ### Example
/// ```rust
Expand All @@ -37,7 +34,7 @@ declare_clippy_lint! {
#[clippy::version = "1.66.0"]
pub FROM_RAW_WITH_VOID_PTR,
suspicious,
"creating a `Box` from a raw void pointer"
"creating a `Box` from a void raw pointer"
}
declare_lint_pass!(FromRawWithVoidPtr => [FROM_RAW_WITH_VOID_PTR]);

Expand All @@ -46,12 +43,35 @@ impl LateLintPass<'_> for FromRawWithVoidPtr {
if let ExprKind::Call(box_from_raw, [arg]) = expr.kind
&& let ExprKind::Path(QPath::TypeRelative(ty, seg)) = box_from_raw.kind
&& seg.ident.name == sym!(from_raw)
// FIXME: This lint is also applicable to other types, like `Rc`, `Arc` and `Weak`.
&& path_def_id(cx, ty).map_or(false, |id| Some(id) == cx.tcx.lang_items().owned_box())
&& let Some(type_str) = path_def_id(cx, ty).and_then(|id| def_id_matches_type(cx, id))
&& let arg_kind = cx.typeck_results().expr_ty(arg).kind()
&& let RawPtr(TypeAndMut { ty, .. }) = arg_kind
&& is_c_void(cx, *ty) {
span_lint_and_help(cx, FROM_RAW_WITH_VOID_PTR, expr.span, "creating a `Box` from a raw void pointer", Some(arg.span), "cast this to a pointer of the actual type");
let msg = format!("creating a `{type_str}` from a void raw pointer");
span_lint_and_help(cx, FROM_RAW_WITH_VOID_PTR, expr.span, &msg, Some(arg.span), "cast this to a pointer of the appropriate type");
}
}
}

/// Checks whether a `DefId` matches `Box`, `Rc`, `Arc`, or one of the `Weak` types.
/// Returns a static string slice with the name of the type, if one was found.
fn def_id_matches_type(cx: &LateContext<'_>, def_id: DefId) -> Option<&'static str> {
// Box
if Some(def_id) == cx.tcx.lang_items().owned_box() {
return Some("Box");
}

if let Some(symbol) = cx.tcx.get_diagnostic_name(def_id) {
if symbol == sym::Arc {
return Some("Arc");
} else if symbol == sym::Rc {
return Some("Rc");
}
}

if match_def_path(cx, def_id, &paths::WEAK_RC) || match_def_path(cx, def_id, &paths::WEAK_ARC) {
Some("Weak")
} else {
None
}
}
18 changes: 18 additions & 0 deletions tests/ui/from_raw_with_void_ptr.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#![warn(clippy::from_raw_with_void_ptr)]

use std::ffi::c_void;
use std::rc::Rc;
use std::sync::Arc;

fn main() {
// must lint
Expand All @@ -13,4 +15,20 @@ fn main() {
// shouldn't be linted
let should_not_lint_ptr = Box::into_raw(Box::new(12u8)) as *mut u8;
let _ = unsafe { Box::from_raw(should_not_lint_ptr as *mut u8) };

// must lint
let ptr = Rc::into_raw(Rc::new(42usize)) as *mut c_void;
let _ = unsafe { Rc::from_raw(ptr) };

// must lint
let ptr = Arc::into_raw(Arc::new(42usize)) as *mut c_void;
let _ = unsafe { Arc::from_raw(ptr) };

// must lint
let ptr = std::rc::Weak::into_raw(Rc::downgrade(&Rc::new(42usize))) as *mut c_void;
let _ = unsafe { std::rc::Weak::from_raw(ptr) };

// must lint
let ptr = std::sync::Weak::into_raw(Arc::downgrade(&Arc::new(42usize))) as *mut c_void;
let _ = unsafe { std::sync::Weak::from_raw(ptr) };
}
58 changes: 53 additions & 5 deletions tests/ui/from_raw_with_void_ptr.stderr
Original file line number Diff line number Diff line change
@@ -1,15 +1,63 @@
error: creating a `Box` from a raw void pointer
--> $DIR/from_raw_with_void_ptr.rs:8:22
error: creating a `Box` from a void raw pointer
--> $DIR/from_raw_with_void_ptr.rs:10:22
|
LL | let _ = unsafe { Box::from_raw(ptr) };
| ^^^^^^^^^^^^^^^^^^
|
help: cast this to a pointer of the actual type
--> $DIR/from_raw_with_void_ptr.rs:8:36
help: cast this to a pointer of the appropriate type
--> $DIR/from_raw_with_void_ptr.rs:10:36
|
LL | let _ = unsafe { Box::from_raw(ptr) };
| ^^^
= note: `-D clippy::from-raw-with-void-ptr` implied by `-D warnings`

error: aborting due to previous error
error: creating a `Rc` from a void raw pointer
--> $DIR/from_raw_with_void_ptr.rs:21:22
|
LL | let _ = unsafe { Rc::from_raw(ptr) };
| ^^^^^^^^^^^^^^^^^
|
help: cast this to a pointer of the appropriate type
--> $DIR/from_raw_with_void_ptr.rs:21:35
|
LL | let _ = unsafe { Rc::from_raw(ptr) };
| ^^^

error: creating a `Arc` from a void raw pointer
--> $DIR/from_raw_with_void_ptr.rs:25:22
|
LL | let _ = unsafe { Arc::from_raw(ptr) };
| ^^^^^^^^^^^^^^^^^^
|
help: cast this to a pointer of the appropriate type
--> $DIR/from_raw_with_void_ptr.rs:25:36
|
LL | let _ = unsafe { Arc::from_raw(ptr) };
| ^^^

error: creating a `Weak` from a void raw pointer
--> $DIR/from_raw_with_void_ptr.rs:29:22
|
LL | let _ = unsafe { std::rc::Weak::from_raw(ptr) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: cast this to a pointer of the appropriate type
--> $DIR/from_raw_with_void_ptr.rs:29:46
|
LL | let _ = unsafe { std::rc::Weak::from_raw(ptr) };
| ^^^

error: creating a `Weak` from a void raw pointer
--> $DIR/from_raw_with_void_ptr.rs:33:22
|
LL | let _ = unsafe { std::sync::Weak::from_raw(ptr) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: cast this to a pointer of the appropriate type
--> $DIR/from_raw_with_void_ptr.rs:33:48
|
LL | let _ = unsafe { std::sync::Weak::from_raw(ptr) };
| ^^^

error: aborting due to 5 previous errors