Merge pull request #37 from CoinFabrik/core-mem-forget

11 `core::mem::forget()` detector

.github/workflows/test-detectors.yml

@@ -97,6 +97,7 @@ jobs:
           - macos-latest
         test:
           [
+            "core-mem-forget",
             "divide-before-multiply",
             "insufficiently-random-values",
             "overflow-check",

detectors/core-mem-forget/Cargo.toml

@@ -0,0 +1,20 @@
+[package]
+name = "core-mem-forget"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+clippy_utils = { workspace = true }
+dylint_linting = { workspace = true }
+if_chain = { workspace = true }
+
+scout-audit-internal = { workspace = true }
+
+[dev-dependencies]
+dylint_testing = { workspace = true }
+
+[package.metadata.rust-analyzer]
+rustc_private = true

detectors/core-mem-forget/src/lib.rs

@@ -0,0 +1,98 @@
+#![feature(rustc_private)]
+
+extern crate rustc_ast;
+extern crate rustc_hir;
+extern crate rustc_span;
+
+use if_chain::if_chain;
+use rustc_ast::{Expr, ExprKind, Item, NodeId};
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_span::sym;
+use scout_audit_internal::Detector;
+
+dylint_linting::impl_pre_expansion_lint! {
+    /// ### What it does
+    /// Checks for `core::mem::forget` usage.
+    /// ### Why is this bad?
+    /// This is a bad practice because it can lead to memory leaks, resource leaks and logic errors.
+    /// ### Example
+    /// ```rust
+    /// pub fn forget_something(n: WithoutCopy) -> u64 {
+    ///     core::mem::forget(n);
+    ///     0
+    /// }
+    /// ```
+    ///
+    /// Use instead:
+    /// ```rust
+    /// pub fn forget_something(n: WithoutCopy) -> u64 {
+    ///     let _ = n;
+    ///     0
+    /// }
+    /// ```
+
+    pub CORE_MEM_FORGET,
+    Warn,
+    Detector::CoreMemForget.get_lint_message(),
+    CoreMemForget::default()
+}
+
+#[derive(Default)]
+pub struct CoreMemForget {
+    stack: Vec<NodeId>,
+}
+
+impl EarlyLintPass for CoreMemForget {
+    fn check_item(&mut self, _cx: &EarlyContext, item: &Item) {
+        if self.in_test_item() || is_test_item(item) {
+            self.stack.push(item.id);
+        }
+    }
+
+    fn check_expr(&mut self, cx: &EarlyContext, expr: &Expr) {
+        if_chain! {
+            if !self.in_test_item();
+            if let ExprKind::Call(a, _) = &expr.kind;
+            if let ExprKind::Path(_, path) = &a.kind;
+            if path.segments.len() == 3;
+            if path.segments[0].ident.name.to_string() == "core";
+            if path.segments[1].ident.name.to_string() == "mem";
+            if path.segments[2].ident.name.to_string() == "forget";
+            then {
+                Detector::CoreMemForget.span_lint_and_help(
+                    cx,
+                    CORE_MEM_FORGET,
+                    expr.span,
+                    "Instead, use the `let _ = ...` pattern or `.drop` method to forget the value.",
+                );
+            }
+        }
+    }
+}
+
+fn is_test_item(item: &Item) -> bool {
+    item.attrs.iter().any(|attr| {
+        if attr.has_name(sym::test) {
+            true
+        } else {
+            if_chain! {
+                if attr.has_name(sym::cfg);
+                if let Some(items) = attr.meta_item_list();
+                if let [item] = items.as_slice();
+                if let Some(feature_item) = item.meta_item();
+                if feature_item.has_name(sym::test);
+                then {
+                    true
+                } else {
+                    false
+                }
+            }
+        }
+    })
+}
+
+impl CoreMemForget {
+    fn in_test_item(&self) -> bool {
+        !self.stack.is_empty()
+    }
+}

scout-audit-internal/src/detector.rs

@@ -26,6 +26,7 @@ use strum::{Display, EnumIter};
 #[derive(Debug, Display, Clone, EnumIter, PartialEq, Eq, Hash)]
 #[strum(serialize_all = "kebab-case")]
 pub enum Detector {
+    CoreMemForget,
     DivideBeforeMultiply,
     InsufficientlyRandomValues,
     OverflowCheck,
@@ -38,14 +39,15 @@ impl Detector {
     /// Returns the lint message for the detector.
     pub const fn get_lint_message(&self) -> &'static str {
         match self {
-            Detector::InsufficientlyRandomValues => INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE,
+            Detector::CoreMemForget => CORE_MEM_FORGET_LINT_MESSAGE,
             Detector::DivideBeforeMultiply => DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE,
+            Detector::InsufficientlyRandomValues => INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE,
             Detector::OverflowCheck => OVERFLOW_CHECK_LINT_MESSAGE,
             Detector::UnprotectedUpdateCurrentContractWasm => {
-                UNPROTECTED_UPDATE_CURRENT_CONTRACT_MESSAGE
+                UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE
             }
             Detector::UnsafeExpect => UNSAFE_EXPECT_LINT_MESSAGE,
-            Detector::UnsafeUnwrap => UNSAFE_UNWRAP_MESSAGE,
+            Detector::UnsafeUnwrap => UNSAFE_UNWRAP_LINT_MESSAGE,
         }
     }
 

scout-audit-internal/src/detector/lint_message.rs

@@ -1,8 +1,10 @@
-pub const INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE: &str = "Use env.prng() to generate random numbers, and remember that all random numbers are under the control of validators";
+pub const CORE_MEM_FORGET_LINT_MESSAGE: &str =
+    "Use the `let _ = ...` pattern or `.drop()` method to forget the value";
 pub const DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE: &str =
     "Division before multiplication might result in a loss of precision";
+pub const INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE: &str = "Use env.prng() to generate random numbers, and remember that all random numbers are under the control of validators";
 pub const OVERFLOW_CHECK_LINT_MESSAGE: &str = "Use `overflow-checks = true` in Cargo.toml profile";
-pub const UNPROTECTED_UPDATE_CURRENT_CONTRACT_MESSAGE: &str =
+pub const UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE: &str =
     "This update_current_contract_wasm is called without access control";
 pub const UNSAFE_EXPECT_LINT_MESSAGE: &str = "Unsafe usage of `expect`";
-pub const UNSAFE_UNWRAP_MESSAGE: &str = "Unsafe usage of `unwrap`";
+pub const UNSAFE_UNWRAP_LINT_MESSAGE: &str = "Unsafe usage of `unwrap`";

test-cases/core-mem-forget/core-mem-forget-1/remediated-example/Cargo.toml

@@ -0,0 +1,30 @@
+[package]
+name = "core-mem-forget-1-remediated"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+soroban-sdk = { version = "20.0.0-rc2" }
+
+[dev_dependencies]
+soroban-sdk = { version = "20.0.0-rc2", features = ["testutils"] }
+
+[features]
+testutils = ["soroban-sdk/testutils"]
+
+[profile.release]
+opt-level = "z"
+overflow-checks = true
+debug = 0
+strip = "symbols"
+debug-assertions = false
+panic = "abort"
+codegen-units = 1
+lto = true
+
+[profile.release-with-logs]
+inherits = "release"
+debug-assertions = true

test-cases/core-mem-forget/core-mem-forget-1/remediated-example/src/lib.rs

@@ -0,0 +1,37 @@
+#![no_std]
+use soroban_sdk::{contract, contractimpl, contracttype};
+
+#[contract]
+pub struct CoreMemForget;
+
+#[contracttype]
+#[derive(Eq, PartialEq)]
+pub struct WithoutCopy {
+    pub a: u64,
+    pub b: u64,
+}
+
+#[contractimpl]
+impl CoreMemForget {
+    pub fn forget_something(n: WithoutCopy) -> u64 {
+        let _ = n;
+        0
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::*;
+
+    #[test]
+    fn test_forget_something() {
+        // Given
+        let test_value: WithoutCopy = WithoutCopy { a: 80, b: 60 };
+
+        // When
+        let result = CoreMemForget::forget_something(test_value);
+
+        // Then
+        assert_eq!(result, 0);
+    }
+}

test-cases/core-mem-forget/core-mem-forget-1/vulnerable-example/Cargo.toml

@@ -0,0 +1,30 @@
+[package]
+name = "core-mem-forget-1-vulnerable"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+soroban-sdk = { version = "20.0.0-rc2" }
+
+[dev_dependencies]
+soroban-sdk = { version = "20.0.0-rc2", features = ["testutils"] }
+
+[features]
+testutils = ["soroban-sdk/testutils"]
+
+[profile.release]
+opt-level = "z"
+overflow-checks = true
+debug = 0
+strip = "symbols"
+debug-assertions = false
+panic = "abort"
+codegen-units = 1
+lto = true
+
+[profile.release-with-logs]
+inherits = "release"
+debug-assertions = true

test-cases/core-mem-forget/core-mem-forget-1/vulnerable-example/src/lib.rs

@@ -0,0 +1,37 @@
+#![no_std]
+use soroban_sdk::{contract, contractimpl, contracttype};
+
+#[contract]
+pub struct CoreMemForget;
+
+#[contracttype]
+#[derive(Eq, PartialEq)]
+pub struct WithoutCopy {
+    pub a: u64,
+    pub b: u64,
+}
+
+#[contractimpl]
+impl CoreMemForget {
+    pub fn forget_something(n: WithoutCopy) -> u64 {
+        core::mem::forget(n);
+        0
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::*;
+
+    #[test]
+    fn test_forget_something() {
+        // Given
+        let test_value: WithoutCopy = WithoutCopy { a: 80, b: 60 };
+
+        // When
+        let result = CoreMemForget::forget_something(test_value);
+
+        // Then
+        assert_eq!(result, 0);
+    }
+}