Merge pull request #149 from CoinFabrik/147-fix-unsafe-unwrap-and-uns…

…afe-expect-detectors

147 fix unsafe unwrap and unsafe expect detectors

detectors/unsafe-expect/src/lib.rs

@@ -7,12 +7,11 @@ extern crate rustc_span;
 use std::{collections::HashSet, hash::Hash};
 
 use if_chain::if_chain;
-use rustc_hir::BinOpKind;
 use rustc_hir::{
     def::Res,
     def_id::LocalDefId,
     intravisit::{walk_expr, FnKind, Visitor},
-    Body, Expr, ExprKind, FnDecl, HirId, Local, QPath, UnOp,
+    BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Local, PathSegment, QPath, UnOp,
 };
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_span::{sym, Span, Symbol};
@@ -196,7 +195,6 @@ impl UnsafeExpectVisitor<'_, '_> {
     fn handle_if_expressions(&mut self) {
         self.conditional_checker.iter().for_each(|checker| {
             if checker.check_type.should_halt_execution() {
-                println!("Halt: {:?}", checker.checked_expr_hir_id);
                 self.checked_exprs.insert(checker.checked_expr_hir_id);
             }
         });
@@ -225,22 +223,64 @@ impl UnsafeExpectVisitor<'_, '_> {
 
         true // If the stack is emptied without finding a non-literal, all elements are literals.
     }
+
+    fn is_method_call_unsafe(
+        &self,
+        path_segment: &PathSegment,
+        receiver: &Expr,
+        args: &[Expr],
+    ) -> bool {
+        if path_segment.ident.name == sym::expect {
+            return self
+                .get_expect_info(receiver)
+                .map_or(true, |id| !self.checked_exprs.contains(&id));
+        }
+
+        args.iter().any(|arg| self.contains_unsafe_method_call(arg))
+            || self.contains_unsafe_method_call(receiver)
+    }
+
+    fn contains_unsafe_method_call(&self, expr: &Expr) -> bool {
+        match &expr.kind {
+            ExprKind::MethodCall(path_segment, receiver, args, _) => {
+                self.is_method_call_unsafe(path_segment, receiver, args)
+            }
+            _ => false,
+        }
+    }
 }
 
 impl<'a, 'tcx> Visitor<'tcx> for UnsafeExpectVisitor<'a, 'tcx> {
     fn visit_local(&mut self, local: &'tcx Local<'tcx>) {
-        if_chain! {
-            if let Some(init) = local.init;
-            if let ExprKind::Call(func, args) = init.kind;
-            if let ExprKind::Path(QPath::Resolved(_, path)) = func.kind;
-            then {
-                let is_some_or_ok = path.segments.iter().any(|segment|
-                    matches!(segment.ident.name, sym::Some | sym::Ok)
-                );
-                let all_literals = args.iter().all(|arg| self.is_literal_or_composed_of_literals(arg));
-                if is_some_or_ok && all_literals {
-                    self.checked_exprs.insert(local.pat.hir_id);
+        if let Some(init) = local.init {
+            match init.kind {
+                ExprKind::MethodCall(path_segment, receiver, args, _) => {
+                    if self.is_method_call_unsafe(path_segment, receiver, args) {
+                        span_lint_and_help(
+                            self.cx,
+                            UNSAFE_EXPECT,
+                            local.span,
+                            LINT_MESSAGE,
+                            None,
+                            "Please, use a custom error instead of `expect`",
+                        );
+                    }
                 }
+                ExprKind::Call(func, args) => {
+                    if let ExprKind::Path(QPath::Resolved(_, path)) = func.kind {
+                        let is_some_or_ok = path
+                            .segments
+                            .iter()
+                            .any(|segment| matches!(segment.ident.name, sym::Some | sym::Ok));
+                        let all_literals = args
+                            .iter()
+                            .all(|arg| self.is_literal_or_composed_of_literals(arg));
+                        if is_some_or_ok && all_literals {
+                            self.checked_exprs.insert(local.pat.hir_id);
+                        }
+                    }
+                }
+                _ => {}
             }
         }
     }
@@ -249,10 +289,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafeExpectVisitor<'a, 'tcx> {
         // If we are inside an `if` or `if let` expression, we analyze its body
         if !self.conditional_checker.is_empty() {
             match &expr.kind {
-                ExprKind::Ret(..) => {
-                    println!("Ret");
-                    self.handle_if_expressions();
-                }
+                ExprKind::Ret(..) => self.handle_if_expressions(),
                 ExprKind::Call(func, _) if self.is_panic_inducing_call(func) => {
                     self.handle_if_expressions()
                 }

detectors/unsafe-unwrap/src/lib.rs

@@ -7,12 +7,11 @@ extern crate rustc_span;
 use std::{collections::HashSet, hash::Hash};
 
 use if_chain::if_chain;
-use rustc_hir::BinOpKind;
 use rustc_hir::{
     def::Res,
     def_id::LocalDefId,
     intravisit::{walk_expr, FnKind, Visitor},
-    Body, Expr, ExprKind, FnDecl, HirId, Local, QPath, UnOp,
+    BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Local, PathSegment, QPath, UnOp,
 };
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_span::{sym, Span, Symbol};
@@ -224,22 +223,64 @@ impl UnsafeUnwrapVisitor<'_, '_> {
 
         true // If the stack is emptied without finding a non-literal, all elements are literals.
     }
+
+    fn is_method_call_unsafe(
+        &self,
+        path_segment: &PathSegment,
+        receiver: &Expr,
+        args: &[Expr],
+    ) -> bool {
+        if path_segment.ident.name == sym::unwrap {
+            return self
+                .get_unwrap_info(receiver)
+                .map_or(true, |id| !self.checked_exprs.contains(&id));
+        }
+
+        args.iter().any(|arg| self.contains_unsafe_method_call(arg))
+            || self.contains_unsafe_method_call(receiver)
+    }
+
+    fn contains_unsafe_method_call(&self, expr: &Expr) -> bool {
+        match &expr.kind {
+            ExprKind::MethodCall(path_segment, receiver, args, _) => {
+                self.is_method_call_unsafe(path_segment, receiver, args)
+            }
+            _ => false,
+        }
+    }
 }
 
 impl<'a, 'tcx> Visitor<'tcx> for UnsafeUnwrapVisitor<'a, 'tcx> {
     fn visit_local(&mut self, local: &'tcx Local<'tcx>) {
-        if_chain! {
-            if let Some(init) = local.init;
-            if let ExprKind::Call(func, args) = init.kind;
-            if let ExprKind::Path(QPath::Resolved(_, path)) = func.kind;
-            then {
-                let is_some_or_ok = path.segments.iter().any(|segment|
-                    matches!(segment.ident.name, sym::Some | sym::Ok)
-                );
-                let all_literals = args.iter().all(|arg| self.is_literal_or_composed_of_literals(arg));
-                if is_some_or_ok && all_literals {
-                    self.checked_exprs.insert(local.pat.hir_id);
+        if let Some(init) = local.init {
+            match init.kind {
+                ExprKind::MethodCall(path_segment, receiver, args, _) => {
+                    if self.is_method_call_unsafe(path_segment, receiver, args) {
+                        span_lint_and_help(
+                            self.cx,
+                            UNSAFE_UNWRAP,
+                            local.span,
+                            LINT_MESSAGE,
+                            None,
+                            "Please, use a custom error instead of `unwrap`",
+                        );
+                    }
                 }
+                ExprKind::Call(func, args) => {
+                    if let ExprKind::Path(QPath::Resolved(_, path)) = func.kind {
+                        let is_some_or_ok = path
+                            .segments
+                            .iter()
+                            .any(|segment| matches!(segment.ident.name, sym::Some | sym::Ok));
+                        let all_literals = args
+                            .iter()
+                            .all(|arg| self.is_literal_or_composed_of_literals(arg));
+                        if is_some_or_ok && all_literals {
+                            self.checked_exprs.insert(local.pat.hir_id);
+                        }
+                    }
+                }
+                _ => {}
             }
         }
     }

scripts/run-tests.py

@@ -34,9 +34,7 @@ def run_tests(detector):
 
 def run_unit_tests(root):
     start_time = time.time()
-    returncode, stdout, _ = run_subprocess(
-        ["cargo", "test", "--all-features", "--all"], root
-    )
+    returncode, stdout, _ = run_subprocess(["cargo", "test", "--all-features"], root)
     print_results(
         returncode,
         stdout,

test-cases/unsafe-expect/unsafe-expect-2/remediated-example/Cargo.toml

@@ -7,24 +7,10 @@ version = "0.1.0"
 crate-type = ["cdylib"]
 
 [dependencies]
-soroban-sdk = { version = "=20.0.0" }
+soroban-sdk = { workspace = true }
 
 [dev_dependencies]
-soroban-sdk = { version = "=20.0.0", features = ["testutils"] }
+soroban-sdk = { workspace = true, features = ["testutils"] }
 
 [features]
 testutils = ["soroban-sdk/testutils"]
-
-[profile.release]
-codegen-units = 1
-debug = 0
-debug-assertions = false
-lto = true
-opt-level = "z"
-overflow-checks = true
-panic = "abort"
-strip = "symbols"
-
-[profile.release-with-logs]
-debug-assertions = true
-inherits = "release"

test-cases/unsafe-expect/unsafe-expect-2/vulnerable-example/Cargo.toml

@@ -7,24 +7,10 @@ version = "0.1.0"
 crate-type = ["cdylib"]
 
 [dependencies]
-soroban-sdk = { version = "=20.0.0" }
+soroban-sdk = { workspace = true }
 
 [dev_dependencies]
-soroban-sdk = { version = "=20.0.0", features = ["testutils"] }
+soroban-sdk = { workspace = true, features = ["testutils"] }
 
 [features]
 testutils = ["soroban-sdk/testutils"]
-
-[profile.release]
-codegen-units = 1
-debug = 0
-debug-assertions = false
-lto = true
-opt-level = "z"
-overflow-checks = true
-panic = "abort"
-strip = "symbols"
-
-[profile.release-with-logs]
-debug-assertions = true
-inherits = "release"

test-cases/unsafe-expect/unsafe-expect-3/remediated-example/Cargo.toml

@@ -7,24 +7,10 @@ version = "0.1.0"
 crate-type = ["cdylib"]
 
 [dependencies]
-soroban-sdk = { version = "=20.0.0" }
+soroban-sdk = { workspace = true }
 
 [dev_dependencies]
-soroban-sdk = { version = "=20.0.0", features = ["testutils"] }
+soroban-sdk = { workspace = true, features = ["testutils"] }
 
 [features]
 testutils = ["soroban-sdk/testutils"]
-
-[profile.release]
-codegen-units = 1
-debug = 0
-debug-assertions = false
-lto = true
-opt-level = "z"
-overflow-checks = true
-panic = "abort"
-strip = "symbols"
-
-[profile.release-with-logs]
-debug-assertions = true
-inherits = "release"

test-cases/unsafe-expect/unsafe-expect-3/vulnerable-example/Cargo.toml

@@ -7,24 +7,10 @@ version = "0.1.0"
 crate-type = ["cdylib"]
 
 [dependencies]
-soroban-sdk = { version = "=20.0.0" }
+soroban-sdk = { workspace = true }
 
 [dev_dependencies]
-soroban-sdk = { version = "=20.0.0", features = ["testutils"] }
+soroban-sdk = { workspace = true, features = ["testutils"] }
 
 [features]
 testutils = ["soroban-sdk/testutils"]
-
-[profile.release]
-codegen-units = 1
-debug = 0
-debug-assertions = false
-lto = true
-opt-level = "z"
-overflow-checks = true
-panic = "abort"
-strip = "symbols"
-
-[profile.release-with-logs]
-debug-assertions = true
-inherits = "release"

test-cases/unsafe-expect/unsafe-expect-4/remediated-example/Cargo.toml

@@ -7,24 +7,10 @@ version = "0.1.0"
 crate-type = ["cdylib"]
 
 [dependencies]
-soroban-sdk = { version = "=20.0.0" }
+soroban-sdk = { workspace = true }
 
 [dev_dependencies]
-soroban-sdk = { version = "=20.0.0", features = ["testutils"] }
+soroban-sdk = { workspace = true, features = ["testutils"] }
 
 [features]
 testutils = ["soroban-sdk/testutils"]
-
-[profile.release]
-codegen-units = 1
-debug = 0
-debug-assertions = false
-lto = true
-opt-level = "z"
-overflow-checks = true
-panic = "abort"
-strip = "symbols"
-
-[profile.release-with-logs]
-debug-assertions = true
-inherits = "release"

test-cases/unsafe-expect/unsafe-expect-4/vulnerable-example/Cargo.toml

@@ -7,24 +7,10 @@ version = "0.1.0"
 crate-type = ["cdylib"]
 
 [dependencies]
-soroban-sdk = { version = "=20.0.0" }
+soroban-sdk = { workspace = true }
 
 [dev_dependencies]
-soroban-sdk = { version = "=20.0.0", features = ["testutils"] }
+soroban-sdk = { workspace = true, features = ["testutils"] }
 
 [features]
 testutils = ["soroban-sdk/testutils"]
-
-[profile.release]
-codegen-units = 1
-debug = 0
-debug-assertions = false
-lto = true
-opt-level = "z"
-overflow-checks = true
-panic = "abort"
-strip = "symbols"
-
-[profile.release-with-logs]
-debug-assertions = true
-inherits = "release"

test-cases/unsafe-expect/unsafe-expect-5/remediated-example/Cargo.toml

@@ -0,0 +1,16 @@
+[package]
+edition = "2021"
+name = "unsafe-expect-remediated-5"
+version = "0.1.0"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+soroban-sdk = { workspace = true }
+
+[dev_dependencies]
+soroban-sdk = { workspace = true, features = ["testutils"] }
+
+[features]
+testutils = ["soroban-sdk/testutils"]