diff options
author | fanquake <fanquake@gmail.com> | 2024-04-08 17:35:09 +0200 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2024-04-08 17:35:15 +0200 |
commit | eaf186d1ee3a0271ec8cad65fa294b4298d79b0a (patch) | |
tree | a362cb2801ffdac4d39d4b719fafb8070899c203 | |
parent | 3206e45412ded0e70c1f15ba66c2ba3b4426f27f (diff) | |
parent | 78407b99ed6dd17f687fcbfb0486ecc433302287 (diff) | |
download | bitcoin-eaf186d1ee3a0271ec8cad65fa294b4298d79b0a.tar.xz |
Merge bitcoin/bitcoin#29690: clang-tidy: Enable misc-no-recursion
78407b99ed6dd17f687fcbfb0486ecc433302287 [clang-tidy] Enable the misc-no-recursion check (dergoegge)
Pull request description:
Recursion is a frequent source of stack overflow bugs. Secondly, introduction of recursion can be non-obvious.
This PR proposes to use the clang-tidy `misc-no-recursion` check to make introduction of new recursion obvious. We don't make use of recursion a lot in our code base but there are a few places that need suppressions anyway (mostly the descriptor and univalue/rpc code).
ACKs for top commit:
stickies-v:
ACK 78407b99ed6dd17f687fcbfb0486ecc433302287
TheCharlatan:
Re-ACK 78407b99ed6dd17f687fcbfb0486ecc433302287
fanquake:
ACK 78407b99ed6dd17f687fcbfb0486ecc433302287
Tree-SHA512: 34126d704c46086fe7371906ca852c25ced1dbd5fcfd85bf623810cd171a797569a92a33c7e26b8dc01c30c7bbf81aa326718926e8354585091411989a4edb14
-rw-r--r-- | doc/developer-notes.md | 2 | ||||
-rw-r--r-- | src/.clang-tidy | 1 | ||||
-rw-r--r-- | src/merkleblock.cpp | 3 | ||||
-rw-r--r-- | src/node/interfaces.cpp | 1 | ||||
-rw-r--r-- | src/qt/optionsmodel.cpp | 2 | ||||
-rw-r--r-- | src/rpc/util.cpp | 6 | ||||
-rw-r--r-- | src/rpc/util.h | 2 | ||||
-rw-r--r-- | src/script/descriptor.cpp | 9 | ||||
-rw-r--r-- | src/test/miniscript_tests.cpp | 1 | ||||
-rw-r--r-- | src/test/validation_block_tests.cpp | 1 | ||||
-rw-r--r-- | src/univalue/include/univalue.h | 1 | ||||
-rw-r--r-- | src/univalue/lib/univalue_write.cpp | 3 | ||||
-rw-r--r-- | src/util/string.h | 1 | ||||
-rw-r--r-- | src/wallet/receive.cpp | 1 | ||||
-rw-r--r-- | src/wallet/rpc/addresses.cpp | 3 | ||||
-rw-r--r-- | src/wallet/rpc/backup.cpp | 1 | ||||
-rw-r--r-- | src/wallet/scriptpubkeyman.cpp | 1 |
17 files changed, 39 insertions, 0 deletions
diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 89c13600eb..cc3f0518e5 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -115,6 +115,8 @@ code. Use `reinterpret_cast` and `const_cast` as appropriate. - Prefer [`list initialization ({})`](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-list) where possible. For example `int x{0};` instead of `int x = 0;` or `int x(0);` + - Recursion is checked by clang-tidy and thus must be made explicit. Use + `NOLINTNEXTLINE(misc-no-recursion)` to suppress the check. For function calls a namespace should be specified explicitly, unless such functions have been declared within it. Otherwise, [argument-dependent lookup](https://en.cppreference.com/w/cpp/language/adl), also known as ADL, could be diff --git a/src/.clang-tidy b/src/.clang-tidy index e4b789dcaa..a00400f083 100644 --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -6,6 +6,7 @@ bugprone-string-constructor, bugprone-use-after-move, bugprone-lambda-function-name, misc-unused-using-decls, +misc-no-recursion, modernize-use-default-member-init, modernize-use-emplace, modernize-use-noexcept, diff --git a/src/merkleblock.cpp b/src/merkleblock.cpp index c75f5c5e60..669c6e3b70 100644 --- a/src/merkleblock.cpp +++ b/src/merkleblock.cpp @@ -54,6 +54,7 @@ CMerkleBlock::CMerkleBlock(const CBlock& block, CBloomFilter* filter, const std: txn = CPartialMerkleTree(vHashes, vMatch); } +// NOLINTNEXTLINE(misc-no-recursion) uint256 CPartialMerkleTree::CalcHash(int height, unsigned int pos, const std::vector<uint256> &vTxid) { //we can never have zero txs in a merkle block, we always need the coinbase tx //if we do not have this assert, we can hit a memory access violation when indexing into vTxid @@ -74,6 +75,7 @@ uint256 CPartialMerkleTree::CalcHash(int height, unsigned int pos, const std::ve } } +// NOLINTNEXTLINE(misc-no-recursion) void CPartialMerkleTree::TraverseAndBuild(int height, unsigned int pos, const std::vector<uint256> &vTxid, const std::vector<bool> &vMatch) { // determine whether this node is the parent of at least one matched txid bool fParentOfMatch = false; @@ -92,6 +94,7 @@ void CPartialMerkleTree::TraverseAndBuild(int height, unsigned int pos, const st } } +// NOLINTNEXTLINE(misc-no-recursion) uint256 CPartialMerkleTree::TraverseAndExtract(int height, unsigned int pos, unsigned int &nBitsUsed, unsigned int &nHashUsed, std::vector<uint256> &vMatch, std::vector<unsigned int> &vnIndex) { if (nBitsUsed >= vBits.size()) { // overflowed the bits array - failure diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index f9a372e3de..4d2d83812e 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -406,6 +406,7 @@ public: NodeContext* m_context{nullptr}; }; +// NOLINTNEXTLINE(misc-no-recursion) bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<RecursiveMutex>& lock, const CChain& active, const BlockManager& blockman) { if (!index) return false; diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index d816a72ca3..d0f7c64357 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -396,6 +396,7 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in return successful; } +// NOLINTNEXTLINE(misc-no-recursion) QVariant OptionsModel::getOption(OptionID option, const std::string& suffix) const { auto setting = [&]{ return node().getPersistentSetting(SettingName(option) + suffix); }; @@ -508,6 +509,7 @@ QFont OptionsModel::getFontForMoney() const return getFontForChoice(m_font_money); } +// NOLINTNEXTLINE(misc-no-recursion) bool OptionsModel::setOption(OptionID option, const QVariant& value, const std::string& suffix) { auto changed = [&] { return value.isValid() && value != getOption(option, suffix); }; diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 51c88cc1ba..6e332e3855 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -414,6 +414,7 @@ struct Sections { /** * Recursive helper to translate an RPCArg into sections */ + // NOLINTNEXTLINE(misc-no-recursion) void Push(const RPCArg& arg, const size_t current_indent = 5, const OuterType outer_type = OuterType::NONE) { const auto indent = std::string(current_indent, ' '); @@ -953,6 +954,7 @@ std::string RPCArg::ToDescriptionString(bool is_named_arg) const return ret; } +// NOLINTNEXTLINE(misc-no-recursion) void RPCResult::ToSections(Sections& sections, const OuterType outer_type, const int current_indent) const { // Indentation @@ -1086,6 +1088,7 @@ static std::optional<UniValue::VType> ExpectedType(RPCResult::Type type) NONFATAL_UNREACHABLE(); } +// NOLINTNEXTLINE(misc-no-recursion) UniValue RPCResult::MatchesType(const UniValue& result) const { if (m_skip_type_check) { @@ -1164,6 +1167,7 @@ void RPCResult::CheckInnerDoc() const CHECK_NONFATAL(inner_needed != m_inner.empty()); } +// NOLINTNEXTLINE(misc-no-recursion) std::string RPCArg::ToStringObj(const bool oneline) const { std::string res; @@ -1202,6 +1206,7 @@ std::string RPCArg::ToStringObj(const bool oneline) const NONFATAL_UNREACHABLE(); } +// NOLINTNEXTLINE(misc-no-recursion) std::string RPCArg::ToString(const bool oneline) const { if (oneline && !m_opts.oneline_description.empty()) { @@ -1228,6 +1233,7 @@ std::string RPCArg::ToString(const bool oneline) const case Type::OBJ: case Type::OBJ_NAMED_PARAMS: case Type::OBJ_USER_KEYS: { + // NOLINTNEXTLINE(misc-no-recursion) const std::string res = Join(m_inner, ",", [&](const RPCArg& i) { return i.ToStringObj(oneline); }); if (m_type == Type::OBJ) { return "{" + res + "}"; diff --git a/src/rpc/util.h b/src/rpc/util.h index ad3ed97b2e..f6ee6a317a 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -162,6 +162,7 @@ struct RPCArgOptions { //!< methods set the also_positional flag and read values from both positions. }; +// NOLINTNEXTLINE(misc-no-recursion) struct RPCArg { enum class Type { OBJ, @@ -271,6 +272,7 @@ struct RPCArg { std::string ToDescriptionString(bool is_named_arg) const; }; +// NOLINTNEXTLINE(misc-no-recursion) struct RPCResult { enum class Type { OBJ, diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index a0e755afac..a11d4dcbd5 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -599,6 +599,7 @@ public: COMPAT, // string calculation that mustn't change over time to stay compatible with previous software versions }; + // NOLINTNEXTLINE(misc-no-recursion) bool IsSolvable() const override { for (const auto& arg : m_subdescriptor_args) { @@ -607,6 +608,7 @@ public: return true; } + // NOLINTNEXTLINE(misc-no-recursion) bool IsRange() const final { for (const auto& pubkey : m_pubkey_args) { @@ -618,6 +620,7 @@ public: return false; } + // NOLINTNEXTLINE(misc-no-recursion) virtual bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, const DescriptorCache* cache = nullptr) const { size_t pos = 0; @@ -630,6 +633,7 @@ public: return true; } + // NOLINTNEXTLINE(misc-no-recursion) virtual bool ToStringHelper(const SigningProvider* arg, std::string& out, const StringType type, const DescriptorCache* cache = nullptr) const { std::string extra = ToStringExtra(); @@ -682,6 +686,7 @@ public: return ret; } + // NOLINTNEXTLINE(misc-no-recursion) bool ExpandHelper(int pos, const SigningProvider& arg, const DescriptorCache* read_cache, std::vector<CScript>& output_scripts, FlatSigningProvider& out, DescriptorCache* write_cache) const { std::vector<std::pair<CPubKey, KeyOriginInfo>> entries; @@ -723,6 +728,7 @@ public: return ExpandHelper(pos, DUMMY_SIGNING_PROVIDER, &read_cache, output_scripts, out, nullptr); } + // NOLINTNEXTLINE(misc-no-recursion) void ExpandPrivate(int pos, const SigningProvider& provider, FlatSigningProvider& out) const final { for (const auto& p : m_pubkey_args) { @@ -750,6 +756,7 @@ public: std::optional<int64_t> MaxSatisfactionElems() const override { return {}; } + // NOLINTNEXTLINE(misc-no-recursion) void GetPubKeys(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const override { for (const auto& p : m_pubkey_args) { @@ -1579,6 +1586,7 @@ struct KeyParser { }; /** Parse a script in a particular context. */ +// NOLINTNEXTLINE(misc-no-recursion) std::unique_ptr<DescriptorImpl> ParseScript(uint32_t& key_exp_index, Span<const char>& sp, ParseScriptContext ctx, FlatSigningProvider& out, std::string& error) { using namespace spanparsing; @@ -1886,6 +1894,7 @@ std::unique_ptr<DescriptorImpl> InferMultiA(const CScript& script, ParseScriptCo return std::make_unique<MultiADescriptor>(match->first, std::move(keys)); } +// NOLINTNEXTLINE(misc-no-recursion) std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptContext ctx, const SigningProvider& provider) { if (ctx == ParseScriptContext::P2TR && script.size() == 34 && script[0] == 32 && script[33] == OP_CHECKSIG) { diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp index 1f28e61bc9..a3699f84b6 100644 --- a/src/test/miniscript_tests.cpp +++ b/src/test/miniscript_tests.cpp @@ -297,6 +297,7 @@ using miniscript::operator"" _mst; using Node = miniscript::Node<CPubKey>; /** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */ +// NOLINTNEXTLINE(misc-no-recursion) std::set<Challenge> FindChallenges(const NodeRef& ref) { std::set<Challenge> chal; for (const auto& key : ref->keys) { diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index 316ab86c2b..69f4e305ab 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -127,6 +127,7 @@ std::shared_ptr<const CBlock> MinerTestingSetup::BadBlock(const uint256& prev_ha return ret; } +// NOLINTNEXTLINE(misc-no-recursion) void MinerTestingSetup::BuildChain(const uint256& root, int height, const unsigned int invalid_rate, const unsigned int branch_rate, const unsigned int max_size, std::vector<std::shared_ptr<const CBlock>>& blocks) { if (height <= 0 || blocks.size() >= max_size) return; diff --git a/src/univalue/include/univalue.h b/src/univalue/include/univalue.h index acbce25741..da12157555 100644 --- a/src/univalue/include/univalue.h +++ b/src/univalue/include/univalue.h @@ -18,6 +18,7 @@ #include <utility> #include <vector> +// NOLINTNEXTLINE(misc-no-recursion) class UniValue { public: enum VType { VNULL, VOBJ, VARR, VSTR, VNUM, VBOOL, }; diff --git a/src/univalue/lib/univalue_write.cpp b/src/univalue/lib/univalue_write.cpp index 4a3cbba20f..4a2219061a 100644 --- a/src/univalue/lib/univalue_write.cpp +++ b/src/univalue/lib/univalue_write.cpp @@ -27,6 +27,7 @@ static std::string json_escape(const std::string& inS) return outS; } +// NOLINTNEXTLINE(misc-no-recursion) std::string UniValue::write(unsigned int prettyIndent, unsigned int indentLevel) const { @@ -66,6 +67,7 @@ static void indentStr(unsigned int prettyIndent, unsigned int indentLevel, std:: s.append(prettyIndent * indentLevel, ' '); } +// NOLINTNEXTLINE(misc-no-recursion) void UniValue::writeArray(unsigned int prettyIndent, unsigned int indentLevel, std::string& s) const { s += "["; @@ -88,6 +90,7 @@ void UniValue::writeArray(unsigned int prettyIndent, unsigned int indentLevel, s s += "]"; } +// NOLINTNEXTLINE(misc-no-recursion) void UniValue::writeObject(unsigned int prettyIndent, unsigned int indentLevel, std::string& s) const { s += "{"; diff --git a/src/util/string.h b/src/util/string.h index 8b69d6ccae..dab92942fb 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -65,6 +65,7 @@ void ReplaceAll(std::string& in_out, const std::string& search, const std::strin * @param unary_op Apply this operator to each item */ template <typename C, typename S, typename UnaryOp> +// NOLINTNEXTLINE(misc-no-recursion) auto Join(const C& container, const S& separator, UnaryOp unary_op) { decltype(unary_op(*container.begin())) ret; diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp index ea3ffff549..c164266f80 100644 --- a/src/wallet/receive.cpp +++ b/src/wallet/receive.cpp @@ -253,6 +253,7 @@ bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminef return (CachedTxGetDebit(wallet, wtx, filter) > 0); } +// NOLINTNEXTLINE(misc-no-recursion) bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<uint256>& trusted_parents) { AssertLockHeld(wallet.cs_wallet); diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index acaa2d8b15..7f068c05ef 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -395,6 +395,7 @@ class DescribeWalletAddressVisitor public: const SigningProvider * const provider; + // NOLINTNEXTLINE(misc-no-recursion) void ProcessSubScript(const CScript& subscript, UniValue& obj) const { // Always present: script type and redeemscript @@ -445,6 +446,7 @@ public: return obj; } + // NOLINTNEXTLINE(misc-no-recursion) UniValue operator()(const ScriptHash& scripthash) const { UniValue obj(UniValue::VOBJ); @@ -465,6 +467,7 @@ public: return obj; } + // NOLINTNEXTLINE(misc-no-recursion) UniValue operator()(const WitnessV0ScriptHash& id) const { UniValue obj(UniValue::VOBJ); diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 5167e986b1..ae2dfe5795 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -854,6 +854,7 @@ enum class ScriptContext // Analyse the provided scriptPubKey, determining which keys and which redeem scripts from the ImportData struct are needed to spend it, and mark them as used. // Returns an error string, or the empty string for success. +// NOLINTNEXTLINE(misc-no-recursion) static std::string RecurseImportData(const CScript& script, ImportData& import_data, const ScriptContext script_ctx) { // Use Solver to obtain script type and parsed pubkeys or hashes: diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 59171f6db7..b42275fe4b 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -97,6 +97,7 @@ bool HaveKeys(const std::vector<valtype>& pubkeys, const LegacyScriptPubKeyMan& //! @param recurse_scripthash whether to recurse into nested p2sh and p2wsh //! scripts or simply treat any script that has been //! stored in the keystore as spendable +// NOLINTNEXTLINE(misc-no-recursion) IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true) { IsMineResult ret = IsMineResult::NO; |