-
Notifications
You must be signed in to change notification settings - Fork 5
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
* Add first test case * Add second test case * Add detector to list * Add detector * Edit second vulnerable test-case * Run cargo-fmt * Add detector unused-return-enum * Add unused return enum vulnerability * Add detector documentation unused return enum * Add second test case reference unused return enum * Fix some clippy errors on tests * Fix CI * Fix CI 2 --------- Co-authored-by: Arturo Beccar-Varela <[email protected]>
- Loading branch information
1 parent
ba10983
commit c25a572
Showing
18 changed files
with
574 additions
and
11 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| [package] | ||
| name = "unused-return-enum" | ||
| version = "0.1.0" | ||
| edition = "2021" | ||
|
|
||
| [lib] | ||
| crate-type = ["cdylib"] | ||
|
|
||
| [dependencies] | ||
| dylint_linting = { workspace = true } | ||
| if_chain = { workspace = true } | ||
|
|
||
| scout-audit-internal = { workspace = true } | ||
|
|
||
| [dev-dependencies] | ||
| dylint_testing = { workspace = true } | ||
|
|
||
| [package.metadata.rust-analyzer] | ||
| rustc_private = true |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| # Unused return enum | ||
|
|
||
| ### What it does | ||
|
|
||
| It warns if a function that returns a Result type does not return the Result enum variant (Ok/Err). | ||
|
|
||
| ### Why is this bad? | ||
|
|
||
| If any of the variants (Ok/Err) is not used, the code could be simplified or it could imply a bug. | ||
|
|
||
| ### Known problems | ||
|
|
||
| If definitions of `Err()` and/or `Ok()` are in the code but do not flow to the return value due to the definition of a variable or because they are defined in a dead code block, the warning will not be shown. If the definitions are made in an auxiliary method, the warning will be shown, resulting in a false positive. | ||
|
|
||
| ### Example | ||
|
|
||
| Instead of using: | ||
|
|
||
| ```rust | ||
| #![no_std] | ||
|
|
||
| use soroban_sdk::{contract, contracterror, contractimpl}; | ||
|
|
||
| #[contract] | ||
| pub struct UnusedReturnEnum; | ||
|
|
||
| #[contracterror] | ||
| #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] | ||
| #[repr(u32)] | ||
| pub enum Error { | ||
| /// An overflow was produced. | ||
| Overflow = 1, | ||
| } | ||
|
|
||
| #[contractimpl] | ||
| impl UnusedReturnEnum { | ||
| /// Returns the percentage difference between two values. | ||
| pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result<u128, Error> { | ||
| let absolute_difference = balance1.abs_diff(balance2); | ||
| let sum = balance1 + balance2; | ||
|
|
||
| match 100u128.checked_mul(absolute_difference / sum) { | ||
| Some(result) => result, | ||
| None => panic!("Overflow"), | ||
| }; | ||
|
|
||
| Err(Error::Overflow) | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Use this: | ||
|
|
||
| ```rust | ||
| #![no_std] | ||
|
|
||
| use soroban_sdk::{contract, contracterror, contractimpl, testutils::arbitrary::arbitrary::Result}; | ||
|
|
||
| #[contract] | ||
| pub struct UnusedReturnEnum; | ||
|
|
||
| #[contracterror] | ||
| #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] | ||
| #[repr(u32)] | ||
| pub enum Error { | ||
| /// An overflow was produced. | ||
| Overflow = 1, | ||
| } | ||
|
|
||
| #[contractimpl] | ||
| impl UnusedReturnEnum { | ||
| /// Returns the percentage difference between two values. | ||
| pub fn get_percentage_difference(balance1: u128, balance2: u128) -> Result<u128, Error> { | ||
| let absolute_difference = balance1.abs_diff(balance2); | ||
| let sum = balance1 + balance2; | ||
|
|
||
| match 100u128.checked_mul(absolute_difference / sum) { | ||
| Some(result) => Ok(result), | ||
| None => Err(Error::Overflow), | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Implementation | ||
|
|
||
| The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unused-return-enum). |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| #![feature(rustc_private)] | ||
|
|
||
| extern crate rustc_hir; | ||
| extern crate rustc_span; | ||
|
|
||
| use if_chain::if_chain; | ||
| use rustc_hir::intravisit::{walk_expr, FnKind, Visitor}; | ||
| use rustc_hir::{Body, Expr, ExprKind, FnDecl, FnRetTy, MatchSource, QPath, TyKind}; | ||
| use rustc_lint::{LateContext, LateLintPass}; | ||
| use rustc_span::{def_id::LocalDefId, sym, Span}; | ||
| use scout_audit_internal::Detector; | ||
|
|
||
| dylint_linting::declare_late_lint! { | ||
| pub UNUSED_RETURN_ENUM, | ||
| Warn, | ||
| Detector::UnusedReturnEnum.get_lint_message() | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| struct CounterVisitor { | ||
| count_err: u32, | ||
| count_ok: u32, | ||
| found_try: bool, | ||
| found_return: bool, | ||
| span: Vec<Span>, | ||
| } | ||
|
|
||
| impl<'tcx> Visitor<'tcx> for CounterVisitor { | ||
| fn visit_expr(&mut self, expr: &'tcx Expr) { | ||
| match expr.kind { | ||
| ExprKind::Call(function, _) => { | ||
| if_chain! { | ||
| if let ExprKind::Path(QPath::Resolved(_, path)) = &function.kind; | ||
| if let Some(last_segment) = path.segments.last(); | ||
| then { | ||
| if last_segment.ident.name == sym::Ok { | ||
| self.count_ok += 1; | ||
| self.span.push(expr.span); | ||
| } else if last_segment.ident.name == sym::Err { | ||
| self.count_err += 1; | ||
| self.span.push(expr.span); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ExprKind::Ret(Some(return_value)) => { | ||
| if_chain! { | ||
| if let ExprKind::Call(function, _) = &return_value.kind; | ||
| if let ExprKind::Path(QPath::Resolved(_, path)) = &function.kind; | ||
| if let Some(last_segment) = path.segments.last(); | ||
| then { | ||
| if last_segment.ident.name != sym::Ok && last_segment.ident.name != sym::Err { | ||
| self.found_return = true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ExprKind::Match(_, _, MatchSource::TryDesugar(_)) => { | ||
| self.found_try = true; | ||
| } | ||
| _ => {} | ||
| } | ||
| walk_expr(self, expr); | ||
| } | ||
| } | ||
| impl<'tcx> LateLintPass<'tcx> for UnusedReturnEnum { | ||
| fn check_fn( | ||
| &mut self, | ||
| cx: &LateContext<'tcx>, | ||
| fnkind: FnKind<'tcx>, | ||
| decl: &'tcx FnDecl<'tcx>, | ||
| body: &'tcx Body<'tcx>, | ||
| _: Span, | ||
| _: LocalDefId, | ||
| ) { | ||
| // If the function is not a method, or it comes from a macro expansion, we ignore it | ||
| if !matches!(fnkind, FnKind::Method(_, fnsig) if !fnsig.span.from_expansion()) { | ||
| return; | ||
| } | ||
|
|
||
| // If the function returns a type different from Result, we ignore it | ||
| if_chain! { | ||
| if let FnRetTy::Return(return_type) = &decl.output; | ||
| if let TyKind::Path(qpath) = &return_type.kind; | ||
| if let QPath::Resolved(_ty, path) = qpath; | ||
| if let Some(path_segment) = path.segments.last(); | ||
| if path_segment.ident.name != sym::Result; | ||
| then { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| let mut visitor = CounterVisitor { | ||
| count_ok: 0, | ||
| count_err: 0, | ||
| found_try: false, | ||
| found_return: false, | ||
| span: Vec::new(), | ||
| }; | ||
|
|
||
| walk_expr(&mut visitor, body.value); | ||
|
|
||
| if !visitor.found_return | ||
| && !visitor.found_try | ||
| && (visitor.count_err == 0 || visitor.count_ok == 0) | ||
| { | ||
| visitor.span.iter().for_each(|span| { | ||
| Detector::UnusedReturnEnum.span_lint_and_help( | ||
| cx, | ||
| UNUSED_RETURN_ENUM, | ||
| *span, | ||
| "If any of the variants (Ok/Err) is not used, the code could be simplified or it could imply a bug", | ||
| ); | ||
| }); | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
30 changes: 30 additions & 0 deletions
30
test-cases/unused-return-enum/unused-return-enum-1/remediated-example/Cargo.toml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| [package] | ||
| name = "unused-return-enum-remediated-1" | ||
| version = "0.1.0" | ||
| edition = "2021" | ||
|
|
||
| [lib] | ||
| crate-type = ["cdylib"] | ||
|
|
||
| [dependencies] | ||
| soroban-sdk = { version = "=20.0.0" } | ||
|
|
||
| [dev_dependencies] | ||
| soroban-sdk = { version = "=20.0.0", features = ["testutils"] } | ||
|
|
||
| [features] | ||
| testutils = ["soroban-sdk/testutils"] | ||
|
|
||
| [profile.release] | ||
| opt-level = "z" | ||
| overflow-checks = true | ||
| debug = 0 | ||
| strip = "symbols" | ||
| debug-assertions = false | ||
| panic = "abort" | ||
| codegen-units = 1 | ||
| lto = true | ||
|
|
||
| [profile.release-with-logs] | ||
| inherits = "release" | ||
| debug-assertions = true |
Oops, something went wrong.