diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fedd3749487..ae316a90b05a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ All notable changes to this project will be documented in this file. ## 0.0.76 — 2016-06-10 * Rustup to *rustc 1.11.0-nightly (7d2f75a95 2016-06-09)* * `cargo clippy` now automatically defines the `clippy` feature +* New lint: [`not_unsafe_ptr_arg_deref`] ## 0.0.75 — 2016-06-08 * Rustup to *rustc 1.11.0-nightly (763f9234b 2016-06-06)* @@ -220,6 +221,7 @@ All notable changes to this project will be documented in this file. [`non_ascii_literal`]: https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal [`nonminimal_bool`]: https://github.com/Manishearth/rust-clippy/wiki#nonminimal_bool [`nonsensical_open_options`]: https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options +[`not_unsafe_ptr_arg_deref`]: https://github.com/Manishearth/rust-clippy/wiki#not_unsafe_ptr_arg_deref [`ok_expect`]: https://github.com/Manishearth/rust-clippy/wiki#ok_expect [`option_map_unwrap_or`]: https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or [`option_map_unwrap_or_else`]: https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or_else diff --git a/README.md b/README.md index 2ae53db2d050..3159a3e8979c 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Table of contents: ## Lints -There are 157 lints included in this crate: +There are 158 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -115,6 +115,7 @@ name [non_ascii_literal](https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal) | allow | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead [nonminimal_bool](https://github.com/Manishearth/rust-clippy/wiki#nonminimal_bool) | allow | checks for boolean expressions that can be written more concisely [nonsensical_open_options](https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options) | warn | nonsensical combination of options for opening a file +[not_unsafe_ptr_arg_deref](https://github.com/Manishearth/rust-clippy/wiki#not_unsafe_ptr_arg_deref) | warn | public functions dereferencing raw pointer arguments but not marked `unsafe` [ok_expect](https://github.com/Manishearth/rust-clippy/wiki#ok_expect) | warn | using `ok().expect()`, which gives worse error messages than calling `expect` directly on the Result [option_map_unwrap_or](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or) | warn | using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)` [option_map_unwrap_or_else](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or_else) | warn | using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `map_or_else(g, f)` diff --git a/clippy_lints/src/functions.rs b/clippy_lints/src/functions.rs index 3d3423a47435..0dec4e94c0b8 100644 --- a/clippy_lints/src/functions.rs +++ b/clippy_lints/src/functions.rs @@ -1,9 +1,11 @@ -use rustc::lint::*; -use rustc::hir; use rustc::hir::intravisit; +use rustc::hir; +use rustc::ty; +use rustc::lint::*; +use std::collections::HashSet; use syntax::ast; use syntax::codemap::Span; -use utils::span_lint; +use utils::{span_lint, type_is_unsafe_function}; /// **What it does:** Check for functions with too many parameters. /// @@ -15,7 +17,7 @@ use utils::span_lint; /// /// **Example:** /// -/// ``` +/// ```rust /// fn foo(x: u32, y: u32, name: &str, c: Color, w: f32, h: f32, a: f32, b: f32) { .. } /// ``` declare_lint! { @@ -24,6 +26,30 @@ declare_lint! { "functions with too many arguments" } +/// **What it does:** Check for public functions that dereferences raw pointer arguments but are +/// not marked unsafe. +/// +/// **Why is this bad?** The function should probably be marked `unsafe`, since for an arbitrary +/// raw pointer, there is no way of telling for sure if it is valid. +/// +/// **Known problems:** +/// +/// * It does not check functions recursively so if the pointer is passed to a private non- +/// `unsafe` function which does the dereferencing, the lint won't trigger. +/// * It only checks for arguments whose type are raw pointers, not raw pointers got from an +/// argument in some other way (`fn foo(bar: &[*const u8])` or `some_argument.get_raw_ptr()`). +/// +/// **Example:** +/// +/// ```rust +/// pub fn foo(x: *const u8) { println!("{}", unsafe { *x }); } +/// ``` +declare_lint! { + pub NOT_UNSAFE_PTR_ARG_DEREF, + Warn, + "public functions dereferencing raw pointer arguments but not marked `unsafe`" +} + #[derive(Copy,Clone)] pub struct Functions { threshold: u64, @@ -37,29 +63,41 @@ impl Functions { impl LintPass for Functions { fn get_lints(&self) -> LintArray { - lint_array!(TOO_MANY_ARGUMENTS) + lint_array!(TOO_MANY_ARGUMENTS, NOT_UNSAFE_PTR_ARG_DEREF) } } impl LateLintPass for Functions { - fn check_fn(&mut self, cx: &LateContext, _: intravisit::FnKind, decl: &hir::FnDecl, _: &hir::Block, span: Span, - nodeid: ast::NodeId) { + fn check_fn(&mut self, cx: &LateContext, kind: intravisit::FnKind, decl: &hir::FnDecl, block: &hir::Block, span: Span, nodeid: ast::NodeId) { use rustc::hir::map::Node::*; - if let Some(NodeItem(ref item)) = cx.tcx.map.find(cx.tcx.map.get_parent_node(nodeid)) { - match item.node { - hir::ItemImpl(_, _, _, Some(_), _, _) | - hir::ItemDefaultImpl(..) => return, - _ => (), - } + let is_impl = if let Some(NodeItem(ref item)) = cx.tcx.map.find(cx.tcx.map.get_parent_node(nodeid)) { + matches!(item.node, hir::ItemImpl(_, _, _, Some(_), _, _) | hir::ItemDefaultImpl(..)) + } else { + false + }; + + let unsafety = match kind { + hir::intravisit::FnKind::ItemFn(_, _, unsafety, _, _, _, _) => unsafety, + hir::intravisit::FnKind::Method(_, sig, _, _) => sig.unsafety, + hir::intravisit::FnKind::Closure(_) => return, + }; + + // don't warn for implementations, it's not their fault + if !is_impl { + self.check_arg_number(cx, decl, span); } - self.check_arg_number(cx, decl, span); + self.check_raw_ptr(cx, unsafety, decl, block, nodeid); } fn check_trait_item(&mut self, cx: &LateContext, item: &hir::TraitItem) { - if let hir::MethodTraitItem(ref sig, _) = item.node { + if let hir::MethodTraitItem(ref sig, ref block) = item.node { self.check_arg_number(cx, &sig.decl, item.span); + + if let Some(ref block) = *block { + self.check_raw_ptr(cx, sig.unsafety, &sig.decl, block, item.id); + } } } } @@ -74,4 +112,75 @@ impl Functions { &format!("this function has too many arguments ({}/{})", args, self.threshold)); } } + + fn check_raw_ptr(&self, cx: &LateContext, unsafety: hir::Unsafety, decl: &hir::FnDecl, block: &hir::Block, nodeid: ast::NodeId) { + if unsafety == hir::Unsafety::Normal && cx.access_levels.is_exported(nodeid) { + let raw_ptrs = decl.inputs.iter().filter_map(|arg| raw_ptr_arg(cx, arg)).collect::>(); + + if !raw_ptrs.is_empty() { + let mut v = DerefVisitor { + cx: cx, + ptrs: raw_ptrs, + }; + + hir::intravisit::walk_block(&mut v, block); + } + } + } +} + +fn raw_ptr_arg(cx: &LateContext, arg: &hir::Arg) -> Option { + if let (&hir::PatKind::Binding(_, _, _), &hir::TyPtr(_)) = (&arg.pat.node, &arg.ty.node) { + cx.tcx.def_map.borrow().get(&arg.pat.id).map(|pr| pr.full_def().def_id()) + } else { + None + } +} + +struct DerefVisitor<'a, 'tcx: 'a> { + cx: &'a LateContext<'a, 'tcx>, + ptrs: HashSet, +} + +impl<'a, 'tcx, 'v> hir::intravisit::Visitor<'v> for DerefVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'v hir::Expr) { + match expr.node { + hir::ExprCall(ref f, ref args) => { + let ty = self.cx.tcx.expr_ty(f); + + if type_is_unsafe_function(ty) { + for arg in args { + self.check_arg(arg); + } + } + } + hir::ExprMethodCall(_, _, ref args) => { + let method_call = ty::MethodCall::expr(expr.id); + let base_type = self.cx.tcx.tables.borrow().method_map[&method_call].ty; + + if type_is_unsafe_function(base_type) { + for arg in args { + self.check_arg(arg); + } + } + } + hir::ExprUnary(hir::UnDeref, ref ptr) => self.check_arg(ptr), + _ => (), + } + + hir::intravisit::walk_expr(self, expr); + } +} + +impl<'a, 'tcx: 'a> DerefVisitor<'a, 'tcx> { + fn check_arg(&self, ptr: &hir::Expr) { + if let Some(def) = self.cx.tcx.def_map.borrow().get(&ptr.id) { + if self.ptrs.contains(&def.full_def().def_id()) { + span_lint(self.cx, + NOT_UNSAFE_PTR_ARG_DEREF, + ptr.span, + "this public function dereferences a raw pointer but is not marked `unsafe`"); + } + } + } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 040096dcbb4b..5ba52374135e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -322,6 +322,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { format::USELESS_FORMAT, formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING, formatting::SUSPICIOUS_ELSE_FORMATTING, + functions::NOT_UNSAFE_PTR_ARG_DEREF, functions::TOO_MANY_ARGUMENTS, identity_op::IDENTITY_OP, len_zero::LEN_WITHOUT_IS_EMPTY, diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 9eb54c2a0139..30bb6dfeb765 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -813,3 +813,12 @@ pub fn recover_for_loop(expr: &Expr) -> Option<(&Pat, &Expr, &Expr)> { }} None } + +/// Return whether the given type is an `unsafe` function. +pub fn type_is_unsafe_function(ty: ty::Ty) -> bool { + match ty.sty { + ty::TyFnDef(_, _, ref f) | + ty::TyFnPtr(ref f) => f.unsafety == Unsafety::Unsafe, + _ => false, + } +} diff --git a/tests/compile-fail/functions.rs b/tests/compile-fail/functions.rs index 2cc165686005..f7ee41d28169 100644 --- a/tests/compile-fail/functions.rs +++ b/tests/compile-fail/functions.rs @@ -3,20 +3,24 @@ #![deny(clippy)] #![allow(dead_code)] +#![allow(unused_unsafe)] +// TOO_MANY_ARGUMENTS fn good(_one: u32, _two: u32, _three: &str, _four: bool, _five: f32, _six: f32, _seven: bool) {} fn bad(_one: u32, _two: u32, _three: &str, _four: bool, _five: f32, _six: f32, _seven: bool, _eight: ()) { //~^ ERROR: this function has too many arguments (8/7) } -trait Foo { +pub trait Foo { fn good(_one: u32, _two: u32, _three: &str, _four: bool, _five: f32, _six: f32, _seven: bool); fn bad(_one: u32, _two: u32, _three: &str, _four: bool, _five: f32, _six: f32, _seven: bool, _eight: ()); //~^ ERROR: this function has too many arguments (8/7) + + fn ptr(p: *const u8); } -struct Bar; +pub struct Bar; impl Bar { fn good_method(_one: u32, _two: u32, _three: &str, _four: bool, _five: f32, _six: f32, _seven: bool) {} @@ -28,6 +32,56 @@ impl Bar { impl Foo for Bar { fn good(_one: u32, _two: u32, _three: &str, _four: bool, _five: f32, _six: f32, _seven: bool) {} fn bad(_one: u32, _two: u32, _three: &str, _four: bool, _five: f32, _six: f32, _seven: bool, _eight: ()) {} + + fn ptr(p: *const u8) { + println!("{}", unsafe { *p }); + //~^ ERROR: this public function dereferences a raw pointer but is not marked `unsafe` + println!("{:?}", unsafe { p.as_ref() }); + //~^ ERROR: this public function dereferences a raw pointer but is not marked `unsafe` + unsafe { std::ptr::read(p) }; + //~^ ERROR: this public function dereferences a raw pointer but is not marked `unsafe` + } +} + +// NOT_UNSAFE_PTR_ARG_DEREF + +fn private(p: *const u8) { + println!("{}", unsafe { *p }); +} + +pub fn public(p: *const u8) { + println!("{}", unsafe { *p }); + //~^ ERROR: this public function dereferences a raw pointer but is not marked `unsafe` + println!("{:?}", unsafe { p.as_ref() }); + //~^ ERROR: this public function dereferences a raw pointer but is not marked `unsafe` + unsafe { std::ptr::read(p) }; + //~^ ERROR: this public function dereferences a raw pointer but is not marked `unsafe` +} + +impl Bar { + fn private(self, p: *const u8) { + println!("{}", unsafe { *p }); + } + + pub fn public(self, p: *const u8) { + println!("{}", unsafe { *p }); + //~^ ERROR: this public function dereferences a raw pointer but is not marked `unsafe` + println!("{:?}", unsafe { p.as_ref() }); + //~^ ERROR: this public function dereferences a raw pointer but is not marked `unsafe` + unsafe { std::ptr::read(p) }; + //~^ ERROR: this public function dereferences a raw pointer but is not marked `unsafe` + } + + pub fn public_ok(self, p: *const u8) { + if !p.is_null() { + println!("{:p}", p); + } + } + + pub unsafe fn public_unsafe(self, p: *const u8) { + println!("{}", unsafe { *p }); + println!("{:?}", unsafe { p.as_ref() }); + } } fn main() {}