Rebase #991

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

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)`

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::<HashSet<_>>();
+
+            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<hir::def_id::DefId> {
+    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<hir::def_id::DefId>,
+}
+
+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`");
+            }
+        }
+    }
 }

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,

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,
+    }
+}

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() {}