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.

Sign up for GitHub

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

Add invalid null pointer usage lint. #6192

Closed
wants to merge 8 commits into from
Closed

Add invalid null pointer usage lint. #6192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
446acc7
Add invalid null pointer usage lint.
boxdot Oct 18, 2020
8dcc446
Check also for std::ptr::{null, null_mut}.
boxdot Nov 20, 2020
b0844cf
Rename lint to invalid_null_ptr_usage.
boxdot Nov 20, 2020
d76f8e5
Add test for ptr::null_mut.
boxdot Nov 20, 2020
8ef5a6d
Address review comments: remove dot (also in CMP_NULL).
boxdot Nov 20, 2020
e15d0ce
Address review comment: extend lint on std::slice::from_raw_parts_mut
boxdot Nov 20, 2020
79ed1c7
Add remaining functions from std::ptr.
boxdot Nov 23, 2020
4c56ef0
Make clippy happy.
boxdot Nov 23, 2020
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1872,6 +1872,7 @@ Released 2018-09-13
[`into_iter_on_array`]: https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_array
[`into_iter_on_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_ref
[`invalid_atomic_ordering`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_atomic_ordering
[`invalid_null_ptr_usage`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_null_ptr_usage
[`invalid_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_ref
[`invalid_regex`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_regex
[`invalid_upcast_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_upcast_comparisons
Expand Down
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -797,6 +797,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&pattern_type_mismatch::PATTERN_TYPE_MISMATCH,
&precedence::PRECEDENCE,
&ptr::CMP_NULL,
&ptr::INVALID_NULL_PTR_USAGE,
&ptr::MUT_FROM_REF,
&ptr::PTR_ARG,
&ptr_eq::PTR_EQ,
Expand Down Expand Up @@ -1522,6 +1523,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&partialeq_ne_impl::PARTIALEQ_NE_IMPL),
LintId::of(&precedence::PRECEDENCE),
LintId::of(&ptr::CMP_NULL),
LintId::of(&ptr::INVALID_NULL_PTR_USAGE),
LintId::of(&ptr::MUT_FROM_REF),
LintId::of(&ptr::PTR_ARG),
LintId::of(&ptr_eq::PTR_EQ),
Expand Down Expand Up @@ -1845,6 +1847,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&mut_key::MUTABLE_KEY_TYPE),
LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS),
LintId::of(&option_env_unwrap::OPTION_ENV_UNWRAP),
LintId::of(&ptr::INVALID_NULL_PTR_USAGE),
LintId::of(&ptr::MUT_FROM_REF),
LintId::of(&ranges::REVERSED_EMPTY_RANGES),
LintId::of(&regex::INVALID_REGEX),
Expand Down
79 changes: 74 additions & 5 deletions clippy_lints/src/ptr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

use crate::utils::ptr::get_spans;
use crate::utils::{
is_allowed, is_type_diagnostic_item, match_qpath, match_type, paths, snippet_opt, span_lint, span_lint_and_sugg,
span_lint_and_then, walk_ptrs_hir_ty,
is_allowed, is_type_diagnostic_item, match_function_call, match_qpath, match_type, paths, snippet_opt, span_lint,
span_lint_and_sugg, span_lint_and_then, walk_ptrs_hir_ty,
};
use if_chain::if_chain;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -94,7 +94,7 @@ declare_clippy_lint! {
/// ```
pub CMP_NULL,
style,
"comparing a pointer to a null pointer, suggesting to use `.is_null()` instead."
"comparing a pointer to a null pointer, suggesting to use `.is_null()` instead"
}

declare_clippy_lint! {
Expand All @@ -119,7 +119,28 @@ declare_clippy_lint! {
"fns that create mutable refs from immutable ref args"
}

declare_lint_pass!(Ptr => [PTR_ARG, CMP_NULL, MUT_FROM_REF]);
declare_clippy_lint! {
/// **What it does:** This lint checks for invalid usages of `ptr::null`.
///
/// **Why is this bad?** This causes undefined behavior.
///
/// **Known problems:** None.
///
/// **Example:**
/// ```ignore
/// // Bad. Undefined behavior
/// unsafe { std::slice::from_raw_parts(ptr::null(), 0); }
/// ```
///
/// // Good
/// unsafe { std::slice::from_raw_parts(NonNull::dangling().as_ptr(), 0); }
/// ```
pub INVALID_NULL_PTR_USAGE,
correctness,
"invalid usage of a null pointer, suggesting `NonNull::dangling()` instead"
}

declare_lint_pass!(Ptr => [PTR_ARG, CMP_NULL, MUT_FROM_REF, INVALID_NULL_PTR_USAGE]);

impl<'tcx> LateLintPass<'tcx> for Ptr {
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
Expand Down Expand Up @@ -161,10 +182,55 @@ impl<'tcx> LateLintPass<'tcx> for Ptr {
"comparing with null is better expressed by the `.is_null()` method",
);
}
} else {
check_invalid_ptr_usage(cx, expr);
}
}
}

fn check_invalid_ptr_usage<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<()> {
// fn_name, arg_idx
const INVALID_NULL_PTR_USAGE_TABLE: [(&[&str], usize); 20] = [
(&paths::SLICE_FROM_RAW_PARTS, 0),
(&paths::SLICE_FROM_RAW_PARTS_MUT, 0),
(&paths::PTR_COPY, 0),
(&paths::PTR_COPY, 1),
(&paths::PTR_COPY_NONOVERLAPPING, 0),
(&paths::PTR_COPY_NONOVERLAPPING, 1),
(&paths::PTR_READ, 0),
(&paths::PTR_READ_UNALIGNED, 0),
(&paths::PTR_READ_VOLATILE, 0),
(&paths::PTR_REPLACE, 0),
(&paths::PTR_SLICE_FROM_RAW_PARTS, 0),
(&paths::PTR_SLICE_FROM_RAW_PARTS_MUT, 0),
(&paths::PTR_SWAP, 0),
(&paths::PTR_SWAP, 1),
(&paths::PTR_SWAP_NONOVERLAPPING, 0),
(&paths::PTR_SWAP_NONOVERLAPPING, 1),
(&paths::PTR_WRITE, 0),
(&paths::PTR_WRITE_UNALIGNED, 0),
(&paths::PTR_WRITE_VOLATILE, 0),
(&paths::PTR_WRITE_BYTES, 0),
];

let arg = INVALID_NULL_PTR_USAGE_TABLE.iter().find_map(|(fn_name, arg_idx)| {
let args = match_function_call(cx, expr, fn_name)?;
args.get(*arg_idx).filter(|arg| is_null_path(arg))
})?;

span_lint_and_sugg(
cx,
INVALID_NULL_PTR_USAGE,
arg.span,
"pointer must be non-null",
"change this to",
"core::ptr::NonNull::dangling().as_ptr()".to_string(),
Applicability::MachineApplicable,
);

Some(())
}

#[allow(clippy::too_many_lines)]
fn check_fn(cx: &LateContext<'_>, decl: &FnDecl<'_>, fn_id: HirId, opt_body_id: Option<BodyId>) {
let fn_def_id = cx.tcx.hir().local_def_id(fn_id);
Expand Down Expand Up @@ -321,7 +387,10 @@ fn is_null_path(expr: &Expr<'_>) -> bool {
if let ExprKind::Call(ref pathexp, ref args) = expr.kind {
if args.is_empty() {
if let ExprKind::Path(ref path) = pathexp.kind {
return match_qpath(path, &paths::PTR_NULL) || match_qpath(path, &paths::PTR_NULL_MUT);
return match_qpath(path, &paths::PTR_NULL)
|| match_qpath(path, &paths::PTR_NULL_MUT)
|| match_qpath(path, &paths::STD_PTR_NULL)
|| match_qpath(path, &paths::STD_PTR_NULL_MUT);
}
}
}
Expand Down
17 changes: 17 additions & 0 deletions clippy_lints/src/utils/paths.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,23 @@ pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"];
pub const POLL: [&str; 4] = ["core", "task", "poll", "Poll"];
pub const POLL_PENDING: [&str; 5] = ["core", "task", "poll", "Poll", "Pending"];
pub const POLL_READY: [&str; 5] = ["core", "task", "poll", "Poll", "Ready"];
pub const PTR_COPY: [&str; 3] = ["core", "ptr", "copy"];
pub const PTR_COPY_NONOVERLAPPING: [&str; 3] = ["core", "ptr", "copy_nonoverlapping"];
pub const PTR_EQ: [&str; 3] = ["core", "ptr", "eq"];
pub const PTR_NULL: [&str; 3] = ["core", "ptr", "null"];
pub const PTR_NULL_MUT: [&str; 3] = ["core", "ptr", "null_mut"];
pub const PTR_READ: [&str; 3] = ["core", "ptr", "read"];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like this is a perfect lint for #5393. Should we try to add diagnostics to the functions instead of hardcoding them here? I would need some hints how to do this though. :)

Copy link
Member

@ebroto ebroto Nov 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think you are right. I'm not familiar with the process of adding diagnostic items, but looking at this commit it seems that just adding the attribute should be enough?

This should be done in the rust-lang/rust repo, and we can sync that change afterwards and finish the lint here. As a heads-up we may try to pin to a nightly soon in this repo, so the aforementioned sync process could change. I apologize in advance if that creates any problem, we still have to discover what the new way of working with the pinned nightly will look like :)

EDIT: GitHub inlined this comment in the review, but not the whole conversation. Take a look at the additional comment there. TL;DR this is not needed for merging.

Copy link
Member

@ebroto ebroto Nov 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

FYI, there are other lints being currently implemented that will add paths (e.g. #6394)

Even though the right thing to do would be probably to turn those into diagnostic items, I will not make a blocker out of this, so it's fine if you leave the paths here. I understand it would make merging this way longer, and it's not a problem introduced in your PR.

Ideally, you can address #5393 after this one if you want :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right about first merging this and then adding diagnostic items to the compiler. I got stuck in the latter step.

pub const PTR_READ_UNALIGNED: [&str; 3] = ["core", "ptr", "read_unaligned"];
pub const PTR_READ_VOLATILE: [&str; 3] = ["core", "ptr", "read_volatile"];
pub const PTR_REPLACE: [&str; 3] = ["core", "ptr", "replace"];
pub const PTR_SLICE_FROM_RAW_PARTS: [&str; 3] = ["core", "ptr", "slice_from_raw_parts"];
pub const PTR_SLICE_FROM_RAW_PARTS_MUT: [&str; 3] = ["core", "ptr", "slice_from_raw_parts_mut"];
pub const PTR_SWAP: [&str; 3] = ["core", "ptr", "swap"];
pub const PTR_SWAP_NONOVERLAPPING: [&str; 3] = ["core", "ptr", "swap_nonoverlapping"];
pub const PTR_WRITE: [&str; 3] = ["core", "ptr", "write"];
pub const PTR_WRITE_BYTES: [&str; 3] = ["core", "intrinsics", "write_bytes"];
pub const PTR_WRITE_UNALIGNED: [&str; 3] = ["core", "ptr", "write_unaligned"];
pub const PTR_WRITE_VOLATILE: [&str; 3] = ["core", "ptr", "write_volatile"];
pub const PUSH_STR: [&str; 4] = ["alloc", "string", "String", "push_str"];
pub const RANGE_ARGUMENT_TRAIT: [&str; 3] = ["core", "ops", "RangeBounds"];
pub const RC: [&str; 3] = ["alloc", "rc", "Rc"];
Expand All @@ -116,6 +130,8 @@ pub const RWLOCK_READ_GUARD: [&str; 4] = ["std", "sync", "rwlock", "RwLockReadGu
pub const RWLOCK_WRITE_GUARD: [&str; 4] = ["std", "sync", "rwlock", "RwLockWriteGuard"];
pub const SERDE_DESERIALIZE: [&str; 3] = ["serde", "de", "Deserialize"];
pub const SERDE_DE_VISITOR: [&str; 3] = ["serde", "de", "Visitor"];
pub const SLICE_FROM_RAW_PARTS: [&str; 4] = ["core", "slice", "raw", "from_raw_parts"];
pub const SLICE_FROM_RAW_PARTS_MUT: [&str; 4] = ["core", "slice", "raw", "from_raw_parts_mut"];
pub const SLICE_INTO_VEC: [&str; 4] = ["alloc", "slice", "<impl [T]>", "into_vec"];
pub const SLICE_ITER: [&str; 4] = ["core", "slice", "iter", "Iter"];
pub const STDERR: [&str; 4] = ["std", "io", "stdio", "stderr"];
Expand All @@ -124,6 +140,7 @@ pub const STD_CONVERT_IDENTITY: [&str; 3] = ["std", "convert", "identity"];
pub const STD_FS_CREATE_DIR: [&str; 3] = ["std", "fs", "create_dir"];
pub const STD_MEM_TRANSMUTE: [&str; 3] = ["std", "mem", "transmute"];
pub const STD_PTR_NULL: [&str; 3] = ["std", "ptr", "null"];
pub const STD_PTR_NULL_MUT: [&str; 3] = ["std", "ptr", "null_mut"];
pub const STRING: [&str; 3] = ["alloc", "string", "String"];
pub const STRING_AS_MUT_STR: [&str; 4] = ["alloc", "string", "String", "as_mut_str"];
pub const STRING_AS_STR: [&str; 4] = ["alloc", "string", "String", "as_str"];
Expand Down
49 changes: 49 additions & 0 deletions tests/ui/invalid_null_ptr_usage.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// run-rustfix

fn main() {
unsafe {
let _slice: &[usize] = std::slice::from_raw_parts(core::ptr::NonNull::dangling().as_ptr(), 0);
let _slice: &[usize] = std::slice::from_raw_parts(core::ptr::NonNull::dangling().as_ptr(), 0);

let _slice: &[usize] = std::slice::from_raw_parts_mut(core::ptr::NonNull::dangling().as_ptr(), 0);

std::ptr::copy::<usize>(std::ptr::null(), std::ptr::NonNull::dangling().as_ptr(), 0);
std::ptr::copy::<usize>(std::ptr::NonNull::dangling().as_ptr(), std::ptr::null_mut(), 0);

std::ptr::copy_nonoverlapping::<usize>(std::ptr::null(), std::ptr::NonNull::dangling().as_ptr(), 0);
std::ptr::copy_nonoverlapping::<usize>(std::ptr::NonNull::dangling().as_ptr(), std::ptr::null_mut(), 0);
Comment on lines +10 to +14
Copy link
Member

Choose a reason for hiding this comment

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

These cases do not seem to be detected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in #7023


struct A; // zero sized struct
assert_eq!(std::mem::size_of::<A>(), 0);

let _a: A = std::ptr::read(core::ptr::NonNull::dangling().as_ptr());
let _a: A = std::ptr::read(core::ptr::NonNull::dangling().as_ptr());

let _a: A = std::ptr::read_unaligned(core::ptr::NonNull::dangling().as_ptr());
let _a: A = std::ptr::read_unaligned(core::ptr::NonNull::dangling().as_ptr());

let _a: A = std::ptr::read_volatile(core::ptr::NonNull::dangling().as_ptr());
let _a: A = std::ptr::read_volatile(core::ptr::NonNull::dangling().as_ptr());

let _a: A = std::ptr::replace(core::ptr::NonNull::dangling().as_ptr(), A);

let _slice: *const [usize] = std::ptr::slice_from_raw_parts(core::ptr::NonNull::dangling().as_ptr(), 0);
let _slice: *const [usize] = std::ptr::slice_from_raw_parts(core::ptr::NonNull::dangling().as_ptr(), 0);

let _slice: *const [usize] = std::ptr::slice_from_raw_parts_mut(core::ptr::NonNull::dangling().as_ptr(), 0);

std::ptr::swap::<A>(core::ptr::NonNull::dangling().as_ptr(), &mut A);
std::ptr::swap::<A>(&mut A, core::ptr::NonNull::dangling().as_ptr());

std::ptr::swap_nonoverlapping::<A>(core::ptr::NonNull::dangling().as_ptr(), &mut A, 0);
std::ptr::swap_nonoverlapping::<A>(&mut A, core::ptr::NonNull::dangling().as_ptr(), 0);

std::ptr::write(core::ptr::NonNull::dangling().as_ptr(), A);

std::ptr::write_unaligned(core::ptr::NonNull::dangling().as_ptr(), A);

std::ptr::write_volatile(core::ptr::NonNull::dangling().as_ptr(), A);

std::ptr::write_bytes::<usize>(core::ptr::NonNull::dangling().as_ptr(), 42, 0);
}
}
49 changes: 49 additions & 0 deletions tests/ui/invalid_null_ptr_usage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// run-rustfix

fn main() {
unsafe {
let _slice: &[usize] = std::slice::from_raw_parts(std::ptr::null(), 0);
let _slice: &[usize] = std::slice::from_raw_parts(std::ptr::null_mut(), 0);

let _slice: &[usize] = std::slice::from_raw_parts_mut(std::ptr::null_mut(), 0);

std::ptr::copy::<usize>(std::ptr::null(), std::ptr::NonNull::dangling().as_ptr(), 0);
std::ptr::copy::<usize>(std::ptr::NonNull::dangling().as_ptr(), std::ptr::null_mut(), 0);

std::ptr::copy_nonoverlapping::<usize>(std::ptr::null(), std::ptr::NonNull::dangling().as_ptr(), 0);
std::ptr::copy_nonoverlapping::<usize>(std::ptr::NonNull::dangling().as_ptr(), std::ptr::null_mut(), 0);

struct A; // zero sized struct
assert_eq!(std::mem::size_of::<A>(), 0);

let _a: A = std::ptr::read(std::ptr::null());
let _a: A = std::ptr::read(std::ptr::null_mut());

let _a: A = std::ptr::read_unaligned(std::ptr::null());
let _a: A = std::ptr::read_unaligned(std::ptr::null_mut());

let _a: A = std::ptr::read_volatile(std::ptr::null());
let _a: A = std::ptr::read_volatile(std::ptr::null_mut());

let _a: A = std::ptr::replace(std::ptr::null_mut(), A);

let _slice: *const [usize] = std::ptr::slice_from_raw_parts(std::ptr::null(), 0);
let _slice: *const [usize] = std::ptr::slice_from_raw_parts(std::ptr::null_mut(), 0);

let _slice: *const [usize] = std::ptr::slice_from_raw_parts_mut(std::ptr::null_mut(), 0);

std::ptr::swap::<A>(std::ptr::null_mut(), &mut A);
std::ptr::swap::<A>(&mut A, std::ptr::null_mut());

std::ptr::swap_nonoverlapping::<A>(std::ptr::null_mut(), &mut A, 0);
std::ptr::swap_nonoverlapping::<A>(&mut A, std::ptr::null_mut(), 0);

std::ptr::write(std::ptr::null_mut(), A);

std::ptr::write_unaligned(std::ptr::null_mut(), A);

std::ptr::write_volatile(std::ptr::null_mut(), A);

std::ptr::write_bytes::<usize>(std::ptr::null_mut(), 42, 0);
}
}
Loading