Linter: Warn about strict balance equality #1914
Conversation
The current implementation works only with intraprocedural MIR and does not support taint propagation across function calls. Closes use-ink#1811
Previously, diagnostics did not work, since `terminator.span` is resulted after macro expansion
This is required to implement interprocedural analysis
|
Can you please add a changelog entry? |
|
@SkymanOne Added. I wasn't sure about this before because we didn't include any information about the linter in the previous release, and it doesn't contain any documentation yet. Should we consider adding information about the available lints to the documentation and/or changelog before the next release? |
|
It is still important to include the entry in the changelog for traceability. Lints are already enabled by default in cargo contract, and the documentation will follow |
It reuses functions implemented in use-ink#1914, so we need to rebase and modify it when it is merged.
* feat(lint+test): add skeleton of storage_never_freed (#1431) * feat(lint): add utils and logic to collect `Vec`/`Mapping` types It reuses functions implemented in #1914, so we need to rebase and modify it when it is merged. * feat(lint): Make it pass simple tests * feat(lint): Support index operations (`vec[]`) * feat(lint): Ignore fields that has mutable unsafe pointers * chore(lint): Refactor * feat(test): Inserting in method arg * chore(test): Refactor * feat(test): Insert inside insert * chore(lint): Refactor * feat(lint): Support local type aliases * chore(lint): More accurate warning text * chore(lint): Refactor * feat(lint): Additional check for `self` * chore(lint+tests): Refactor; update tests output * chore: Update changelog * chore: fmt
|
@SkymanOne Thanks for the review. |
|
@jubnzv Regarding false positives:
How does the error message look if those are used? Do you see any way of getting rid of those false positives? |
|
@cmichi These cases are false negatives. It means the linter doesn't raise any warnings on them, but it should ideally. I intentionally chose to skip them to simplify and optimize the implementation, as these cases probably won't occur in real production smart contracts. |
🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑These are the results when building the
Link to the run | Last update: Wed Oct 25 15:19:00 CEST 2023 |
|
Thanks for the review @cmichi |
Summary
Closes #1811
cargo-contractorpallet-contracts?This PR adds the lint that warns about strict balance equality to the
lintingmodule.Description
The lint is implemented as a forward dataflow problem using MIR dataflow framework. The goal of the analysis is to propagate values tainted with
self.env().balance()throughout the MIR, handling all possible MIR statements along the way, including propagation through different kinds of references and across contract function calls. Then the tainted values are reported when they appear in conditions.The implementation of the traits needed for MIR dataflow framework includes the definitions of the transfer function and the lattice that keeps state during the analysis. I created a struct
TransferFunctionthat encapsulates all the logic related to visiting MIR, as it is done in the compiler itself and usedBitSet<Local>as a lattice, which is powerset: inclusion means that the value is tainted with balance, exclusion that it is not.MIR dataflow framework works only with bodies of single functions. Therefore, we have to implement interprocedural analysis to propagate tainted values across function calls. This analysis was included in the lint itself and includes a cache (see:
VisitedFunctionsCache) that stores the final states after executing the analysis on functions defined in user code.For more information about MIR dataflow, I found the source code of rustc to be useful, as it provides several examples of MIR dataflow. Additionally, I found the following thesis to be helpful because it offers a comprehensive description of the rustc internals involved in dataflow analysis.
Current status
It works on my tests. I'd like to additionally try it on the real-world smart contracts from the ecosystem to make sure I don't miss something.
There are a few cases that are not handled with this lint, therefore it will generate false negatives:
let env = self.env(); let balance = env.balance(); /* use balance */self.env().balance()to contract fieldsPartialEq::{eq, ne}are supported.I intentionally skipped these, since it seems not very realistic to appear in the production code. Please let me know, if there are any cases, including these, that should be included in the lint.
Checklist before requesting a review
CHANGELOG.md