diff --git a/CHANGELOG.md b/CHANGELOG.md index a8b00c9cdbe..ebf96986dc6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,10 +6,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -## Changed +### Changed - Make `set_code_hash` generic - [#1906](https://github.com/paritytech/ink/pull/1906) - Clean E2E configuration parsing - [#1922](https://github.com/paritytech/ink/pull/1922) +### Added +- Linter: `storage_never_freed` lint - [#1932](https://github.com/paritytech/ink/pull/1932) + ## Version 5.0.0-alpha The preview release of the ink! 5.0.0 release. diff --git a/linting/Cargo.toml b/linting/Cargo.toml index 3f91dc017ae..6da4f19c306 100644 --- a/linting/Cargo.toml +++ b/linting/Cargo.toml @@ -51,6 +51,12 @@ path = "ui/pass/primitive_topic.rs" [[example]] name = "primitive_topic_fail" path = "ui/fail/primitive_topic.rs" +[[example]] +name = "storage_never_freed_pass" +path = "ui/pass/storage_never_freed.rs" +[[example]] +name = "storage_never_freed_fail" +path = "ui/fail/storage_never_freed.rs" [package.metadata.rust-analyzer] rustc_private = true diff --git a/linting/src/ink_utils.rs b/linting/src/ink_utils.rs new file mode 100644 index 00000000000..d1987c17f91 --- /dev/null +++ b/linting/src/ink_utils.rs @@ -0,0 +1,148 @@ +// 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 clippy_utils::match_def_path; +use if_chain::if_chain; +use rustc_hir::{ + ExprKind, + HirId, + ItemId, + ItemKind, + QPath, + StmtKind, + Ty, + TyKind, +}; +use rustc_lint::LateContext; + +/// Returns `true` iff the ink storage attribute is defined for the given HIR +fn has_storage_attr(cx: &LateContext, hir: HirId) -> bool { + const INK_STORAGE: &str = "__ink_dylint_Storage"; + let attrs = format!("{:?}", cx.tcx.hir().attrs(hir)); + attrs.contains(INK_STORAGE) +} + +/// Returns `ItemId` of the structure annotated with `#[ink(storage)]` +pub(crate) fn find_storage_struct( + cx: &LateContext, + item_ids: &[ItemId], +) -> Option { + item_ids + .iter() + .find(|&item_id| { + let item = cx.tcx.hir().item(*item_id); + if_chain! { + if has_storage_attr(cx, item.hir_id()); + if let ItemKind::Struct(..) = item.kind; + then { true } else { false } + + } + }) + .copied() +} + +// TODO: Extracted from #1914; reuse this in #1914 when it is merged +/// Returns `ItemId`s defined inside the code block of `const _: () = {}`. +/// +/// The Rust code expanded after ink! code generation used these to define different +/// implementations of a contract. +fn items_in_unnamed_const(cx: &LateContext<'_>, id: &ItemId) -> Vec { + if_chain! { + if let ItemKind::Const(ty, body_id) = cx.tcx.hir().item(*id).kind; + if let TyKind::Tup([]) = ty.kind; + let body = cx.tcx.hir().body(body_id); + if let ExprKind::Block(block, _) = body.value.kind; + then { + block.stmts.iter().fold(Vec::new(), |mut acc, stmt| { + if let StmtKind::Item(id) = stmt.kind { + // We don't call `items_in_unnamed_const` here recursively, because the source + // code generated by ink! doesn't have nested `const _: () = {}` expressions. + acc.push(id) + } + acc + }) + } else { + vec![] + } + } +} + +// TODO: Extracted from #1914; reuse this in #1914 when it is merged +/// Collect all the `ItemId`s in nested `const _: () = {}` +pub(crate) fn expand_unnamed_consts( + cx: &LateContext<'_>, + item_ids: &[ItemId], +) -> Vec { + item_ids.iter().fold(Vec::new(), |mut acc, item_id| { + acc.push(*item_id); + acc.append(&mut items_in_unnamed_const(cx, item_id)); + acc + }) +} + +// TODO: Extracted from #1914; reuse this in #1914 when it is merged +/// Finds type of the struct that implements a contract with user-defined code +fn find_contract_ty_hir<'tcx>( + cx: &LateContext<'tcx>, + item_ids: &[ItemId], +) -> Option<&'tcx Ty<'tcx>> { + item_ids + .iter() + .find_map(|item_id| { + if_chain! { + let item = cx.tcx.hir().item(*item_id); + if let ItemKind::Impl(item_impl) = &item.kind; + if let Some(trait_ref) = cx.tcx.impl_trait_ref(item.owner_id); + if match_def_path( + cx, + trait_ref.skip_binder().def_id, + &["ink_env", "contract", "ContractEnv"], + ); + then { Some(&item_impl.self_ty) } else { None } + } + }) + .copied() +} + +// TODO: Extracted from #1914; reuse this in #1914 when it is merged +/// Compares types of two user-defined structs +fn eq_hir_struct_tys(lhs: &Ty<'_>, rhs: &Ty<'_>) -> bool { + match (lhs.kind, rhs.kind) { + ( + TyKind::Path(QPath::Resolved(_, lhs_path)), + TyKind::Path(QPath::Resolved(_, rhs_path)), + ) => lhs_path.res.eq(&rhs_path.res), + _ => false, + } +} + +// TODO: Extracted from #1914; reuse this in #1914 when it is merged +/// Finds an ID of the implementation of the contract struct containing user-defined code +pub(crate) fn find_contract_impl_id( + cx: &LateContext<'_>, + item_ids: Vec, +) -> Option { + let contract_struct_ty = find_contract_ty_hir(cx, &item_ids)?; + item_ids + .iter() + .find(|item_id| { + if_chain! { + let item = cx.tcx.hir().item(**item_id); + if let ItemKind::Impl(item_impl) = &item.kind; + if item_impl.of_trait.is_none(); + if eq_hir_struct_tys(contract_struct_ty, item_impl.self_ty); + then { true } else { false } + } + }) + .copied() +} diff --git a/linting/src/lib.rs b/linting/src/lib.rs index 5b1dc50b4b6..91c0dd640c7 100644 --- a/linting/src/lib.rs +++ b/linting/src/lib.rs @@ -28,7 +28,9 @@ extern crate rustc_middle; extern crate rustc_session; extern crate rustc_span; +mod ink_utils; mod primitive_topic; +mod storage_never_freed; #[doc(hidden)] #[no_mangle] @@ -36,8 +38,12 @@ pub fn register_lints( _sess: &rustc_session::Session, lint_store: &mut rustc_lint::LintStore, ) { - lint_store.register_lints(&[primitive_topic::PRIMITIVE_TOPIC]); + lint_store.register_lints(&[ + primitive_topic::PRIMITIVE_TOPIC, + storage_never_freed::STORAGE_NEVER_FREED, + ]); lint_store.register_late_pass(|_| Box::new(primitive_topic::PrimitiveTopic)); + lint_store.register_late_pass(|_| Box::new(storage_never_freed::StorageNeverFreed)); } #[test] diff --git a/linting/src/storage_never_freed.rs b/linting/src/storage_never_freed.rs new file mode 100644 index 00000000000..6d09769b5f1 --- /dev/null +++ b/linting/src/storage_never_freed.rs @@ -0,0 +1,339 @@ +// 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, + find_storage_struct, +}; +use clippy_utils::{ + diagnostics::span_lint_and_help, + is_lint_allowed, + match_def_path, + match_path, +}; +use if_chain::if_chain; +use rustc_hir::{ + self as hir, + def::{ + DefKind, + Res, + }, + def_id::{ + DefId, + LocalDefId, + }, + intravisit::{ + walk_body, + walk_expr, + Visitor, + }, + Expr, + ExprKind, + ImplItemKind, + ItemId, + ItemKind, + Node, + Path, + QPath, + TyKind, +}; +use rustc_lint::{ + LateContext, + LateLintPass, +}; +use rustc_session::{ + declare_lint, + declare_lint_pass, +}; +use std::collections::{ + btree_map::Entry, + BTreeMap, +}; + +declare_lint! { + /// **What it does:** + /// This lint ensures that for every storage field with a collection type, when there is an + /// operation to insert new elements, there's also an operation for removing elements. + /// + /// **Why is this bad?** + /// When a user executes a contract function that writes to storage, they have to put a + /// deposit down for the amount of storage space used. Whoever frees up that storage at some + /// later point gets the deposit back. Therefore, it is always a good idea to make it possible + /// for users to free up their storage space. + /// + /// **Example:** + /// + /// In the following example there is a storage field with the `Mapping` type that has an + /// function that inserts new elements: + /// + /// ```rust + /// #[ink(storage)] + /// pub struct Transaction { + /// values: Mapping, + /// } + /// + /// fn add_value(&mut self, k: &AccountId, v: &AccountId) { + /// // ... + /// self.values.insert(k, v); + /// // ... + /// } + /// ``` + /// + /// But, ideally, there also should be a function that allows the user to remove elements from + /// the Mapping freeing storage space: + /// + /// ```rust + /// fn del_value(&mut self, k: &AccountId) { + /// // ... + /// self.values.remove(k); + /// // ... + /// } + /// ``` + pub STORAGE_NEVER_FREED, + Allow, + "storage never freed" +} + +declare_lint_pass!(StorageNeverFreed => [STORAGE_NEVER_FREED]); + +mod methods { + // https://paritytech.github.io/ink/ink_prelude/vec/struct.Vec.html + pub const VEC_INSERT: [&str; 6] = [ + "append", + "extend_from_slice", + "extend_from_within", + "insert", + "push", + "push_with_capacity", + ]; + pub const VEC_REMOVE: [&str; 8] = [ + "clear", + "dedup", + "pop", + "remove", + "retain", + "retain_mut", + "swap_remove", + "truncate", + ]; + pub const VEC_IGNORE: [&str; 2] = ["as_mut_ptr", "as_mut_slice"]; + + // https://paritytech.github.io/ink/ink_storage/struct.Mapping.html + pub const MAP_INSERT: [&str; 1] = ["insert"]; + pub const MAP_REMOVE: [&str; 2] = ["remove", "take"]; +} + +enum CollectionTy { + Vec, + Map, +} + +/// Fields with collection types that should have both insert and remove operations +struct FieldInfo { + pub did: LocalDefId, + pub ty: CollectionTy, + pub has_insert: bool, + pub has_remove: bool, +} +type FieldName = String; +type FieldsMap = BTreeMap; + +impl FieldInfo { + pub fn new(did: LocalDefId, ty: CollectionTy) -> Self { + Self { + did, + ty, + has_insert: false, + has_remove: false, + } + } +} + +/// Finds `DefId` of the path that has the supported collection type +fn find_collection_def_id( + cx: &LateContext, + path: &Path, +) -> Option<(DefId, CollectionTy)> { + if_chain! { + if let Res::Def(DefKind::TyAlias, def_id) = path.res; + if let Some(local_id) = def_id.as_local(); + if let Some(alias_ty) = cx.tcx.hir().get_by_def_id(local_id).alias_ty(); + if let TyKind::Path(QPath::Resolved(_, path)) = alias_ty.kind; + then { return find_collection_def_id(cx, path); } + }; + if let Res::Def(DefKind::Struct, def_id) = path.res { + if match_def_path(cx, def_id, &["alloc", "vec", "Vec"]) { + return Some((def_id, CollectionTy::Vec)) + } + if match_def_path(cx, def_id, &["ink_storage", "lazy", "mapping", "Mapping"]) { + return Some((def_id, CollectionTy::Map)) + } + }; + None +} + +/// Returns vectors of fields that have collection types +fn find_collection_fields(cx: &LateContext, storage_struct_id: ItemId) -> FieldsMap { + let mut result = FieldsMap::new(); + let item = cx.tcx.hir().item(storage_struct_id); + if let ItemKind::Struct(var_data, _) = item.kind { + var_data.fields().iter().for_each(|field_def| { + if_chain! { + // Collection fields of the storage are expanded like this: + // vec_field: as ::ink::storage::traits::AutoStorableHint< + // ::ink::storage::traits::ManualKey<993959520u32, ()>, + // >>::Type, + if let TyKind::Path(QPath::Resolved(Some(ty), path)) = field_def.ty.kind; + if match_path(path, &["ink", "storage", "traits", "AutoStorableHint", "Type"]); + if let TyKind::Path(QPath::Resolved(None, path)) = ty.kind; + then { + let field_name = field_def.ident.name.as_str(); + if let Some((_, ty)) = find_collection_def_id(cx, path) { + result.insert(field_name.to_string(), FieldInfo::new(field_def.def_id, ty)); + } + } + } + }) + }; + result +} + +/// Reports the given field defintion +fn report_field(cx: &LateContext, field_info: &FieldInfo) { + if_chain! { + if let Node::Field(field) = cx.tcx.hir().get_by_def_id(field_info.did); + if !is_lint_allowed(cx, STORAGE_NEVER_FREED, field.hir_id); + then { + span_lint_and_help( + cx, + STORAGE_NEVER_FREED, + field.span, + "field's storage cannot be freed", + None, + "consider adding operations to remove elements available to the user" + ) + + } + } +} + +/// Visitor that collects `insert` and `remove` operations +struct InsertRemoveCollector<'a> { + fields: &'a mut FieldsMap, +} + +impl<'a> InsertRemoveCollector<'a> { + fn new(fields: &'a mut FieldsMap) -> Self { + Self { fields } + } + + /// Finds a field of the supported type in the given expression present with the form + /// `self.field_name` + fn find_field_name(&self, e: &Expr) -> Option { + if_chain! { + if let ExprKind::Field(s, field) = &e.kind; + if let ExprKind::Path(QPath::Resolved(_, path)) = s.kind; + if match_path(path, &["self"]); + then { Some(field.name.as_str().to_string()) } else { None } + } + } +} + +impl<'hir> Visitor<'hir> for InsertRemoveCollector<'_> { + fn visit_expr(&mut self, e: &'hir Expr<'hir>) { + match &e.kind { + ExprKind::Assign(lhs, ..) => { + if_chain! { + if let ExprKind::Index(field, _) = lhs.kind; + if let Some(field_name) = self.find_field_name(field); + then { + self.fields + .entry(field_name.to_string()) + .and_modify(|field_info| { + field_info.has_insert = true; + }); + } + } + } + ExprKind::MethodCall(method_path, receiver, args, _) => { + args.iter().for_each(|arg| walk_expr(self, arg)); + if_chain! { + if let Some(field_name) = self.find_field_name(receiver); + if let Entry::Occupied(mut e) = self.fields.entry(field_name.to_string()); + let method_name = method_path.ident.as_str(); + then { + let field_info = e.get_mut(); + match field_info.ty { + CollectionTy::Vec => { + if methods::VEC_IGNORE.contains(&method_name) { + e.remove(); + } else if methods::VEC_INSERT.contains(&method_name) { + field_info.has_insert = true; + } else if methods::VEC_REMOVE.contains(&method_name) { + field_info.has_remove = true; + } + }, + CollectionTy::Map => { + if methods::MAP_INSERT.contains(&method_name) { + field_info.has_insert = true; + } else if methods::MAP_REMOVE.contains(&method_name) { + field_info.has_remove = true; + } + } + } + } + } + } + _ => (), + } + walk_expr(self, e); + } +} + +impl<'tcx> LateLintPass<'tcx> for StorageNeverFreed { + fn check_mod( + &mut self, + cx: &LateContext<'tcx>, + m: &'tcx hir::Mod<'tcx>, + _: hir::HirId, + ) { + if_chain! { + // Find fields of Vec/Mapping type + if let Some(storage_struct_id) = find_storage_struct(cx, m.item_ids); + let mut fields = find_collection_fields(cx, storage_struct_id); + if !fields.is_empty(); + // Find all the user-defined functions of the contract + 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(_, fn_body_id) = impl_item.kind { + let mut visitor = InsertRemoveCollector::new(&mut fields); + walk_body(&mut visitor, cx.tcx.hir().body(fn_body_id)); + } + }); + fields.values().for_each(|field| { + if field.has_insert && !field.has_remove { + report_field(cx, field) + } + }) + } + } + } +} diff --git a/linting/ui/fail/storage_never_freed.rs b/linting/ui/fail/storage_never_freed.rs new file mode 100644 index 00000000000..d0fd1cdfa66 --- /dev/null +++ b/linting/ui/fail/storage_never_freed.rs @@ -0,0 +1,53 @@ +#![cfg_attr(dylint_lib = "ink_linting", deny(storage_never_freed))] +pub type MapAlias1 = ink::storage::Mapping; +pub type MapAlias2 = MapAlias1; + +#[ink::contract] +pub mod storage_never_freed { + use crate::MapAlias2; + use ink::storage::Mapping; + + #[ink(storage)] + pub struct StorageNeverFreed { + // All the fields generate warnings, since there are `insert` operations for + // them, but there are no `remove` operations. + vec_1: Vec, + vec_2: Vec, + vec_subscription: Vec, + map_1: Mapping, + map_2: Mapping, + map_3: Mapping, + map_alias: MapAlias2, + } + + impl StorageNeverFreed { + #[ink(constructor)] + pub fn new() -> Self { + Self { + vec_1: Vec::new(), + vec_2: Vec::new(), + vec_subscription: Vec::new(), + map_1: Mapping::new(), + map_2: Mapping::new(), + map_3: Mapping::new(), + map_alias: Mapping::new(), + } + } + + fn flip(a: bool) -> bool { + !a + } + + #[ink(message)] + pub fn add_to_fields(&mut self, v: AccountId) { + self.vec_1.push(v); + self.vec_subscription[0] = v; + self.map_1.insert(v, &v); + let _ = Self::flip(self.map_2.insert(v, &v).is_some()); + self.map_alias.insert(v, &v); + self.vec_2.push(self.map_3.insert(v, &v).is_some()); + } + } +} + +fn main() {} diff --git a/linting/ui/fail/storage_never_freed.stderr b/linting/ui/fail/storage_never_freed.stderr new file mode 100644 index 00000000000..33dc0b438d1 --- /dev/null +++ b/linting/ui/fail/storage_never_freed.stderr @@ -0,0 +1,63 @@ +error: field's storage cannot be freed + --> $DIR/storage_never_freed.rs:17:9 + | +LL | map_1: Mapping, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider adding operations to remove elements available to the user +note: the lint level is defined here + --> $DIR/storage_never_freed.rs:1:46 + | +LL | #![cfg_attr(dylint_lib = "ink_linting", deny(storage_never_freed))] + | ^^^^^^^^^^^^^^^^^^^ + +error: field's storage cannot be freed + --> $DIR/storage_never_freed.rs:18:9 + | +LL | map_2: Mapping, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider adding operations to remove elements available to the user + +error: field's storage cannot be freed + --> $DIR/storage_never_freed.rs:19:9 + | +LL | map_3: Mapping, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider adding operations to remove elements available to the user + +error: field's storage cannot be freed + --> $DIR/storage_never_freed.rs:20:9 + | +LL | map_alias: MapAlias2, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider adding operations to remove elements available to the user + +error: field's storage cannot be freed + --> $DIR/storage_never_freed.rs:14:9 + | +LL | vec_1: Vec, + | ^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider adding operations to remove elements available to the user + +error: field's storage cannot be freed + --> $DIR/storage_never_freed.rs:15:9 + | +LL | vec_2: Vec, + | ^^^^^^^^^^^^^^^^ + | + = help: consider adding operations to remove elements available to the user + +error: field's storage cannot be freed + --> $DIR/storage_never_freed.rs:16:9 + | +LL | vec_subscription: Vec, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider adding operations to remove elements available to the user + +error: aborting due to 7 previous errors + diff --git a/linting/ui/pass/storage_never_freed.rs b/linting/ui/pass/storage_never_freed.rs new file mode 100644 index 00000000000..aa14f2fd873 --- /dev/null +++ b/linting/ui/pass/storage_never_freed.rs @@ -0,0 +1,60 @@ +#![cfg_attr(dylint_lib = "ink_linting", deny(storage_never_freed))] +pub type MapAlias = ink::storage::Mapping; + +#[ink::contract] +pub mod storage_never_freed { + use crate::MapAlias; + use ink::storage::Mapping; + + #[ink(storage)] + pub struct StorageNeverFreed { + vec_1: Vec, + map_1: Mapping, + map_2: MapAlias, + #[cfg_attr(dylint_lib = "ink_linting", allow(storage_never_freed))] + map_field_suppressed: Mapping, + + // Vec which buffer was used unsafe operations with their raw pointers are not + // reported + vec_field_mut_pointer: Vec, + } + + impl StorageNeverFreed { + #[ink(constructor)] + pub fn new() -> Self { + Self { + vec_1: Vec::new(), + map_1: Mapping::new(), + map_2: Mapping::new(), + map_field_suppressed: Mapping::new(), + vec_field_mut_pointer: Vec::new(), + } + } + + #[ink(message)] + pub fn add_to_fields(&mut self, v: AccountId) { + self.vec_1.push(v); + self.map_1.insert(v, &v); + self.map_2.insert(v, &v); + self.map_field_suppressed.insert(v, &v); + + // Should not be reported, since elements may be removed using the pointer + self.vec_field_mut_pointer[0] = v; + unsafe { + let ptr = self.vec_field_mut_pointer.as_mut_ptr(); + let new_len = self.vec_field_mut_pointer.len() - 1; + std::ptr::copy(ptr.offset(1), ptr, new_len); + self.vec_field_mut_pointer.set_len(new_len); + } + } + + #[ink(message)] + pub fn remove_from_fields(&mut self, v: AccountId) { + self.vec_1.pop(); + self.map_1.remove(v); + self.map_2.remove(v); + } + } +} + +fn main() {}