From a79fc9af54e65e49b392f86ec5329ac0278c7f83 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Wed, 24 Jul 2024 12:21:50 -0300 Subject: [PATCH 1/8] Add detector --- detectors/Cargo.toml | 2 +- .../integer-overflow-or-underflow/Cargo.toml | 15 ++ .../integer-overflow-or-underflow/src/lib.rs | 239 ++++++++++++++++++ 3 files changed, 255 insertions(+), 1 deletion(-) create mode 100644 detectors/integer-overflow-or-underflow/Cargo.toml create mode 100644 detectors/integer-overflow-or-underflow/src/lib.rs diff --git a/detectors/Cargo.toml b/detectors/Cargo.toml index 8c1b5d7e..a03df777 100644 --- a/detectors/Cargo.toml +++ b/detectors/Cargo.toml @@ -1,5 +1,5 @@ [workspace] -exclude = [".cargo", "target"] +exclude = [".cargo", ".vscode", "target"] members = ["*"] resolver = "2" diff --git a/detectors/integer-overflow-or-underflow/Cargo.toml b/detectors/integer-overflow-or-underflow/Cargo.toml new file mode 100644 index 00000000..7c58dfdc --- /dev/null +++ b/detectors/integer-overflow-or-underflow/Cargo.toml @@ -0,0 +1,15 @@ +[package] +edition = "2021" +name = "integer-overflow-or-underflow" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { workspace = true } +dylint_linting = { workspace = true } +if_chain = { workspace = true } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/integer-overflow-or-underflow/src/lib.rs b/detectors/integer-overflow-or-underflow/src/lib.rs new file mode 100644 index 00000000..83a36f07 --- /dev/null +++ b/detectors/integer-overflow-or-underflow/src/lib.rs @@ -0,0 +1,239 @@ +#![feature(rustc_private)] + +extern crate rustc_hir; +extern crate rustc_span; + +use clippy_utils::consts::constant_simple; +use clippy_utils::is_integer_literal; +use rustc_hir::{self as hir, Expr, ExprKind, UnOp}; +use rustc_lint::LateContext; +use rustc_lint::LateLintPass; +use rustc_span::Span; + +pub const LINT_MESSAGE: &str = "Potential for integer arithmetic overflow/underflow. Consider checked, wrapping or saturating arithmetic."; + +dylint_linting::impl_late_lint! { + /// ### What it does + /// Checks for integer arithmetic operations which could overflow or panic. + /// + /// Specifically, checks for any operators (`+`, `-`, `*`, `<<`, etc) which are capable + /// of overflowing according to the [Rust + /// Reference](https://doc.rust-lang.org/reference/expressions/operator-expr.html#overflow), + /// or which can panic (`/`, `%`). No bounds analysis or sophisticated reasoning is + /// attempted. + /// + /// ### Why is this bad? + /// Integer overflow will trigger a panic in debug builds or will wrap in + /// release mode. Division by zero will cause a panic in either mode. In some applications one + /// wants explicitly checked, wrapping or saturating arithmetic. + /// + /// ### Example + /// ```rust + /// # let a = 0; + /// a + 1; + /// ``` + pub INTEGER_OVERFLOW_UNDERFLOW, + Warn, + LINT_MESSAGE, + IntegerOverflowUnderflow::default(), + { + name: "Integer Overflow/Underflow", + long_message: "An overflow/underflow is typically caught and generates an error. When it is not caught, the operation will result in an inexact result which could lead to serious problems.\n In Ink! 5.0.0, using raw math operations will result in `cargo contract build` failing with an error message.", + severity: "Critical", + help: "https://coinfabrik.github.io/scout-soroban/docs/vulnerabilities/integer-overflow-or-underflow", + vulnerability_class: "Arithmetic", + } +} + +#[derive(Default)] +pub struct IntegerOverflowUnderflow { + arithmetic_context: ArithmeticContext, +} + +impl IntegerOverflowUnderflow { + pub fn new() -> Self { + Self { + arithmetic_context: ArithmeticContext::default(), + } + } +} + +impl<'tcx> LateLintPass<'tcx> for IntegerOverflowUnderflow { + fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { + match e.kind { + ExprKind::Binary(op, lhs, rhs) => { + self.arithmetic_context + .check_binary(cx, e, op.node, lhs, rhs); + } + ExprKind::AssignOp(op, lhs, rhs) => { + self.arithmetic_context + .check_binary(cx, e, op.node, lhs, rhs); + } + ExprKind::Unary(op, arg) => { + if op == UnOp::Neg { + self.arithmetic_context.check_negate(cx, e, arg); + } + } + _ => (), + } + } + + fn check_expr_post(&mut self, _: &LateContext<'_>, e: &Expr<'_>) { + self.arithmetic_context.expr_post(e.hir_id); + } + + fn check_body(&mut self, cx: &LateContext<'tcx>, body: &rustc_hir::Body<'tcx>) { + self.arithmetic_context.enter_body(cx, body); + } + + fn check_body_post(&mut self, cx: &LateContext<'tcx>, body: &rustc_hir::Body<'tcx>) { + self.arithmetic_context.body_post(cx, body); + } +} + +#[derive(Default)] +pub struct ArithmeticContext { + expr_id: Option, + /// This field is used to check whether expressions are constants, such as in enum discriminants + /// and consts + const_span: Option, +} +impl ArithmeticContext { + fn skip_expr(&mut self, e: &hir::Expr<'_>) -> bool { + self.expr_id.is_some() || self.const_span.map_or(false, |span| span.contains(e.span)) + } + + pub fn check_binary<'tcx>( + &mut self, + cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'_>, + op: hir::BinOpKind, + l: &'tcx hir::Expr<'_>, + r: &'tcx hir::Expr<'_>, + ) { + if self.skip_expr(expr) { + return; + } + match op { + hir::BinOpKind::And + | hir::BinOpKind::Or + | hir::BinOpKind::BitAnd + | hir::BinOpKind::BitOr + | hir::BinOpKind::BitXor + | hir::BinOpKind::Eq + | hir::BinOpKind::Lt + | hir::BinOpKind::Le + | hir::BinOpKind::Ne + | hir::BinOpKind::Ge + | hir::BinOpKind::Gt => return, + _ => (), + } + + let (l_ty, r_ty) = ( + cx.typeck_results().expr_ty(l), + cx.typeck_results().expr_ty(r), + ); + if l_ty.peel_refs().is_integral() && r_ty.peel_refs().is_integral() { + match op { + hir::BinOpKind::Div | hir::BinOpKind::Rem => match &r.kind { + hir::ExprKind::Lit(_lit) => (), + hir::ExprKind::Unary(hir::UnOp::Neg, expr) => { + if is_integer_literal(expr, 1) { + clippy_utils::diagnostics::span_lint_and_help( + cx, + INTEGER_OVERFLOW_UNDERFLOW, + expr.span, + LINT_MESSAGE, + None, + "Potential for integer arithmetic overflow/underflow in unary operation with negative expression. Consider checked, wrapping or saturating arithmetic." + ); + self.expr_id = Some(expr.hir_id); + } + } + _ => { + clippy_utils::diagnostics::span_lint_and_help( + cx, + INTEGER_OVERFLOW_UNDERFLOW, + expr.span, + LINT_MESSAGE, + None, + format!("Potential for integer arithmetic overflow/underflow in operation '{}'. Consider checked, wrapping or saturating arithmetic.", op.as_str()), + ); + self.expr_id = Some(expr.hir_id); + } + }, + _ => { + clippy_utils::diagnostics::span_lint_and_help( + cx, + INTEGER_OVERFLOW_UNDERFLOW, + expr.span, + LINT_MESSAGE, + None, + format!("Potential for integer arithmetic overflow/underflow in operation '{}'. Consider checked, wrapping or saturating arithmetic.", op.as_str()), + ); + self.expr_id = Some(expr.hir_id); + } + } + } + } + + pub fn check_negate<'tcx>( + &mut self, + cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'_>, + arg: &'tcx hir::Expr<'_>, + ) { + if self.skip_expr(expr) { + return; + } + let ty = cx.typeck_results().expr_ty(arg); + if constant_simple(cx, cx.typeck_results(), expr).is_none() && ty.is_integral() { + clippy_utils::diagnostics::span_lint_and_help( + cx, + INTEGER_OVERFLOW_UNDERFLOW, + expr.span, + LINT_MESSAGE, + None, + "Potential for integer arithmetic overflow/underflow. Consider checked, wrapping or saturating arithmetic.", + ); + self.expr_id = Some(expr.hir_id); + } + } + + pub fn expr_post(&mut self, id: hir::HirId) { + if Some(id) == self.expr_id { + self.expr_id = None; + } + } + + pub fn enter_body(&mut self, cx: &LateContext<'_>, body: &hir::Body<'_>) { + let body_owner = cx.tcx.hir().body_owner(body.id()); + let body_owner_def_id = cx.tcx.hir().body_owner_def_id(body.id()); + + match cx.tcx.hir().body_owner_kind(body_owner_def_id) { + hir::BodyOwnerKind::Static(_) | hir::BodyOwnerKind::Const { .. } => { + let body_span = cx.tcx.hir().span_with_body(body_owner); + + if let Some(span) = self.const_span { + if span.contains(body_span) { + return; + } + } + self.const_span = Some(body_span); + } + hir::BodyOwnerKind::Fn | hir::BodyOwnerKind::Closure => (), + } + } + + pub fn body_post(&mut self, cx: &LateContext<'_>, body: &hir::Body<'_>) { + let body_owner = cx.tcx.hir().body_owner(body.id()); + let body_span = cx.tcx.hir().span_with_body(body_owner); + + if let Some(span) = self.const_span { + if span.contains(body_span) { + return; + } + } + self.const_span = None; + } +} From 955b5147064b1f897a4af5805fe65364eec4e4f2 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Wed, 24 Jul 2024 15:09:32 -0300 Subject: [PATCH 2/8] Add test-cases --- .../integer-overflow-or-underflow/Cargo.toml | 21 +++ .../remediated-example/Cargo.toml | 16 +++ .../remediated-example/src/lib.rs | 96 ++++++++++++++ .../vulnerable-example/Cargo.toml | 16 +++ .../vulnerable-example/src/lib.rs | 82 ++++++++++++ .../remediated-example/Cargo.toml | 16 +++ .../remediated-example/src/lib.rs | 121 ++++++++++++++++++ .../vulnerable-example/Cargo.toml | 16 +++ .../vulnerable-example/src/lib.rs | 103 +++++++++++++++ 9 files changed, 487 insertions(+) create mode 100644 test-cases/integer-overflow-or-underflow/Cargo.toml create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/Cargo.toml create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/src/lib.rs create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/Cargo.toml create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/src/lib.rs create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/remediated-example/Cargo.toml create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/remediated-example/src/lib.rs create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/vulnerable-example/Cargo.toml create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/vulnerable-example/src/lib.rs diff --git a/test-cases/integer-overflow-or-underflow/Cargo.toml b/test-cases/integer-overflow-or-underflow/Cargo.toml new file mode 100644 index 00000000..ca2a186c --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/Cargo.toml @@ -0,0 +1,21 @@ +[workspace] +exclude = [".cargo", "target"] +members = ["integer-overflow-or-underflow-*/*"] +resolver = "2" + +[workspace.dependencies] +soroban-sdk = { version = "=21.3.0" } + +[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" diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/Cargo.toml b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..0c8cdb76 --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "integer-overflow-or-underflow-remediated-1" +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"] diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..ad0fd83f --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/src/lib.rs @@ -0,0 +1,96 @@ +#![no_std] +use soroban_sdk::{contract, contracterror, contractimpl, symbol_short, Env, Symbol}; + +#[contract] +pub struct IntegerOverflowUnderflow; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + OverflowError = 1, + UnderflowError = 2, +} + +#[contractimpl] +impl IntegerOverflowUnderflow { + const VALUE: Symbol = symbol_short!("VALUE"); + + pub fn initialize(env: Env, value: u32) { + env.storage().temporary().set(&Self::VALUE, &value); + } + + pub fn add(env: Env, value: u32) -> Result<(), Error> { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = match current.checked_add(value) { + Some(value) => value, + None => return Err(Error::OverflowError), + }; + env.storage().temporary().set(&Self::VALUE, &new_value); + Ok(()) + } + + pub fn sub(env: Env, value: u32) -> Result<(), Error> { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = match current.checked_sub(value) { + Some(value) => value, + None => return Err(Error::UnderflowError), + }; + env.storage().temporary().set(&Self::VALUE, &new_value); + Ok(()) + } + + pub fn get(env: Env) -> u32 { + env.storage().temporary().get(&Self::VALUE).unwrap_or(0) + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::Env; + + #[test] + fn test_initialize() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&42); + + // Then + assert_eq!(client.get(), 42); + } + + #[test] + fn test_add_overflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&u32::MAX); + let result = client.try_add(&1); + + // Then + assert_eq!(result, Err(Ok(Error::OverflowError))); + } + + #[test] + fn test_sub_underflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&0); + let result = client.try_sub(&1); + + // Then + assert_eq!(result, Err(Ok(Error::UnderflowError))); + } +} diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/Cargo.toml b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..d143c46a --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "integer-overflow-or-underflow-vulnerable-1" +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"] diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..00f757e3 --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/src/lib.rs @@ -0,0 +1,82 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, symbol_short, Env, Symbol}; + +#[contract] +pub struct IntegerOverflowUnderflow; + +#[contractimpl] +impl IntegerOverflowUnderflow { + const VALUE: Symbol = symbol_short!("VALUE"); + + pub fn initialize(env: Env, value: u32) { + env.storage().temporary().set(&Self::VALUE, &value); + } + + pub fn add(env: Env, value: u32) { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = current + value; + env.storage().temporary().set(&Self::VALUE, &new_value); + } + + pub fn sub(env: Env, value: u32) { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = current - value; + env.storage().temporary().set(&Self::VALUE, &new_value); + } + + pub fn get(env: Env) -> u32 { + env.storage().temporary().get(&Self::VALUE).unwrap_or(0) + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::Env; + + #[test] + fn test_initialize() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&42); + + // Then + assert_eq!(client.get(), 42); + } + + #[test] + #[should_panic(expected = "attempt to add with overflow")] + fn test_add_overflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&u32::MAX); + client.add(&1); + + // Then + // Panic + } + + #[test] + #[should_panic(expected = "attempt to subtract with overflow")] + fn test_sub_underflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&0); + client.sub(&1); + + // Then + // Panic + } +} diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/remediated-example/Cargo.toml b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/remediated-example/Cargo.toml new file mode 100644 index 00000000..abbb5ad6 --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/remediated-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "integer-overflow-or-underflow-remediated-2" +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"] diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/remediated-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/remediated-example/src/lib.rs new file mode 100644 index 00000000..e0347b80 --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/remediated-example/src/lib.rs @@ -0,0 +1,121 @@ +#![no_std] +use soroban_sdk::{contract, contracterror, contractimpl, symbol_short, Env, Symbol}; + +#[contract] +pub struct IntegerOverflowUnderflow; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + OverflowError = 1, + UnderflowError = 2, +} + +#[contractimpl] +impl IntegerOverflowUnderflow { + const VALUE: Symbol = symbol_short!("VALUE"); + + pub fn initialize(env: Env, value: u32) { + env.storage().temporary().set(&Self::VALUE, &value); + } + + pub fn mul(env: Env, value: u32) -> Result<(), Error> { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = match current.checked_mul(value) { + Some(value) => value, + None => return Err(Error::OverflowError), + }; + env.storage().temporary().set(&Self::VALUE, &new_value); + Ok(()) + } + + pub fn pow(env: Env, value: u32) -> Result<(), Error> { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = match current.checked_pow(value) { + Some(value) => value, + None => return Err(Error::OverflowError), + }; + env.storage().temporary().set(&Self::VALUE, &new_value); + Ok(()) + } + + pub fn neg(env: Env) -> Result<(), Error> { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = match current.checked_neg() { + Some(value) => value, + None => return Err(Error::UnderflowError), + }; + env.storage().temporary().set(&Self::VALUE, &new_value); + Ok(()) + } + + pub fn get(env: Env) -> u32 { + env.storage().temporary().get(&Self::VALUE).unwrap_or(0) + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::Env; + + #[test] + fn test_initialize() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&42); + + // Then + assert_eq!(client.get(), 42); + } + + #[test] + fn test_mul_overflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&u32::MAX); + let result = client.try_mul(&2); + + // Then + assert_eq!(result, Err(Ok(Error::OverflowError))); + } + + #[test] + fn test_pow_overflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&2); + let result = client.try_pow(&u32::MAX); + + // Then + assert_eq!(result, Err(Ok(Error::OverflowError))); + } + + #[test] + fn test_neg() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&u32::MAX); + let result = client.try_neg(); + + // Then + assert_eq!(result, Err(Ok(Error::UnderflowError))); + } +} diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/vulnerable-example/Cargo.toml b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..d1571812 --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/vulnerable-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "integer-overflow-or-underflow-vulnerable-2" +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"] diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/vulnerable-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..20e7c11a --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/vulnerable-example/src/lib.rs @@ -0,0 +1,103 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, symbol_short, Env, Symbol}; + +#[contract] +pub struct IntegerOverflowUnderflow; + +#[contractimpl] +impl IntegerOverflowUnderflow { + const VALUE: Symbol = symbol_short!("VALUE"); + + pub fn initialize(env: Env, value: u32) { + env.storage().temporary().set(&Self::VALUE, &value); + } + + pub fn mul(env: Env, value: u32) { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = current * value; + env.storage().temporary().set(&Self::VALUE, &new_value); + } + + pub fn pow(env: Env, value: u32) { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = current.pow(value); + env.storage().temporary().set(&Self::VALUE, &new_value); + } + + pub fn neg(env: Env) { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = current.wrapping_neg(); + env.storage().temporary().set(&Self::VALUE, &new_value); + } + + pub fn get(env: Env) -> u32 { + env.storage().temporary().get(&Self::VALUE).unwrap_or(0) + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::Env; + + #[test] + fn test_initialize() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&42); + + // Then + assert_eq!(client.get(), 42); + } + + #[test] + #[should_panic(expected = "attempt to multiply with overflow")] + fn test_mul_overflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&u32::MAX); + client.mul(&2); + + // Then + // Panic + } + + #[test] + #[should_panic(expected = "attempt to multiply with overflow")] + fn test_pow_overflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&2); + client.pow(&u32::MAX); + + // Then + // Panic + } + + #[test] + fn test_neg() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&u32::MIN); + client.neg(); + + // Then + assert_eq!(client.get(), u32::MIN.wrapping_neg()); + } +} From 606b20986eab44eb9a986284bde9932f87b4a074 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 25 Jul 2024 15:16:39 -0300 Subject: [PATCH 3/8] Edit detector and test-cases --- .../integer-overflow-or-underflow/src/lib.rs | 360 +++++++++--------- .../integer-overflow-or-underflow/Cargo.toml | 3 + .../remediated-example/src/lib.rs | 40 -- .../vulnerable-example/src/lib.rs | 36 -- .../remediated-example/src/lib.rs | 77 +--- .../vulnerable-example/src/lib.rs | 69 +--- .../remediated-example/Cargo.toml | 16 + .../remediated-example/src/lib.rs | 57 +++ .../vulnerable-example/Cargo.toml | 17 + .../vulnerable-example/src/lib.rs | 47 +++ .../remediated-example/Cargo.toml | 16 + .../remediated-example/src/lib.rs | 57 +++ .../vulnerable-example/Cargo.toml | 17 + .../vulnerable-example/src/lib.rs | 47 +++ .../remediated-example/Cargo.toml | 16 + .../remediated-example/src/lib.rs | 57 +++ .../vulnerable-example/Cargo.toml | 16 + .../vulnerable-example/src/lib.rs | 47 +++ 18 files changed, 607 insertions(+), 388 deletions(-) create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/remediated-example/Cargo.toml create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/remediated-example/src/lib.rs create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/vulnerable-example/Cargo.toml create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/vulnerable-example/src/lib.rs create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/remediated-example/Cargo.toml create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/remediated-example/src/lib.rs create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/vulnerable-example/Cargo.toml create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/vulnerable-example/src/lib.rs create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/remediated-example/Cargo.toml create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/remediated-example/src/lib.rs create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/vulnerable-example/Cargo.toml create mode 100644 test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/vulnerable-example/src/lib.rs diff --git a/detectors/integer-overflow-or-underflow/src/lib.rs b/detectors/integer-overflow-or-underflow/src/lib.rs index 83a36f07..58a75fd8 100644 --- a/detectors/integer-overflow-or-underflow/src/lib.rs +++ b/detectors/integer-overflow-or-underflow/src/lib.rs @@ -1,239 +1,243 @@ #![feature(rustc_private)] +extern crate rustc_errors; extern crate rustc_hir; extern crate rustc_span; -use clippy_utils::consts::constant_simple; -use clippy_utils::is_integer_literal; -use rustc_hir::{self as hir, Expr, ExprKind, UnOp}; -use rustc_lint::LateContext; -use rustc_lint::LateLintPass; -use rustc_span::Span; +use clippy_utils::{consts::constant_simple, diagnostics::span_lint_and_sugg}; +use rustc_errors::Applicability; +use rustc_hir::{ + intravisit::{walk_expr, FnKind, Visitor}, + BinOpKind, Body, Expr, ExprKind, FnDecl, UnOp, +}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{def_id::LocalDefId, Span}; pub const LINT_MESSAGE: &str = "Potential for integer arithmetic overflow/underflow. Consider checked, wrapping or saturating arithmetic."; -dylint_linting::impl_late_lint! { - /// ### What it does - /// Checks for integer arithmetic operations which could overflow or panic. - /// - /// Specifically, checks for any operators (`+`, `-`, `*`, `<<`, etc) which are capable - /// of overflowing according to the [Rust - /// Reference](https://doc.rust-lang.org/reference/expressions/operator-expr.html#overflow), - /// or which can panic (`/`, `%`). No bounds analysis or sophisticated reasoning is - /// attempted. - /// - /// ### Why is this bad? - /// Integer overflow will trigger a panic in debug builds or will wrap in - /// release mode. Division by zero will cause a panic in either mode. In some applications one - /// wants explicitly checked, wrapping or saturating arithmetic. - /// - /// ### Example - /// ```rust - /// # let a = 0; - /// a + 1; - /// ``` +dylint_linting::declare_late_lint! { pub INTEGER_OVERFLOW_UNDERFLOW, Warn, LINT_MESSAGE, - IntegerOverflowUnderflow::default(), { name: "Integer Overflow/Underflow", - long_message: "An overflow/underflow is typically caught and generates an error. When it is not caught, the operation will result in an inexact result which could lead to serious problems.\n In Ink! 5.0.0, using raw math operations will result in `cargo contract build` failing with an error message.", + long_message: "An overflow/underflow is typically caught and generates an error. When it is not caught, the operation will result in an inexact result which could lead to serious problems.", severity: "Critical", help: "https://coinfabrik.github.io/scout-soroban/docs/vulnerabilities/integer-overflow-or-underflow", vulnerability_class: "Arithmetic", } } - -#[derive(Default)] -pub struct IntegerOverflowUnderflow { - arithmetic_context: ArithmeticContext, +enum Type { + Overflow, + Underflow, } -impl IntegerOverflowUnderflow { - pub fn new() -> Self { - Self { - arithmetic_context: ArithmeticContext::default(), +impl Type { + fn message(&self) -> &'static str { + match self { + Type::Overflow => "overflow", + Type::Underflow => "underflow", } } } -impl<'tcx> LateLintPass<'tcx> for IntegerOverflowUnderflow { - fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { - match e.kind { - ExprKind::Binary(op, lhs, rhs) => { - self.arithmetic_context - .check_binary(cx, e, op.node, lhs, rhs); - } - ExprKind::AssignOp(op, lhs, rhs) => { - self.arithmetic_context - .check_binary(cx, e, op.node, lhs, rhs); - } - ExprKind::Unary(op, arg) => { - if op == UnOp::Neg { - self.arithmetic_context.check_negate(cx, e, arg); - } - } - _ => (), +enum Cause { + Add, + Sub, + Mul, + Pow, + Negate, +} + +impl Cause { + fn message(&self) -> &'static str { + match self { + Cause::Add => "addition", + Cause::Sub => "subtraction", + Cause::Mul => "multiplication", + Cause::Pow => "exponentiation", + Cause::Negate => "negation", } } +} - fn check_expr_post(&mut self, _: &LateContext<'_>, e: &Expr<'_>) { - self.arithmetic_context.expr_post(e.hir_id); +pub struct Finding { + span: Span, + type_: Type, + cause: Cause, + left: String, + right: String, +} + +impl Finding { + fn new(span: Span, type_: Type, cause: Cause, left: String, right: String) -> Self { + Finding { + span, + type_, + cause, + left, + right, + } } - fn check_body(&mut self, cx: &LateContext<'tcx>, body: &rustc_hir::Body<'tcx>) { - self.arithmetic_context.enter_body(cx, body); + fn get_suggestion(&self) -> String { + match self.cause { + Cause::Add => format!("{}.checked_add({})", self.left, self.right), + Cause::Sub => format!("{}.checked_sub({})", self.left, self.right), + Cause::Mul => format!("{}.checked_mul({})", self.left, self.right), + Cause::Pow => format!("{}.checked_pow({})", self.left, self.right), + Cause::Negate => format!("{}.checked_neg()", self.left), + } + .to_string() } - fn check_body_post(&mut self, cx: &LateContext<'tcx>, body: &rustc_hir::Body<'tcx>) { - self.arithmetic_context.body_post(cx, body); + fn generate_message(&self) -> String { + format!( + "This {} operation could {}.", + self.cause.message(), + self.type_.message() + ) } } - -#[derive(Default)] -pub struct ArithmeticContext { - expr_id: Option, - /// This field is used to check whether expressions are constants, such as in enum discriminants - /// and consts - const_span: Option, +pub struct IntegerOverflowUnderflowVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + findings: Vec, } -impl ArithmeticContext { - fn skip_expr(&mut self, e: &hir::Expr<'_>) -> bool { - self.expr_id.is_some() || self.const_span.map_or(false, |span| span.contains(e.span)) - } - pub fn check_binary<'tcx>( +impl IntegerOverflowUnderflowVisitor<'_, '_> { + pub fn check_pow<'tcx>( &mut self, - cx: &LateContext<'tcx>, - expr: &'tcx hir::Expr<'_>, - op: hir::BinOpKind, - l: &'tcx hir::Expr<'_>, - r: &'tcx hir::Expr<'_>, + expr: &'tcx Expr<'_>, + receiver: &'tcx Expr<'tcx>, + exponent: &'tcx Expr<'tcx>, ) { - if self.skip_expr(expr) { - return; - } - match op { - hir::BinOpKind::And - | hir::BinOpKind::Or - | hir::BinOpKind::BitAnd - | hir::BinOpKind::BitOr - | hir::BinOpKind::BitXor - | hir::BinOpKind::Eq - | hir::BinOpKind::Lt - | hir::BinOpKind::Le - | hir::BinOpKind::Ne - | hir::BinOpKind::Ge - | hir::BinOpKind::Gt => return, - _ => (), + let base_ty = self.cx.typeck_results().expr_ty(receiver); + if base_ty.is_integral() { + self.findings.push(Finding::new( + expr.span, + Type::Overflow, + Cause::Pow, + self.get_snippet(receiver), + self.get_snippet(exponent), + )); } + } - let (l_ty, r_ty) = ( - cx.typeck_results().expr_ty(l), - cx.typeck_results().expr_ty(r), - ); - if l_ty.peel_refs().is_integral() && r_ty.peel_refs().is_integral() { - match op { - hir::BinOpKind::Div | hir::BinOpKind::Rem => match &r.kind { - hir::ExprKind::Lit(_lit) => (), - hir::ExprKind::Unary(hir::UnOp::Neg, expr) => { - if is_integer_literal(expr, 1) { - clippy_utils::diagnostics::span_lint_and_help( - cx, - INTEGER_OVERFLOW_UNDERFLOW, - expr.span, - LINT_MESSAGE, - None, - "Potential for integer arithmetic overflow/underflow in unary operation with negative expression. Consider checked, wrapping or saturating arithmetic." - ); - self.expr_id = Some(expr.hir_id); - } - } - _ => { - clippy_utils::diagnostics::span_lint_and_help( - cx, - INTEGER_OVERFLOW_UNDERFLOW, - expr.span, - LINT_MESSAGE, - None, - format!("Potential for integer arithmetic overflow/underflow in operation '{}'. Consider checked, wrapping or saturating arithmetic.", op.as_str()), - ); - self.expr_id = Some(expr.hir_id); - } - }, - _ => { - clippy_utils::diagnostics::span_lint_and_help( - cx, - INTEGER_OVERFLOW_UNDERFLOW, - expr.span, - LINT_MESSAGE, - None, - format!("Potential for integer arithmetic overflow/underflow in operation '{}'. Consider checked, wrapping or saturating arithmetic.", op.as_str()), - ); - self.expr_id = Some(expr.hir_id); - } + pub fn check_negate<'tcx>(&mut self, expr: &'tcx Expr<'_>, arg: &'tcx Expr<'_>) { + let ty = self.cx.typeck_results().expr_ty(arg); + if ty.is_integral() && ty.is_signed() { + // We only care about non-constant values + if constant_simple(self.cx, self.cx.typeck_results(), arg).is_none() { + self.findings.push(Finding::new( + expr.span, + Type::Overflow, + Cause::Negate, + self.get_snippet(arg), + "".to_string(), + )); } } } - pub fn check_negate<'tcx>( + pub fn check_binary<'tcx>( &mut self, - cx: &LateContext<'tcx>, - expr: &'tcx hir::Expr<'_>, - arg: &'tcx hir::Expr<'_>, + expr: &'tcx Expr<'_>, + op: BinOpKind, + l: &'tcx Expr<'_>, + r: &'tcx Expr<'_>, ) { - if self.skip_expr(expr) { + // Check if either operand is non-constant + let l_const = constant_simple(self.cx, self.cx.typeck_results(), l); + let r_const = constant_simple(self.cx, self.cx.typeck_results(), r); + if l_const.is_some() && r_const.is_some() { return; } - let ty = cx.typeck_results().expr_ty(arg); - if constant_simple(cx, cx.typeck_results(), expr).is_none() && ty.is_integral() { - clippy_utils::diagnostics::span_lint_and_help( - cx, - INTEGER_OVERFLOW_UNDERFLOW, - expr.span, - LINT_MESSAGE, - None, - "Potential for integer arithmetic overflow/underflow. Consider checked, wrapping or saturating arithmetic.", - ); - self.expr_id = Some(expr.hir_id); - } - } - pub fn expr_post(&mut self, id: hir::HirId) { - if Some(id) == self.expr_id { - self.expr_id = None; + // Check if any of the operands is not an integer + let (l_ty, r_ty) = ( + self.cx.typeck_results().expr_ty(l), + self.cx.typeck_results().expr_ty(r), + ); + if !l_ty.peel_refs().is_integral() || !r_ty.peel_refs().is_integral() { + return; } - } - pub fn enter_body(&mut self, cx: &LateContext<'_>, body: &hir::Body<'_>) { - let body_owner = cx.tcx.hir().body_owner(body.id()); - let body_owner_def_id = cx.tcx.hir().body_owner_def_id(body.id()); + // Check if the operation is an addition, subtraction or multiplication + let (finding_type, cause) = match op { + BinOpKind::Add => (Type::Overflow, Cause::Add), + BinOpKind::Sub => (Type::Underflow, Cause::Sub), + BinOpKind::Mul => (Type::Overflow, Cause::Mul), + _ => return, // We're not interested in other operations + }; + + self.findings.push(Finding::new( + expr.span, + finding_type, + cause, + self.get_snippet(l), + self.get_snippet(r), + )); + } - match cx.tcx.hir().body_owner_kind(body_owner_def_id) { - hir::BodyOwnerKind::Static(_) | hir::BodyOwnerKind::Const { .. } => { - let body_span = cx.tcx.hir().span_with_body(body_owner); + fn get_snippet(&self, expr: &Expr<'_>) -> String { + self.cx + .sess() + .source_map() + .span_to_snippet(expr.span) + .unwrap() + } +} - if let Some(span) = self.const_span { - if span.contains(body_span) { - return; - } +impl<'a, 'tcx> Visitor<'tcx> for IntegerOverflowUnderflowVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + match expr.kind { + ExprKind::Binary(op, lhs, rhs) | ExprKind::AssignOp(op, lhs, rhs) => { + self.check_binary(expr, op.node, lhs, rhs); + } + ExprKind::Unary(UnOp::Neg, arg) => { + self.check_negate(expr, arg); + } + ExprKind::MethodCall(method_name, receiver, args, ..) => { + if method_name.ident.as_str() == "pow" { + self.check_pow(expr, receiver, &args[0]); } - self.const_span = Some(body_span); } - hir::BodyOwnerKind::Fn | hir::BodyOwnerKind::Closure => (), + _ => (), } + walk_expr(self, expr); } +} + +impl<'tcx> LateLintPass<'tcx> for IntegerOverflowUnderflow { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: FnKind<'tcx>, + _: &'tcx FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + span: Span, + _: LocalDefId, + ) { + if span.from_expansion() { + return; + } - pub fn body_post(&mut self, cx: &LateContext<'_>, body: &hir::Body<'_>) { - let body_owner = cx.tcx.hir().body_owner(body.id()); - let body_span = cx.tcx.hir().span_with_body(body_owner); + let mut visitor = IntegerOverflowUnderflowVisitor { + cx, + findings: Vec::new(), + }; + visitor.visit_body(body); - if let Some(span) = self.const_span { - if span.contains(body_span) { - return; - } + for finding in visitor.findings { + span_lint_and_sugg( + cx, + INTEGER_OVERFLOW_UNDERFLOW, + finding.span, + finding.generate_message(), + "Consider using the checked version of this operation", + finding.get_suggestion(), + Applicability::MaybeIncorrect, + ); } - self.const_span = None; } } diff --git a/test-cases/integer-overflow-or-underflow/Cargo.toml b/test-cases/integer-overflow-or-underflow/Cargo.toml index ca2a186c..5fa447bf 100644 --- a/test-cases/integer-overflow-or-underflow/Cargo.toml +++ b/test-cases/integer-overflow-or-underflow/Cargo.toml @@ -19,3 +19,6 @@ strip = "symbols" [profile.release-with-logs] debug-assertions = true inherits = "release" + +[workspace.metadata.dylint] +libraries = [{ path = "/Users/josegarcia/Desktop/coinfabrik/scout-soroban/detectors/integer-overflow-or-underflow" }] diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/src/lib.rs index ad0fd83f..a5d17cdf 100644 --- a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/src/lib.rs +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/remediated-example/src/lib.rs @@ -9,7 +9,6 @@ pub struct IntegerOverflowUnderflow; #[repr(u32)] pub enum Error { OverflowError = 1, - UnderflowError = 2, } #[contractimpl] @@ -30,16 +29,6 @@ impl IntegerOverflowUnderflow { Ok(()) } - pub fn sub(env: Env, value: u32) -> Result<(), Error> { - let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); - let new_value = match current.checked_sub(value) { - Some(value) => value, - None => return Err(Error::UnderflowError), - }; - env.storage().temporary().set(&Self::VALUE, &new_value); - Ok(()) - } - pub fn get(env: Env) -> u32 { env.storage().temporary().get(&Self::VALUE).unwrap_or(0) } @@ -50,20 +39,6 @@ mod test { use super::*; use soroban_sdk::Env; - #[test] - fn test_initialize() { - // Given - let env = Env::default(); - let contract_id = env.register_contract(None, IntegerOverflowUnderflow); - let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); - - // When - client.initialize(&42); - - // Then - assert_eq!(client.get(), 42); - } - #[test] fn test_add_overflow() { // Given @@ -78,19 +53,4 @@ mod test { // Then assert_eq!(result, Err(Ok(Error::OverflowError))); } - - #[test] - fn test_sub_underflow() { - // Given - let env = Env::default(); - let contract_id = env.register_contract(None, IntegerOverflowUnderflow); - let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); - - // When - client.initialize(&0); - let result = client.try_sub(&1); - - // Then - assert_eq!(result, Err(Ok(Error::UnderflowError))); - } } diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/src/lib.rs index 00f757e3..41b6fe4d 100644 --- a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/src/lib.rs +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-1/vulnerable-example/src/lib.rs @@ -18,12 +18,6 @@ impl IntegerOverflowUnderflow { env.storage().temporary().set(&Self::VALUE, &new_value); } - pub fn sub(env: Env, value: u32) { - let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); - let new_value = current - value; - env.storage().temporary().set(&Self::VALUE, &new_value); - } - pub fn get(env: Env) -> u32 { env.storage().temporary().get(&Self::VALUE).unwrap_or(0) } @@ -34,20 +28,6 @@ mod test { use super::*; use soroban_sdk::Env; - #[test] - fn test_initialize() { - // Given - let env = Env::default(); - let contract_id = env.register_contract(None, IntegerOverflowUnderflow); - let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); - - // When - client.initialize(&42); - - // Then - assert_eq!(client.get(), 42); - } - #[test] #[should_panic(expected = "attempt to add with overflow")] fn test_add_overflow() { @@ -63,20 +43,4 @@ mod test { // Then // Panic } - - #[test] - #[should_panic(expected = "attempt to subtract with overflow")] - fn test_sub_underflow() { - // Given - let env = Env::default(); - let contract_id = env.register_contract(None, IntegerOverflowUnderflow); - let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); - - // When - client.initialize(&0); - client.sub(&1); - - // Then - // Panic - } } diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/remediated-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/remediated-example/src/lib.rs index e0347b80..df001569 100644 --- a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/remediated-example/src/lib.rs +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/remediated-example/src/lib.rs @@ -8,8 +8,7 @@ pub struct IntegerOverflowUnderflow; #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] #[repr(u32)] pub enum Error { - OverflowError = 1, - UnderflowError = 2, + UnderflowError = 1, } #[contractimpl] @@ -20,29 +19,9 @@ impl IntegerOverflowUnderflow { env.storage().temporary().set(&Self::VALUE, &value); } - pub fn mul(env: Env, value: u32) -> Result<(), Error> { + pub fn sub(env: Env, value: u32) -> Result<(), Error> { let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); - let new_value = match current.checked_mul(value) { - Some(value) => value, - None => return Err(Error::OverflowError), - }; - env.storage().temporary().set(&Self::VALUE, &new_value); - Ok(()) - } - - pub fn pow(env: Env, value: u32) -> Result<(), Error> { - let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); - let new_value = match current.checked_pow(value) { - Some(value) => value, - None => return Err(Error::OverflowError), - }; - env.storage().temporary().set(&Self::VALUE, &new_value); - Ok(()) - } - - pub fn neg(env: Env) -> Result<(), Error> { - let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); - let new_value = match current.checked_neg() { + let new_value = match current.checked_sub(value) { Some(value) => value, None => return Err(Error::UnderflowError), }; @@ -61,59 +40,15 @@ mod test { use soroban_sdk::Env; #[test] - fn test_initialize() { - // Given - let env = Env::default(); - let contract_id = env.register_contract(None, IntegerOverflowUnderflow); - let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); - - // When - client.initialize(&42); - - // Then - assert_eq!(client.get(), 42); - } - - #[test] - fn test_mul_overflow() { - // Given - let env = Env::default(); - let contract_id = env.register_contract(None, IntegerOverflowUnderflow); - let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); - - // When - client.initialize(&u32::MAX); - let result = client.try_mul(&2); - - // Then - assert_eq!(result, Err(Ok(Error::OverflowError))); - } - - #[test] - fn test_pow_overflow() { - // Given - let env = Env::default(); - let contract_id = env.register_contract(None, IntegerOverflowUnderflow); - let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); - - // When - client.initialize(&2); - let result = client.try_pow(&u32::MAX); - - // Then - assert_eq!(result, Err(Ok(Error::OverflowError))); - } - - #[test] - fn test_neg() { + fn test_sub_underflow() { // Given let env = Env::default(); let contract_id = env.register_contract(None, IntegerOverflowUnderflow); let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); // When - client.initialize(&u32::MAX); - let result = client.try_neg(); + client.initialize(&0); + let result = client.try_sub(&1); // Then assert_eq!(result, Err(Ok(Error::UnderflowError))); diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/vulnerable-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/vulnerable-example/src/lib.rs index 20e7c11a..83a57ea9 100644 --- a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/vulnerable-example/src/lib.rs +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-2/vulnerable-example/src/lib.rs @@ -12,21 +12,9 @@ impl IntegerOverflowUnderflow { env.storage().temporary().set(&Self::VALUE, &value); } - pub fn mul(env: Env, value: u32) { + pub fn sub(env: Env, value: u32) { let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); - let new_value = current * value; - env.storage().temporary().set(&Self::VALUE, &new_value); - } - - pub fn pow(env: Env, value: u32) { - let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); - let new_value = current.pow(value); - env.storage().temporary().set(&Self::VALUE, &new_value); - } - - pub fn neg(env: Env) { - let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); - let new_value = current.wrapping_neg(); + let new_value = current - value; env.storage().temporary().set(&Self::VALUE, &new_value); } @@ -41,63 +29,18 @@ mod test { use soroban_sdk::Env; #[test] - fn test_initialize() { - // Given - let env = Env::default(); - let contract_id = env.register_contract(None, IntegerOverflowUnderflow); - let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); - - // When - client.initialize(&42); - - // Then - assert_eq!(client.get(), 42); - } - - #[test] - #[should_panic(expected = "attempt to multiply with overflow")] - fn test_mul_overflow() { - // Given - let env = Env::default(); - let contract_id = env.register_contract(None, IntegerOverflowUnderflow); - let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); - - // When - client.initialize(&u32::MAX); - client.mul(&2); - - // Then - // Panic - } - - #[test] - #[should_panic(expected = "attempt to multiply with overflow")] - fn test_pow_overflow() { + #[should_panic(expected = "attempt to subtract with overflow")] + fn test_sub_underflow() { // Given let env = Env::default(); let contract_id = env.register_contract(None, IntegerOverflowUnderflow); let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); // When - client.initialize(&2); - client.pow(&u32::MAX); + client.initialize(&0); + client.sub(&1); // Then // Panic } - - #[test] - fn test_neg() { - // Given - let env = Env::default(); - let contract_id = env.register_contract(None, IntegerOverflowUnderflow); - let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); - - // When - client.initialize(&u32::MIN); - client.neg(); - - // Then - assert_eq!(client.get(), u32::MIN.wrapping_neg()); - } } diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/remediated-example/Cargo.toml b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/remediated-example/Cargo.toml new file mode 100644 index 00000000..0f8c6824 --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/remediated-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "integer-overflow-or-underflow-remediated-3" +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"] diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/remediated-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/remediated-example/src/lib.rs new file mode 100644 index 00000000..c51cee28 --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/remediated-example/src/lib.rs @@ -0,0 +1,57 @@ +#![no_std] +use soroban_sdk::{contract, contracterror, contractimpl, symbol_short, Env, Symbol}; + +#[contract] +pub struct IntegerOverflowUnderflow; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + OverflowError = 1, + UnderflowError = 2, +} + +#[contractimpl] +impl IntegerOverflowUnderflow { + const VALUE: Symbol = symbol_short!("VALUE"); + + pub fn initialize(env: Env, value: u32) { + env.storage().temporary().set(&Self::VALUE, &value); + } + + pub fn mul(env: Env, value: u32) -> Result<(), Error> { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = match current.checked_mul(value) { + Some(value) => value, + None => return Err(Error::OverflowError), + }; + env.storage().temporary().set(&Self::VALUE, &new_value); + Ok(()) + } + + pub fn get(env: Env) -> u32 { + env.storage().temporary().get(&Self::VALUE).unwrap_or(0) + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::Env; + + #[test] + fn test_mul_overflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&u32::MAX); + let result = client.try_mul(&2); + + // Then + assert_eq!(result, Err(Ok(Error::OverflowError))); + } +} diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/vulnerable-example/Cargo.toml b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..cbd5cf7d --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/vulnerable-example/Cargo.toml @@ -0,0 +1,17 @@ +[package] +edition = "2021" +name = "integer-overflow-or-underflow-vulnerable-3" +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"] + diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/vulnerable-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..7564b34b --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-3/vulnerable-example/src/lib.rs @@ -0,0 +1,47 @@ +#![no_std] + +use soroban_sdk::{contract, contractimpl, symbol_short, Env, Symbol}; + +#[contract] +pub struct IntegerOverflowUnderflow; + +#[contractimpl] +impl IntegerOverflowUnderflow { + const VALUE: Symbol = symbol_short!("VALUE"); + + pub fn initialize(env: Env, value: u32) { + env.storage().temporary().set(&Self::VALUE, &value); + } + + pub fn mul(env: Env, value: u32) { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = current * value; + env.storage().temporary().set(&Self::VALUE, &new_value); + } + + pub fn get(env: Env) -> u32 { + env.storage().temporary().get(&Self::VALUE).unwrap_or(0) + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::Env; + + #[test] + #[should_panic(expected = "attempt to multiply with overflow")] + fn test_mul_overflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&u32::MAX); + client.mul(&2); + + // Then + // Panic + } +} diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/remediated-example/Cargo.toml b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/remediated-example/Cargo.toml new file mode 100644 index 00000000..4c1bd994 --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/remediated-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "integer-overflow-or-underflow-remediated-4" +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"] diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/remediated-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/remediated-example/src/lib.rs new file mode 100644 index 00000000..53272651 --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/remediated-example/src/lib.rs @@ -0,0 +1,57 @@ +#![no_std] +use soroban_sdk::{contract, contracterror, contractimpl, symbol_short, Env, Symbol}; + +#[contract] +pub struct IntegerOverflowUnderflow; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + OverflowError = 1, + UnderflowError = 2, +} + +#[contractimpl] +impl IntegerOverflowUnderflow { + const VALUE: Symbol = symbol_short!("VALUE"); + + pub fn initialize(env: Env, value: u32) { + env.storage().temporary().set(&Self::VALUE, &value); + } + + pub fn pow(env: Env, value: u32) -> Result<(), Error> { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = match current.checked_pow(value) { + Some(value) => value, + None => return Err(Error::OverflowError), + }; + env.storage().temporary().set(&Self::VALUE, &new_value); + Ok(()) + } + + pub fn get(env: Env) -> u32 { + env.storage().temporary().get(&Self::VALUE).unwrap_or(0) + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::Env; + + #[test] + fn test_pow_overflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&2); + let result = client.try_pow(&u32::MAX); + + // Then + assert_eq!(result, Err(Ok(Error::OverflowError))); + } +} diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/vulnerable-example/Cargo.toml b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..2dc0ba18 --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/vulnerable-example/Cargo.toml @@ -0,0 +1,17 @@ +[package] +edition = "2021" +name = "integer-overflow-or-underflow-vulnerable-4" +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"] + diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/vulnerable-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..34619e1a --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-4/vulnerable-example/src/lib.rs @@ -0,0 +1,47 @@ +#![no_std] + +use soroban_sdk::{contract, contractimpl, symbol_short, Env, Symbol}; + +#[contract] +pub struct IntegerOverflowUnderflow; + +#[contractimpl] +impl IntegerOverflowUnderflow { + const VALUE: Symbol = symbol_short!("VALUE"); + + pub fn initialize(env: Env, value: u32) { + env.storage().temporary().set(&Self::VALUE, &value); + } + + pub fn pow(env: Env, value: u32) { + let current: u32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = current.pow(value); + env.storage().temporary().set(&Self::VALUE, &new_value); + } + + pub fn get(env: Env) -> u32 { + env.storage().temporary().get(&Self::VALUE).unwrap_or(0) + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::Env; + + #[test] + #[should_panic(expected = "attempt to multiply with overflow")] + fn test_pow_overflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&2); + client.pow(&u32::MAX); + + // Then + // Panic + } +} diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/remediated-example/Cargo.toml b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/remediated-example/Cargo.toml new file mode 100644 index 00000000..d2fc3df9 --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/remediated-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "integer-overflow-or-underflow-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"] diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/remediated-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/remediated-example/src/lib.rs new file mode 100644 index 00000000..691cf47b --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/remediated-example/src/lib.rs @@ -0,0 +1,57 @@ +#![no_std] +use soroban_sdk::{contract, contracterror, contractimpl, symbol_short, Env, Symbol}; + +#[contract] +pub struct IntegerOverflowUnderflow; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + OverflowError = 1, + UnderflowError = 2, +} + +#[contractimpl] +impl IntegerOverflowUnderflow { + const VALUE: Symbol = symbol_short!("VALUE"); + + pub fn initialize(env: Env, value: i32) { + env.storage().temporary().set(&Self::VALUE, &value); + } + + pub fn neg(env: Env) -> Result<(), Error> { + let current: i32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = match current.checked_neg() { + Some(value) => value, + None => return Err(Error::UnderflowError), + }; + env.storage().temporary().set(&Self::VALUE, &new_value); + Ok(()) + } + + pub fn get(env: Env) -> i32 { + env.storage().temporary().get(&Self::VALUE).unwrap_or(0) + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::Env; + + #[test] + fn test_neg() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&i32::MIN); + let result = client.try_neg(); + + // Then + assert_eq!(result, Err(Ok(Error::UnderflowError))); + } +} diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/vulnerable-example/Cargo.toml b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..75390e80 --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/vulnerable-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "integer-overflow-or-underflow-vulnerable-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"] diff --git a/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/vulnerable-example/src/lib.rs b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..b9d1e04c --- /dev/null +++ b/test-cases/integer-overflow-or-underflow/integer-overflow-or-underflow-5/vulnerable-example/src/lib.rs @@ -0,0 +1,47 @@ +#![no_std] + +use soroban_sdk::{contract, contractimpl, symbol_short, Env, Symbol}; + +#[contract] +pub struct IntegerOverflowUnderflow; + +#[contractimpl] +impl IntegerOverflowUnderflow { + const VALUE: Symbol = symbol_short!("VALUE"); + + pub fn initialize(env: Env, value: i32) { + env.storage().temporary().set(&Self::VALUE, &value); + } + + pub fn neg(env: Env) { + let current: i32 = env.storage().temporary().get(&Self::VALUE).unwrap_or(0); + let new_value = -current; + env.storage().temporary().set(&Self::VALUE, &new_value); + } + + pub fn get(env: Env) -> i32 { + env.storage().temporary().get(&Self::VALUE).unwrap_or(0) + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::Env; + + #[test] + #[should_panic(expected = "attempt to negate with overflow")] + fn test_neg() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, IntegerOverflowUnderflow); + let client = IntegerOverflowUnderflowClient::new(&env, &contract_id); + + // When + client.initialize(&i32::MIN); + client.neg(); + + // Then + // Panic + } +} From f0e20a6c5a1b72298c9e3119353d823632849f16 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 25 Jul 2024 15:51:53 -0300 Subject: [PATCH 4/8] Fix CI --- detectors/divide-before-multiply/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/detectors/divide-before-multiply/src/lib.rs b/detectors/divide-before-multiply/src/lib.rs index 00d293a7..70a8f14f 100644 --- a/detectors/divide-before-multiply/src/lib.rs +++ b/detectors/divide-before-multiply/src/lib.rs @@ -177,8 +177,9 @@ fn navigate_trough_basicblocks<'tcx>( if BinOp::Div == *op { tainted_places.push(assign.0); } else if BinOp::Mul == *op - && (check_operand(&operands.0, tainted_places, &assign.0) - || check_operand(&operands.1, tainted_places, &assign.0)) + || BinOp::MulWithOverflow == *op + && (check_operand(&operands.0, tainted_places, &assign.0) + || check_operand(&operands.1, tainted_places, &assign.0)) { spans.push(statement.source_info.span); }; From 351bd9dd671b03aef837de090a3bab68ecb816a6 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Sat, 27 Jul 2024 19:11:15 -0300 Subject: [PATCH 5/8] Improve detector with compile time known values --- .../integer-overflow-or-underflow/Cargo.toml | 1 + .../integer-overflow-or-underflow/src/lib.rs | 187 ++++++++-------- .../integer-overflow-or-underflow/Cargo.toml | 3 - utils/Cargo.lock | 207 ++++++++++++++++++ utils/Cargo.toml | 3 +- utils/src/constant_analyzer/mod.rs | 88 ++++++++ utils/src/lib.rs | 3 + 7 files changed, 388 insertions(+), 104 deletions(-) create mode 100644 utils/src/constant_analyzer/mod.rs diff --git a/detectors/integer-overflow-or-underflow/Cargo.toml b/detectors/integer-overflow-or-underflow/Cargo.toml index 7c58dfdc..15f1a7fd 100644 --- a/detectors/integer-overflow-or-underflow/Cargo.toml +++ b/detectors/integer-overflow-or-underflow/Cargo.toml @@ -10,6 +10,7 @@ crate-type = ["cdylib"] clippy_utils = { workspace = true } dylint_linting = { workspace = true } if_chain = { workspace = true } +utils = { workspace = true } [package.metadata.rust-analyzer] rustc_private = true diff --git a/detectors/integer-overflow-or-underflow/src/lib.rs b/detectors/integer-overflow-or-underflow/src/lib.rs index 58a75fd8..325f7889 100644 --- a/detectors/integer-overflow-or-underflow/src/lib.rs +++ b/detectors/integer-overflow-or-underflow/src/lib.rs @@ -1,17 +1,17 @@ #![feature(rustc_private)] -extern crate rustc_errors; extern crate rustc_hir; extern crate rustc_span; -use clippy_utils::{consts::constant_simple, diagnostics::span_lint_and_sugg}; -use rustc_errors::Applicability; +use clippy_utils::{consts::constant, diagnostics::span_lint_and_help}; use rustc_hir::{ intravisit::{walk_expr, FnKind, Visitor}, BinOpKind, Body, Expr, ExprKind, FnDecl, UnOp, }; -use rustc_lint::{LateContext, LateLintPass, LintContext}; -use rustc_span::{def_id::LocalDefId, Span}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_span::{def_id::LocalDefId, Span, Symbol}; +use std::collections::HashSet; +use utils::ConstantAnalyzer; pub const LINT_MESSAGE: &str = "Potential for integer arithmetic overflow/underflow. Consider checked, wrapping or saturating arithmetic."; @@ -30,6 +30,7 @@ dylint_linting::declare_late_lint! { enum Type { Overflow, Underflow, + OverflowUnderflow, } impl Type { @@ -37,6 +38,7 @@ impl Type { match self { Type::Overflow => "overflow", Type::Underflow => "underflow", + Type::OverflowUnderflow => "overflow or underflow", } } } @@ -47,16 +49,18 @@ enum Cause { Mul, Pow, Negate, + Multiple, } impl Cause { fn message(&self) -> &'static str { match self { - Cause::Add => "addition", - Cause::Sub => "subtraction", - Cause::Mul => "multiplication", - Cause::Pow => "exponentiation", - Cause::Negate => "negation", + Cause::Add => "addition operation", + Cause::Sub => "subtraction operation", + Cause::Mul => "multiplication operation", + Cause::Pow => "exponentiation operation", + Cause::Negate => "negation operation", + Cause::Multiple => "operation", } } } @@ -65,35 +69,16 @@ pub struct Finding { span: Span, type_: Type, cause: Cause, - left: String, - right: String, } impl Finding { - fn new(span: Span, type_: Type, cause: Cause, left: String, right: String) -> Self { - Finding { - span, - type_, - cause, - left, - right, - } - } - - fn get_suggestion(&self) -> String { - match self.cause { - Cause::Add => format!("{}.checked_add({})", self.left, self.right), - Cause::Sub => format!("{}.checked_sub({})", self.left, self.right), - Cause::Mul => format!("{}.checked_mul({})", self.left, self.right), - Cause::Pow => format!("{}.checked_pow({})", self.left, self.right), - Cause::Negate => format!("{}.checked_neg()", self.left), - } - .to_string() + fn new(span: Span, type_: Type, cause: Cause) -> Self { + Finding { span, type_, cause } } fn generate_message(&self) -> String { format!( - "This {} operation could {}.", + "This {} could {}.", self.cause.message(), self.type_.message() ) @@ -102,89 +87,74 @@ impl Finding { pub struct IntegerOverflowUnderflowVisitor<'a, 'tcx> { cx: &'a LateContext<'tcx>, findings: Vec, + is_complex_operation: bool, + constant_detector: ConstantAnalyzer<'a, 'tcx>, } -impl IntegerOverflowUnderflowVisitor<'_, '_> { - pub fn check_pow<'tcx>( - &mut self, - expr: &'tcx Expr<'_>, - receiver: &'tcx Expr<'tcx>, - exponent: &'tcx Expr<'tcx>, - ) { - let base_ty = self.cx.typeck_results().expr_ty(receiver); - if base_ty.is_integral() { - self.findings.push(Finding::new( - expr.span, - Type::Overflow, - Cause::Pow, - self.get_snippet(receiver), - self.get_snippet(exponent), - )); +impl<'tcx> IntegerOverflowUnderflowVisitor<'_, 'tcx> { + pub fn check_pow(&mut self, expr: &Expr<'tcx>, base: &Expr<'tcx>, exponent: &Expr<'tcx>) { + let is_base_known = self.constant_detector.is_compile_time_known(base); + let is_exponent_known = self.constant_detector.is_compile_time_known(exponent); + if is_base_known && is_exponent_known { + return; + } + + let base_type = self.cx.typeck_results().expr_ty(base); + if base_type.is_integral() { + self.findings + .push(Finding::new(expr.span, Type::Overflow, Cause::Pow)); } } - pub fn check_negate<'tcx>(&mut self, expr: &'tcx Expr<'_>, arg: &'tcx Expr<'_>) { - let ty = self.cx.typeck_results().expr_ty(arg); - if ty.is_integral() && ty.is_signed() { - // We only care about non-constant values - if constant_simple(self.cx, self.cx.typeck_results(), arg).is_none() { - self.findings.push(Finding::new( - expr.span, - Type::Overflow, - Cause::Negate, - self.get_snippet(arg), - "".to_string(), - )); + pub fn check_negate(&mut self, expr: &Expr<'tcx>, operand: &Expr<'tcx>) { + let is_operand_known = self.constant_detector.is_compile_time_known(operand); + if is_operand_known { + return; + } + + let operand_type = self.cx.typeck_results().expr_ty(operand); + if operand_type.is_integral() && operand_type.is_signed() { + if constant(self.cx, self.cx.typeck_results(), operand).is_none() { + self.findings + .push(Finding::new(expr.span, Type::Overflow, Cause::Negate)); } } } - pub fn check_binary<'tcx>( + pub fn check_binary( &mut self, - expr: &'tcx Expr<'_>, + expr: &Expr<'tcx>, op: BinOpKind, - l: &'tcx Expr<'_>, - r: &'tcx Expr<'_>, + left: &Expr<'tcx>, + right: &Expr<'tcx>, ) { - // Check if either operand is non-constant - let l_const = constant_simple(self.cx, self.cx.typeck_results(), l); - let r_const = constant_simple(self.cx, self.cx.typeck_results(), r); - if l_const.is_some() && r_const.is_some() { + let is_left_known = self.constant_detector.is_compile_time_known(left); + let is_right_known = self.constant_detector.is_compile_time_known(right); + if is_left_known && is_right_known { return; } - // Check if any of the operands is not an integer - let (l_ty, r_ty) = ( - self.cx.typeck_results().expr_ty(l), - self.cx.typeck_results().expr_ty(r), + let (left_type, right_type) = ( + self.cx.typeck_results().expr_ty(left), + self.cx.typeck_results().expr_ty(right), ); - if !l_ty.peel_refs().is_integral() || !r_ty.peel_refs().is_integral() { + if !left_type.peel_refs().is_integral() || !right_type.peel_refs().is_integral() { return; } - // Check if the operation is an addition, subtraction or multiplication - let (finding_type, cause) = match op { - BinOpKind::Add => (Type::Overflow, Cause::Add), - BinOpKind::Sub => (Type::Underflow, Cause::Sub), - BinOpKind::Mul => (Type::Overflow, Cause::Mul), - _ => return, // We're not interested in other operations + let (finding_type, cause) = if self.is_complex_operation { + (Type::OverflowUnderflow, Cause::Multiple) + } else { + match op { + BinOpKind::Add => (Type::Overflow, Cause::Add), + BinOpKind::Sub => (Type::Underflow, Cause::Sub), + BinOpKind::Mul => (Type::Overflow, Cause::Mul), + _ => return, + } }; - self.findings.push(Finding::new( - expr.span, - finding_type, - cause, - self.get_snippet(l), - self.get_snippet(r), - )); - } - - fn get_snippet(&self, expr: &Expr<'_>) -> String { - self.cx - .sess() - .source_map() - .span_to_snippet(expr.span) - .unwrap() + self.findings + .push(Finding::new(expr.span, finding_type, cause)); } } @@ -192,18 +162,24 @@ impl<'a, 'tcx> Visitor<'tcx> for IntegerOverflowUnderflowVisitor<'a, 'tcx> { fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { match expr.kind { ExprKind::Binary(op, lhs, rhs) | ExprKind::AssignOp(op, lhs, rhs) => { + self.is_complex_operation = matches!(lhs.kind, ExprKind::Binary(..)) + || matches!(rhs.kind, ExprKind::Binary(..)); self.check_binary(expr, op.node, lhs, rhs); + if self.is_complex_operation { + return; + } } ExprKind::Unary(UnOp::Neg, arg) => { self.check_negate(expr, arg); } ExprKind::MethodCall(method_name, receiver, args, ..) => { - if method_name.ident.as_str() == "pow" { + if method_name.ident.name == Symbol::intern("pow") { self.check_pow(expr, receiver, &args[0]); } } _ => (), } + walk_expr(self, expr); } } @@ -218,26 +194,37 @@ impl<'tcx> LateLintPass<'tcx> for IntegerOverflowUnderflow { span: Span, _: LocalDefId, ) { + // If the function comes from a macro expansion, we ignore it if span.from_expansion() { return; } + // Gather all compile-time variables in the function + let mut constant_detector = ConstantAnalyzer { + cx, + constants: HashSet::new(), + }; + constant_detector.visit_body(body); + + // Analyze the function for integer overflow/underflow let mut visitor = IntegerOverflowUnderflowVisitor { cx, findings: Vec::new(), + is_complex_operation: false, + constant_detector, }; visitor.visit_body(body); + // Report any findings for finding in visitor.findings { - span_lint_and_sugg( + span_lint_and_help( cx, INTEGER_OVERFLOW_UNDERFLOW, finding.span, finding.generate_message(), - "Consider using the checked version of this operation", - finding.get_suggestion(), - Applicability::MaybeIncorrect, - ); + None, + "Consider using the checked version of this operation/s", + ) } } } diff --git a/test-cases/integer-overflow-or-underflow/Cargo.toml b/test-cases/integer-overflow-or-underflow/Cargo.toml index 5fa447bf..ca2a186c 100644 --- a/test-cases/integer-overflow-or-underflow/Cargo.toml +++ b/test-cases/integer-overflow-or-underflow/Cargo.toml @@ -19,6 +19,3 @@ strip = "symbols" [profile.release-with-logs] debug-assertions = true inherits = "release" - -[workspace.metadata.dylint] -libraries = [{ path = "/Users/josegarcia/Desktop/coinfabrik/scout-soroban/detectors/integer-overflow-or-underflow" }] diff --git a/utils/Cargo.lock b/utils/Cargo.lock index 7cda8d78..7004de71 100644 --- a/utils/Cargo.lock +++ b/utils/Cargo.lock @@ -2,15 +2,222 @@ # It is not intended for manual editing. version = 3 +[[package]] +name = "arrayvec" +version = "0.7.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "96d30a06541fbafbc7f82ed10c06164cfbd2c401138f6addd8404629c4b16711" + +[[package]] +name = "bitflags" +version = "1.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a" + +[[package]] +name = "clippy_config" +version = "0.1.81" +source = "git+https://github.com/rust-lang/rust-clippy?rev=51a1cf0#51a1cf07876d36a5008f9dc07cd36ba9d412a5b7" +dependencies = [ + "rustc-semver", + "serde", + "toml", +] + +[[package]] +name = "clippy_utils" +version = "0.1.81" +source = "git+https://github.com/rust-lang/rust-clippy?rev=51a1cf0#51a1cf07876d36a5008f9dc07cd36ba9d412a5b7" +dependencies = [ + "arrayvec", + "clippy_config", + "itertools", + "rustc-semver", + "rustc_apfloat", +] + +[[package]] +name = "either" +version = "1.13.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "60b1af1c220855b6ceac025d3f6ecdd2b7c4894bfe9cd9bda4fbb4bc7c0d4cf0" + +[[package]] +name = "equivalent" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5443807d6dff69373d433ab9ef5378ad8df50ca6298caf15de6e52e24aaf54d5" + +[[package]] +name = "hashbrown" +version = "0.14.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e5274423e17b7c9fc20b6e7e208532f9b19825d82dfd615708b70edd83df41f1" + [[package]] name = "if_chain" version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cb56e1aa765b4b4f3aadfab769793b7087bb03a4ea4920644a6d238e2df5b9ed" +[[package]] +name = "indexmap" +version = "2.2.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "168fb715dda47215e360912c096649d23d58bf392ac62f73919e831745e40f26" +dependencies = [ + "equivalent", + "hashbrown", +] + +[[package]] +name = "itertools" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba291022dbbd398a455acf126c1e341954079855bc60dfdda641363bd6922569" +dependencies = [ + "either", +] + +[[package]] +name = "memchr" +version = "2.7.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "78ca9ab1a0babb1e7d5695e3530886289c18cf2f87ec19a575a0abdce112e3a3" + +[[package]] +name = "proc-macro2" +version = "1.0.86" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5e719e8df665df0d1c8fbfd238015744736151d4445ec0836b8e628aae103b77" +dependencies = [ + "unicode-ident", +] + +[[package]] +name = "quote" +version = "1.0.36" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0fa76aaf39101c457836aec0ce2316dbdc3ab723cdda1c6bd4e6ad4208acaca7" +dependencies = [ + "proc-macro2", +] + +[[package]] +name = "rustc-semver" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5be1bdc7edf596692617627bbfeaba522131b18e06ca4df2b6b689e3c5d5ce84" + +[[package]] +name = "rustc_apfloat" +version = "0.2.1+llvm-462a31f5a5ab" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "886d94c63c812a8037c4faca2607453a0fa4cf82f734665266876b022244543f" +dependencies = [ + "bitflags", + "smallvec", +] + +[[package]] +name = "serde" +version = "1.0.204" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bc76f558e0cbb2a839d37354c575f1dc3fdc6546b5be373ba43d95f231bf7c12" +dependencies = [ + "serde_derive", +] + +[[package]] +name = "serde_derive" +version = "1.0.204" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e0cd7e117be63d3c3678776753929474f3b04a43a080c744d6b0ae2a8c28e222" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "serde_spanned" +version = "0.6.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eb5b1b31579f3811bf615c144393417496f152e12ac8b7663bf664f4a815306d" +dependencies = [ + "serde", +] + +[[package]] +name = "smallvec" +version = "1.13.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3c5e1a9a646d36c3599cd173a41282daf47c44583ad367b8e6837255952e5c67" + +[[package]] +name = "syn" +version = "2.0.72" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dc4b9b9bf2add8093d3f2c0204471e951b2285580335de42f9d2534f3ae7a8af" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + +[[package]] +name = "toml" +version = "0.7.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dd79e69d3b627db300ff956027cc6c3798cef26d22526befdfcd12feeb6d2257" +dependencies = [ + "serde", + "serde_spanned", + "toml_datetime", + "toml_edit", +] + +[[package]] +name = "toml_datetime" +version = "0.6.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8fb9f64314842840f1d940ac544da178732128f1c78c21772e876579e0da1db" +dependencies = [ + "serde", +] + +[[package]] +name = "toml_edit" +version = "0.19.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1b5bb770da30e5cbfde35a2d7b9b8a2c4b8ef89548a7a6aeab5c9a576e3e7421" +dependencies = [ + "indexmap", + "serde", + "serde_spanned", + "toml_datetime", + "winnow", +] + +[[package]] +name = "unicode-ident" +version = "1.0.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3354b9ac3fae1ff6755cb6db53683adb661634f67557942dea4facebec0fee4b" + [[package]] name = "utils" version = "0.1.0" dependencies = [ + "clippy_utils", "if_chain", ] + +[[package]] +name = "winnow" +version = "0.5.40" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f593a95398737aeed53e489c785df13f3618e41dbcd6718c6addbf1395aa6876" +dependencies = [ + "memchr", +] diff --git a/utils/Cargo.toml b/utils/Cargo.toml index 5bf141dc..f4cd3b28 100644 --- a/utils/Cargo.toml +++ b/utils/Cargo.toml @@ -4,7 +4,8 @@ name = "utils" version = "0.1.0" [dependencies] -if_chain = "1.0.2" +if_chain = "=1.0.2" +clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "51a1cf0" } [package.metadata.rust-analyzer] rustc_private = true diff --git a/utils/src/constant_analyzer/mod.rs b/utils/src/constant_analyzer/mod.rs new file mode 100644 index 00000000..b10ad0b8 --- /dev/null +++ b/utils/src/constant_analyzer/mod.rs @@ -0,0 +1,88 @@ +use std::collections::HashSet; + +extern crate rustc_hir; +extern crate rustc_lint; + +use clippy_utils::consts::constant; +use rustc_hir::{ + def::{DefKind, Res}, + intravisit::{walk_expr, Visitor}, + Expr, ExprKind, HirId, QPath, StmtKind, +}; +use rustc_lint::LateContext; + +pub struct ConstantAnalyzer<'a, 'tcx> { + pub cx: &'a LateContext<'tcx>, + pub constants: HashSet, +} + +impl<'a, 'tcx> ConstantAnalyzer<'a, 'tcx> { + fn is_qpath_constant(&self, path: &QPath) -> bool { + if let QPath::Resolved(_, path) = path { + match path.res { + Res::Def(def_kind, _) => matches!( + def_kind, + DefKind::AnonConst + | DefKind::AssocConst + | DefKind::Const + | DefKind::InlineConst + ), + Res::Local(hir_id) => self.constants.contains(&hir_id), + _ => false, + } + } else { + false + } + } + + fn is_expr_constant(&self, expr: &Expr<'tcx>) -> bool { + // Evaluate the expression as a compile-time constant + if constant(self.cx, self.cx.typeck_results(), expr).is_some() { + return true; + } + + // If the expression is not a constant, verify if it is known at compile time + match expr.kind { + ExprKind::Array(expr_array) => expr_array + .iter() + .all(|expr_in_array| self.is_expr_constant(expr_in_array)), + ExprKind::Binary(_, left_expr, right_expr) => { + self.is_expr_constant(left_expr) && self.is_expr_constant(right_expr) + } + ExprKind::Cast(cast_expr, _) => self.is_expr_constant(cast_expr), + ExprKind::Field(field_expr, _) => self.is_expr_constant(field_expr), + // TODO: array with just index checking + ExprKind::Index(array_expr, index_expr, _) => { + self.is_expr_constant(array_expr) && self.is_expr_constant(index_expr) + } + ExprKind::Lit(_) => true, + ExprKind::Path(qpath_expr) => self.is_qpath_constant(&qpath_expr), + ExprKind::Repeat(repeat_expr, _) => self.is_expr_constant(repeat_expr), + ExprKind::Struct(_, expr_fields, _) => expr_fields + .iter() + .all(|field_expr| self.is_expr_constant(field_expr.expr)), + _ => false, + } + } + + pub fn is_compile_time_known(&self, expr: &Expr<'tcx>) -> bool { + self.is_expr_constant(expr) + } +} + +impl<'a, 'tcx> Visitor<'tcx> for ConstantAnalyzer<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + if let ExprKind::Block(block, _) = expr.kind { + for stmt in block.stmts { + if let StmtKind::Let(local) = stmt.kind { + if let Some(init) = local.init { + if self.is_expr_constant(init) { + self.constants.insert(local.pat.hir_id); + } + } + } + } + } + walk_expr(self, expr); + } +} diff --git a/utils/src/lib.rs b/utils/src/lib.rs index 352bef89..22c331d8 100644 --- a/utils/src/lib.rs +++ b/utils/src/lib.rs @@ -8,3 +8,6 @@ pub use soroban_utils::*; mod lint_utils; pub use lint_utils::*; + +mod constant_analyzer; +pub use constant_analyzer::*; From 50c90117f964da5a5079b7c8485359a55add14c7 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Sun, 28 Jul 2024 13:13:32 -0300 Subject: [PATCH 6/8] Edit ConstantAnalyzer to only check the array element in index --- utils/src/constant_analyzer/mod.rs | 69 +++++++++++++++++++++--------- 1 file changed, 48 insertions(+), 21 deletions(-) diff --git a/utils/src/constant_analyzer/mod.rs b/utils/src/constant_analyzer/mod.rs index b10ad0b8..a9e9457e 100644 --- a/utils/src/constant_analyzer/mod.rs +++ b/utils/src/constant_analyzer/mod.rs @@ -1,22 +1,24 @@ -use std::collections::HashSet; - extern crate rustc_hir; extern crate rustc_lint; -use clippy_utils::consts::constant; +use clippy_utils::consts::{constant, Constant}; +use if_chain::if_chain; use rustc_hir::{ def::{DefKind, Res}, - intravisit::{walk_expr, Visitor}, - Expr, ExprKind, HirId, QPath, StmtKind, + intravisit::{walk_local, Visitor}, + Expr, ExprKind, HirId, Node, QPath, }; use rustc_lint::LateContext; +use std::collections::HashSet; +/// Analyzes expressions to determine if they are constants or known at compile-time. pub struct ConstantAnalyzer<'a, 'tcx> { pub cx: &'a LateContext<'tcx>, pub constants: HashSet, } impl<'a, 'tcx> ConstantAnalyzer<'a, 'tcx> { + /// Checks if a QPath refers to a constant. fn is_qpath_constant(&self, path: &QPath) -> bool { if let QPath::Resolved(_, path) = path { match path.res { @@ -35,13 +37,12 @@ impl<'a, 'tcx> ConstantAnalyzer<'a, 'tcx> { } } + /// Determines if an expression is constant or known at compile-time. fn is_expr_constant(&self, expr: &Expr<'tcx>) -> bool { - // Evaluate the expression as a compile-time constant if constant(self.cx, self.cx.typeck_results(), expr).is_some() { return true; } - // If the expression is not a constant, verify if it is known at compile time match expr.kind { ExprKind::Array(expr_array) => expr_array .iter() @@ -51,9 +52,8 @@ impl<'a, 'tcx> ConstantAnalyzer<'a, 'tcx> { } ExprKind::Cast(cast_expr, _) => self.is_expr_constant(cast_expr), ExprKind::Field(field_expr, _) => self.is_expr_constant(field_expr), - // TODO: array with just index checking ExprKind::Index(array_expr, index_expr, _) => { - self.is_expr_constant(array_expr) && self.is_expr_constant(index_expr) + self.is_array_index_constant(array_expr, index_expr) } ExprKind::Lit(_) => true, ExprKind::Path(qpath_expr) => self.is_qpath_constant(&qpath_expr), @@ -65,24 +65,51 @@ impl<'a, 'tcx> ConstantAnalyzer<'a, 'tcx> { } } - pub fn is_compile_time_known(&self, expr: &Expr<'tcx>) -> bool { + /// Checks if an array index operation results in a constant value. + fn is_array_index_constant(&self, array_expr: &Expr<'tcx>, index_expr: &Expr<'tcx>) -> bool { + match ( + &array_expr.kind, + constant(self.cx, self.cx.typeck_results(), index_expr), + ) { + (ExprKind::Array(array_elements), Some(Constant::Int(index))) => { + self.is_array_element_constant(array_elements, index) + } + (ExprKind::Path(QPath::Resolved(_, path)), Some(Constant::Int(index))) => { + if_chain! { + if let Res::Local(hir_id) = path.res; + if let Node::LetStmt(let_stmt) = self.cx.tcx.parent_hir_node(hir_id); + if let Some(ExprKind::Array(array_elements)) = let_stmt.init.map(|init| &init.kind); + then { + self.is_array_element_constant(array_elements, index) + } else { + false + } + } + } + _ => false, + } + } + + /// Checks if a specific array element is constant. + fn is_array_element_constant(&self, elements: &[Expr<'tcx>], index: u128) -> bool { + elements + .get(index as usize) + .map_or(false, |element| self.is_expr_constant(element)) + } + + /// Public method to check if an expression is constant. + pub fn is_constant(&self, expr: &Expr<'tcx>) -> bool { self.is_expr_constant(expr) } } impl<'a, 'tcx> Visitor<'tcx> for ConstantAnalyzer<'a, 'tcx> { - fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { - if let ExprKind::Block(block, _) = expr.kind { - for stmt in block.stmts { - if let StmtKind::Let(local) = stmt.kind { - if let Some(init) = local.init { - if self.is_expr_constant(init) { - self.constants.insert(local.pat.hir_id); - } - } - } + fn visit_local(&mut self, l: &'tcx rustc_hir::LetStmt<'tcx>) -> Self::Result { + if let Some(init) = l.init { + if self.is_expr_constant(init) { + self.constants.insert(l.pat.hir_id); } } - walk_expr(self, expr); + walk_local(self, l); } } From b8090a1d1adb994b930e657625003371970bd7dd Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Sun, 28 Jul 2024 13:15:13 -0300 Subject: [PATCH 7/8] Update detector to check compile-time known constants --- detectors/integer-overflow-or-underflow/src/lib.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/detectors/integer-overflow-or-underflow/src/lib.rs b/detectors/integer-overflow-or-underflow/src/lib.rs index 325f7889..e1c9756e 100644 --- a/detectors/integer-overflow-or-underflow/src/lib.rs +++ b/detectors/integer-overflow-or-underflow/src/lib.rs @@ -93,8 +93,8 @@ pub struct IntegerOverflowUnderflowVisitor<'a, 'tcx> { impl<'tcx> IntegerOverflowUnderflowVisitor<'_, 'tcx> { pub fn check_pow(&mut self, expr: &Expr<'tcx>, base: &Expr<'tcx>, exponent: &Expr<'tcx>) { - let is_base_known = self.constant_detector.is_compile_time_known(base); - let is_exponent_known = self.constant_detector.is_compile_time_known(exponent); + let is_base_known = self.constant_detector.is_constant(base); + let is_exponent_known = self.constant_detector.is_constant(exponent); if is_base_known && is_exponent_known { return; } @@ -107,7 +107,7 @@ impl<'tcx> IntegerOverflowUnderflowVisitor<'_, 'tcx> { } pub fn check_negate(&mut self, expr: &Expr<'tcx>, operand: &Expr<'tcx>) { - let is_operand_known = self.constant_detector.is_compile_time_known(operand); + let is_operand_known = self.constant_detector.is_constant(operand); if is_operand_known { return; } @@ -128,8 +128,8 @@ impl<'tcx> IntegerOverflowUnderflowVisitor<'_, 'tcx> { left: &Expr<'tcx>, right: &Expr<'tcx>, ) { - let is_left_known = self.constant_detector.is_compile_time_known(left); - let is_right_known = self.constant_detector.is_compile_time_known(right); + let is_left_known = self.constant_detector.is_constant(left); + let is_right_known = self.constant_detector.is_constant(right); if is_left_known && is_right_known { return; } From a8f487696a4b1d3e81a755fad37f2426245aa21d Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Sun, 28 Jul 2024 17:41:08 -0300 Subject: [PATCH 8/8] Fix clippy issue --- .../integer-overflow-or-underflow/src/lib.rs | 34 ++++++++----------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/detectors/integer-overflow-or-underflow/src/lib.rs b/detectors/integer-overflow-or-underflow/src/lib.rs index e1c9756e..031282ca 100644 --- a/detectors/integer-overflow-or-underflow/src/lib.rs +++ b/detectors/integer-overflow-or-underflow/src/lib.rs @@ -3,7 +3,7 @@ extern crate rustc_hir; extern crate rustc_span; -use clippy_utils::{consts::constant, diagnostics::span_lint_and_help}; +use clippy_utils::diagnostics::span_lint_and_help; use rustc_hir::{ intravisit::{walk_expr, FnKind, Visitor}, BinOpKind, Body, Expr, ExprKind, FnDecl, UnOp, @@ -88,14 +88,13 @@ pub struct IntegerOverflowUnderflowVisitor<'a, 'tcx> { cx: &'a LateContext<'tcx>, findings: Vec, is_complex_operation: bool, - constant_detector: ConstantAnalyzer<'a, 'tcx>, + constant_analyzer: ConstantAnalyzer<'a, 'tcx>, } impl<'tcx> IntegerOverflowUnderflowVisitor<'_, 'tcx> { pub fn check_pow(&mut self, expr: &Expr<'tcx>, base: &Expr<'tcx>, exponent: &Expr<'tcx>) { - let is_base_known = self.constant_detector.is_constant(base); - let is_exponent_known = self.constant_detector.is_constant(exponent); - if is_base_known && is_exponent_known { + if self.constant_analyzer.is_constant(base) && self.constant_analyzer.is_constant(exponent) + { return; } @@ -107,17 +106,14 @@ impl<'tcx> IntegerOverflowUnderflowVisitor<'_, 'tcx> { } pub fn check_negate(&mut self, expr: &Expr<'tcx>, operand: &Expr<'tcx>) { - let is_operand_known = self.constant_detector.is_constant(operand); - if is_operand_known { + if self.constant_analyzer.is_constant(operand) { return; } let operand_type = self.cx.typeck_results().expr_ty(operand); if operand_type.is_integral() && operand_type.is_signed() { - if constant(self.cx, self.cx.typeck_results(), operand).is_none() { - self.findings - .push(Finding::new(expr.span, Type::Overflow, Cause::Negate)); - } + self.findings + .push(Finding::new(expr.span, Type::Overflow, Cause::Negate)); } } @@ -128,17 +124,15 @@ impl<'tcx> IntegerOverflowUnderflowVisitor<'_, 'tcx> { left: &Expr<'tcx>, right: &Expr<'tcx>, ) { - let is_left_known = self.constant_detector.is_constant(left); - let is_right_known = self.constant_detector.is_constant(right); - if is_left_known && is_right_known { + if self.constant_analyzer.is_constant(left) && self.constant_analyzer.is_constant(right) { return; } let (left_type, right_type) = ( - self.cx.typeck_results().expr_ty(left), - self.cx.typeck_results().expr_ty(right), + self.cx.typeck_results().expr_ty(left).peel_refs(), + self.cx.typeck_results().expr_ty(right).peel_refs(), ); - if !left_type.peel_refs().is_integral() || !right_type.peel_refs().is_integral() { + if !left_type.is_integral() || !right_type.is_integral() { return; } @@ -200,18 +194,18 @@ impl<'tcx> LateLintPass<'tcx> for IntegerOverflowUnderflow { } // Gather all compile-time variables in the function - let mut constant_detector = ConstantAnalyzer { + let mut constant_analyzer = ConstantAnalyzer { cx, constants: HashSet::new(), }; - constant_detector.visit_body(body); + constant_analyzer.visit_body(body); // Analyze the function for integer overflow/underflow let mut visitor = IntegerOverflowUnderflowVisitor { cx, findings: Vec::new(), is_complex_operation: false, - constant_detector, + constant_analyzer, }; visitor.visit_body(body);