diff --git a/CHANGELOG.md b/CHANGELOG.md index 41e6a2dd2e..1971c2d966 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added +- Linter: `non_fallible_api` lint - [#2004](https://github.com/paritytech/ink/pull/2004) + ### Fixed - Fix the `StorageVec` type by excluding the `len_cached` field from its type info - [#2052](https://github.com/paritytech/ink/pull/2052) diff --git a/linting/extra/Cargo.toml b/linting/extra/Cargo.toml index dbbeb2719d..3fff7174a2 100644 --- a/linting/extra/Cargo.toml +++ b/linting/extra/Cargo.toml @@ -23,11 +23,12 @@ dylint_linting = "2.1.12" if_chain = "1.0.2" log = "0.4.14" regex = "1.5.4" +# ink! dependencies used in the linter implementation +ink_env = { path = "../../crates/env", default-features = false } [dev-dependencies] dylint_testing = "2.1.12" # The following are ink! dependencies, they are only required for the `ui` tests. -ink_env = { path = "../../crates/env", default-features = false } ink = { path = "../../crates/ink", default-features = false, features = ["std"] } ink_metadata = { path = "../../crates/metadata", default-features = false } ink_primitives = { path = "../../crates/primitives", default-features = false } @@ -62,6 +63,12 @@ path = "ui/pass/strict_balance_equality.rs" [[example]] name = "strict_balance_equality_fail" path = "ui/fail/strict_balance_equality.rs" +[[example]] +name = "non_fallible_api_pass" +path = "ui/pass/non_fallible_api.rs" +[[example]] +name = "non_fallible_api_fail" +path = "ui/fail/non_fallible_api.rs" [package.metadata.rust-analyzer] rustc_private = true diff --git a/linting/extra/src/lib.rs b/linting/extra/src/lib.rs index fdb19ab04b..00cc666d70 100644 --- a/linting/extra/src/lib.rs +++ b/linting/extra/src/lib.rs @@ -30,8 +30,10 @@ extern crate rustc_middle; extern crate rustc_mir_dataflow; extern crate rustc_session; extern crate rustc_span; +extern crate rustc_type_ir; mod ink_utils; +mod non_fallible_api; mod primitive_topic; mod storage_never_freed; mod strict_balance_equality; @@ -46,11 +48,13 @@ pub fn register_lints( primitive_topic::PRIMITIVE_TOPIC, storage_never_freed::STORAGE_NEVER_FREED, strict_balance_equality::STRICT_BALANCE_EQUALITY, + non_fallible_api::NON_FALLIBLE_API, ]); lint_store.register_late_pass(|_| Box::new(primitive_topic::PrimitiveTopic)); lint_store.register_late_pass(|_| Box::new(storage_never_freed::StorageNeverFreed)); lint_store .register_late_pass(|_| Box::new(strict_balance_equality::StrictBalanceEquality)); + lint_store.register_late_pass(|_| Box::new(non_fallible_api::NonFallibleAPI)); } #[test] diff --git a/linting/extra/src/non_fallible_api.rs b/linting/extra/src/non_fallible_api.rs new file mode 100644 index 0000000000..d709ece227 --- /dev/null +++ b/linting/extra/src/non_fallible_api.rs @@ -0,0 +1,299 @@ +// Copyright (C) Parity Technologies (UK) Ltd. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use crate::ink_utils::{ + expand_unnamed_consts, + find_contract_impl_id, +}; +use clippy_utils::{ + diagnostics::span_lint_and_then, + is_lint_allowed, + match_def_path, +}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{ + self as hir, + def_id::DefId, + intravisit::{ + walk_body, + walk_expr, + Visitor, + }, + Body, + Expr, + ExprKind, + ImplItemKind, + ItemKind, + PathSegment, +}; +use rustc_lint::{ + LateContext, + LateLintPass, +}; +use rustc_middle::{ + hir::nested_filter, + ty::{ + self, + ConstKind, + Ty, + TypeckResults, + }, +}; +use rustc_session::{ + declare_lint, + declare_lint_pass, +}; +use rustc_type_ir::sty::TyKind; + +declare_lint! { + /// ## What it does + /// + /// The lint detects potentially unsafe uses of methods for which there are safer alternatives. + /// + /// ## Why is this bad? + /// + /// In some standard collections in ink!, there are two types of implementations: non-fallible + /// (e.g. `get`) and fallible (e.g. `try_get`). Fallible alternatives are considered safer, + /// as they perform additional checks for incorrect input parameters and return `Result::Err` + /// when they are used improperly. On the other hand, non-fallible methods do not provide these + /// checks and will panic on incorrect input, placing the responsibility on the user to + /// implement these checks. + /// + /// For more context, see: [ink#1910](https://github.com/paritytech/ink/pull/1910). + /// + /// ## Example + /// + /// Consider the contract that has the following `Mapping` field: + /// + /// ```rust + /// #[ink(storage)] + /// pub struct Example { map: Mapping } + /// ``` + /// + /// The following usage of the non-fallible API is unsafe: + /// + /// ```rust + /// // Bad: can panic if `input_string` doesn't fit into the static buffer + /// self.map.insert(input_string, &self.sender); + /// ``` + /// + /// It could be replaced with the fallible version of `Mapping::insert`: + /// + /// ```rust + /// // Good: returns Result::Err on incorrect input + /// self.map.try_insert(input_string, &self.sender); + /// ``` + /// + /// Otherwise, the user could explicitly check the encoded size of the argument in their code: + /// + /// ```rust + /// // Good: explicitly checked encoded size of the input + /// if String::encoded_size(&input_string) < ink_env::BUFFER_SIZE { + /// self.map.insert(input_string, &self.sender); + /// } + /// ``` + pub NON_FALLIBLE_API, + Warn, + "using non-fallible API" +} + +declare_lint_pass!(NonFallibleAPI => [NON_FALLIBLE_API]); + +#[derive(Debug)] +enum TyToCheck { + Mapping, + Lazy, +} + +impl TyToCheck { + pub fn try_from_adt(cx: &LateContext<'_>, did: DefId) -> Option { + if match_def_path(cx, did, &["ink_storage", "lazy", "Lazy"]) { + return Some(Self::Lazy) + } + + if match_def_path(cx, did, &["ink_storage", "lazy", "mapping", "Mapping"]) { + return Some(Self::Mapping) + } + None + } + + pub fn find_fallible_alternative(&self, method_name: &str) -> Option { + use TyToCheck::*; + match self { + Mapping => { + match method_name { + "insert" => Some("try_insert".to_string()), + "get" => Some("try_get".to_string()), + "take" => Some("try_take".to_string()), + _ => None, + } + } + Lazy => { + match method_name { + "get" => Some("try_get".to_string()), + "set" => Some("try_set".to_string()), + _ => None, + } + } + } + } +} + +/// Visitor that finds usage of non-fallible calls in the bodies of functions +struct APIUsageChecker<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + maybe_typeck_results: Option<&'tcx TypeckResults<'tcx>>, +} + +impl<'a, 'tcx> APIUsageChecker<'a, 'tcx> { + pub fn new(cx: &'a LateContext<'tcx>) -> Self { + Self { + cx, + maybe_typeck_results: cx.maybe_typeck_results(), + } + } + + /// Returns true iff the given type has statically known size when encoded with + /// `scale_codec` + fn is_statically_known(&self, ty: &Ty<'tcx>) -> bool { + match ty.kind() { + ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str => { + true + } + ty::Tuple(inner_tys) => { + inner_tys.iter().all(|ty| self.is_statically_known(&ty)) + } + ty::Ref(_, inner, _) => self.is_statically_known(inner), + ty::Adt(adt_def, substs) => { + adt_def.variants().iter().all(|variant| { + variant.fields.iter().all(|field| { + self.is_statically_known(&field.ty(self.cx.tcx, substs)) + }) + }) + } + ty::Array(inner_ty, len_const) => { + if_chain! { + if self.is_statically_known(inner_ty); + if let ConstKind::Value(ty::ValTree::Leaf(elements_count)) = len_const.kind(); + if let Ok(elements_size) = elements_count.try_to_target_usize(self.cx.tcx); + if elements_size < (ink_env::BUFFER_SIZE as u64); + then { true } else { false } + } + } + _ => false, + } + } + + /// Raises warnings if the given method call is potentially unsafe and could be + /// replaced + fn check_method_call( + &self, + receiver_ty: &TyToCheck, + method_path: &PathSegment, + method_name: &str, + arg_ty: Ty<'tcx>, + ) { + if_chain! { + if !self.is_statically_known(&arg_ty); + if let Some(fallible_method) = receiver_ty.find_fallible_alternative(method_name); + then { + span_lint_and_then( + self.cx, + NON_FALLIBLE_API, + method_path.ident.span, + format!( + "using a non-fallible `{:?}::{}` with an argument that may not fit into the static buffer", + receiver_ty, + method_name, + ).as_str(), + |diag| { + diag.span_suggestion( + method_path.ident.span, + format!("consider using `{}`", fallible_method), + "", + Applicability::Unspecified, + ); + }, + ) + } + } + } +} + +impl<'a, 'tcx> Visitor<'tcx> for APIUsageChecker<'a, 'tcx> { + type NestedFilter = nested_filter::OnlyBodies; + + fn visit_expr(&mut self, e: &'tcx Expr<'tcx>) { + if_chain! { + if !is_lint_allowed(self.cx, NON_FALLIBLE_API, e.hir_id); + if let ExprKind::MethodCall(method_path, receiver, _, _) = &e.kind; + if let Some(typeck_results) = self.maybe_typeck_results; + let ty = typeck_results.expr_ty(receiver); + if let TyKind::Adt(def, substs) = ty.kind(); + if let Some(ty) = TyToCheck::try_from_adt(self.cx, def.0.0.did); + then { + substs + .iter() + .take(substs.len() - 1) + .filter_map(|subst| subst.as_type()) + .for_each(|arg_ty| { + self.check_method_call( + &ty, + method_path, + &method_path.ident.to_string(), + arg_ty) + }) + } + } + walk_expr(self, e); + } + + fn visit_body(&mut self, body: &'tcx Body<'_>) { + let old_maybe_typeck_results = self + .maybe_typeck_results + .replace(self.cx.tcx.typeck_body(body.id())); + walk_body(self, body); + self.maybe_typeck_results = old_maybe_typeck_results; + } + + fn nested_visit_map(&mut self) -> Self::Map { + self.cx.tcx.hir() + } +} + +impl<'tcx> LateLintPass<'tcx> for NonFallibleAPI { + fn check_mod( + &mut self, + cx: &LateContext<'tcx>, + m: &'tcx hir::Mod<'tcx>, + _: hir::HirId, + ) { + if_chain! { + let all_item_ids = expand_unnamed_consts(cx, m.item_ids); + if let Some(contract_impl_id) = find_contract_impl_id(cx, all_item_ids); + let contract_impl = cx.tcx.hir().item(contract_impl_id); + if let ItemKind::Impl(contract_impl) = contract_impl.kind; + then { + contract_impl.items.iter().for_each(|impl_item| { + let impl_item = cx.tcx.hir().impl_item(impl_item.id); + if let ImplItemKind::Fn(..) = impl_item.kind { + let mut visitor = APIUsageChecker::new(cx); + visitor.visit_impl_item(impl_item); + } + }) + } + } + } +} diff --git a/linting/extra/ui/fail/non_fallible_api.rs b/linting/extra/ui/fail/non_fallible_api.rs new file mode 100644 index 0000000000..35cf109ac5 --- /dev/null +++ b/linting/extra/ui/fail/non_fallible_api.rs @@ -0,0 +1,59 @@ +#![cfg_attr(not(feature = "std"), no_std, no_main)] + +pub type TyAlias1 = ink::prelude::vec::Vec; +pub type TyAlias2 = TyAlias1; + +#[ink::contract] +pub mod non_fallible_api { + use crate::TyAlias2; + use ink::{ + prelude::{ + string::String, + vec::Vec, + }, + storage::{ + Lazy, + Mapping, + }, + }; + + #[ink(storage)] + pub struct NonFallibleAPI { + map_1: Mapping, + map_2: Mapping, + lazy_1: Lazy, + lazy_2: Lazy<(String, String)>, + } + + impl NonFallibleAPI { + #[ink(constructor)] + pub fn new() -> Self { + Self { + map_1: Mapping::new(), + map_2: Mapping::new(), + lazy_1: Lazy::new(), + lazy_2: Lazy::new(), + } + } + + // Raise warnings when using non-fallible API with argument which encoded size is + // statically unknown. + #[ink(message)] + pub fn non_fallible_not_statically_known(&mut self, a: String, b: String) { + // Mapping + let _ = self.map_1.insert(a.clone(), &b); + let _ = self.map_1.get(a.clone()); + let _ = self.map_1.take(a.clone()); + let mut v = Vec::new(); + v.push(42); + let _ = self.map_2.insert(42, &v); + + // Lazy + let _ = self.lazy_1.get(); + self.lazy_1.set(&a); + self.lazy_2.set(&(a.clone(), a)); + } + } +} + +fn main() {} diff --git a/linting/extra/ui/fail/non_fallible_api.stderr b/linting/extra/ui/fail/non_fallible_api.stderr new file mode 100644 index 0000000000..84ed53d429 --- /dev/null +++ b/linting/extra/ui/fail/non_fallible_api.stderr @@ -0,0 +1,46 @@ +error: using a non-fallible `Mapping::insert` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:44:32 + | +LL | let _ = self.map_1.insert(a.clone(), &b); + | ^^^^^^ help: consider using `try_insert` + | + = note: `-D non-fallible-api` implied by `-D warnings` + +error: using a non-fallible `Mapping::get` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:45:32 + | +LL | let _ = self.map_1.get(a.clone()); + | ^^^ help: consider using `try_get` + +error: using a non-fallible `Mapping::take` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:46:32 + | +LL | let _ = self.map_1.take(a.clone()); + | ^^^^ help: consider using `try_take` + +error: using a non-fallible `Mapping::insert` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:49:32 + | +LL | let _ = self.map_2.insert(42, &v); + | ^^^^^^ help: consider using `try_insert` + +error: using a non-fallible `Lazy::get` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:52:33 + | +LL | let _ = self.lazy_1.get(); + | ^^^ help: consider using `try_get` + +error: using a non-fallible `Lazy::set` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:53:25 + | +LL | self.lazy_1.set(&a); + | ^^^ help: consider using `try_set` + +error: using a non-fallible `Lazy::set` with an argument that may not fit into the static buffer + --> $DIR/non_fallible_api.rs:54:25 + | +LL | self.lazy_2.set(&(a.clone(), a)); + | ^^^ help: consider using `try_set` + +error: aborting due to 7 previous errors + diff --git a/linting/extra/ui/pass/non_fallible_api.rs b/linting/extra/ui/pass/non_fallible_api.rs new file mode 100644 index 0000000000..b275c87b04 --- /dev/null +++ b/linting/extra/ui/pass/non_fallible_api.rs @@ -0,0 +1,82 @@ +#![cfg_attr(not(feature = "std"), no_std, no_main)] + +use ink_primitives::AccountId; + +pub type TyAlias1 = AccountId; +pub type TyAlias2 = TyAlias1; + +#[ink::contract] +pub mod non_fallible_api { + use crate::TyAlias2; + use ink::{ + prelude::string::String, + storage::{ + Lazy, + Mapping, + }, + }; + + #[ink(storage)] + pub struct NonFallibleAPI { + map_1: Mapping, + map_2: Mapping, + map_3: Mapping, + lazy_1: Lazy, + lazy_2: Lazy, + lazy_3: Lazy, + } + + impl NonFallibleAPI { + #[ink(constructor)] + pub fn new() -> Self { + Self { + map_1: Mapping::new(), + map_2: Mapping::new(), + map_3: Mapping::new(), + lazy_1: Lazy::new(), + lazy_2: Lazy::new(), + lazy_3: Lazy::new(), + } + } + + // Don't generate warnings when using the fallible API + #[ink(message)] + pub fn fallible(&mut self, a: AccountId, b: AccountId) { + // Mapping + let _ = self.map_1.try_insert(a, &b); + let _ = self.map_1.try_get(a); + let _ = self.map_1.try_take(a); + + // Lazy + let _ = self.lazy_1.try_get(); + let _ = self.lazy_1.try_set(&a); + } + + // Don't raise warnings when using non-fallible API with argument which encoded + // size is statically known. + #[ink(message)] + pub fn non_fallible_statically_known(&mut self, a: AccountId, b: AccountId) { + // Mapping + let _ = self.map_1.insert(a, &b); + let _ = self.map_1.get(a); + let _ = self.map_1.take(a); + let _ = self.map_2.insert(a, &[b; 1]); + let _ = self.map_3.insert(a, &(b, b)); + + // Lazy + let _ = self.lazy_1.get(); + self.lazy_1.set(&a); + let _ = self.lazy_2.get(); + self.lazy_2.set(&a); + } + + // Check if local suppressions work + #[ink(message)] + pub fn suppressions(&mut self, a: String) { + #[cfg_attr(dylint_lib = "ink_linting", allow(non_fallible_api))] + self.lazy_3.set(&a); + } + } +} + +fn main() {}