From e8da48bd418b5783b527e9b64685fbbf10bf8fdf Mon Sep 17 00:00:00 2001 From: MacroFake Date: Mon, 27 Jun 2022 08:25:13 +0200 Subject: [PATCH 1/3] Merge bitcoin/bitcoin#23418: Fix signed integer overflow in prioritisetransaction RPC fa07f84e316171d60dd9941fb8db37e0a0de6654 Fix signed integer overflow in prioritisetransaction RPC (MarcoFalke) fa52cf8e11b3af6e0a302d5d17aab6cea78899d5 refactor: Replace feeDelta by m_modified_fee (MarcoFalke) Pull request description: Signed integer overflow is UB in theory, but not in practice. Still, it would be nice to avoid this UB to allow Bitcoin Core to be compiled with sanitizers such as `-ftrapv` or ubsan. It is impossible to predict when and if an overflow occurs, since the overflow caused by a prioritisetransaction RPC might only be later hit when descendant txs are added to the mempool. Since it is impossible to predict reliably, leave it up to the user to use the RPC endpoint responsibly, considering their mempool limits and usage patterns. Fixes: #20626 Fixes: #20383 Fixes: #19278 Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34146 / https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=47132 ## Steps to reproduce Build the code without the changes in this pull. Make sure to pass the sanitizer flag: ``` ./autogen.sh && ./configure --with-sanitizers=signed-integer-overflow && make clean && make -j $(nproc) ``` ### Reproduce on RPC ``` ./src/bitcoind -chain=regtest -noprinttoconsole & ./src/bitcoin-cli -chain=regtest prioritisetransaction 00000000deadbeef00000000deadbeef00000000deadbeef00000000deadbeef 0 9123456789123456789 ./src/bitcoin-cli -chain=regtest prioritisetransaction 00000000deadbeef00000000deadbeef00000000deadbeef00000000deadbeef 0 9123456789123456789 |> txmempool.cpp:920:15: runtime error: signed integer overflow: 9123456789123456789 + 9123456789123456789 cannot be represented in type 'long int' ./src/bitcoin-cli -chain=regtest stop ``` ### By fuzzing ``` wget https://github.com/bitcoin/bitcoin/files/8921302/clusterfuzz-testcase-minimized-validation_load_mempool-5599531390074880.bin.txt FUZZ=validation_load_mempool ./src/test/fuzz/fuzz ./clusterfuzz-testcase-minimized-validation_load_mempool-5599531390074880.bin.txt |> txmempool.cpp:920:15: runtime error: signed integer overflow: 7214801925397553184 + 2314885530818453536 cannot be represented in type 'long int' |> validation_load_mempool: succeeded against 1 files in 0s. ACKs for top commit: vasild: ACK fa07f84e316171d60dd9941fb8db37e0a0de6654 dunxen: ACK fa07f84 LarryRuane: ACK fa07f84e316171d60dd9941fb8db37e0a0de6654 Tree-SHA512: 4a357950af55a49c9113da0a50c2e743c5b752f0514dd8d16cd92bfde2f77dd0ef56aa98452626df6f7f7a5b51d1227021f6bc94091201a179f0d488ee32a0df --- src/txmempool.cpp | 22 +++++++++++++--------- src/txmempool.h | 9 ++++----- test/sanitizer_suppressions/ubsan | 4 ++-- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 10ae5fdfda795..f7c6d1372f6aa 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -66,7 +67,7 @@ struct update_fee_delta { explicit update_fee_delta(int64_t _feeDelta) : feeDelta(_feeDelta) { } - void operator() (CTxMemPoolEntry &e) { e.UpdateFeeDelta(feeDelta); } + void operator() (CTxMemPoolEntry &e) { e.UpdateModifiedFee(feeDelta); } private: int64_t feeDelta; @@ -100,6 +101,7 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee, entryHeight{entry_height}, spendsCoinbase{spends_coinbase}, sigOpCount{sigops_count}, + m_modified_fee{nFee}, lockPoints{lp}, nSizeWithDescendants{GetTxSize()}, nModFeesWithDescendants{nFee}, @@ -107,11 +109,11 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee, nModFeesWithAncestors{nFee}, nSigOpCountWithAncestors{sigOpCount} {} -void CTxMemPoolEntry::UpdateFeeDelta(int64_t newFeeDelta) +void CTxMemPoolEntry::UpdateModifiedFee(int64_t fee_diff) { - nModFeesWithDescendants += newFeeDelta - feeDelta; - nModFeesWithAncestors += newFeeDelta - feeDelta; - feeDelta = newFeeDelta; + nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, fee_diff); + nModFeesWithAncestors = SaturatingAdd(nModFeesWithAncestors, fee_diff); + m_modified_fee = SaturatingAdd(m_modified_fee, fee_diff); } void CTxMemPoolEntry::UpdateLockPoints(const LockPoints& lp) @@ -453,7 +455,7 @@ void CTxMemPoolEntry::UpdateDescendantState(int64_t modifySize, CAmount modifyFe { nSizeWithDescendants += modifySize; assert(int64_t(nSizeWithDescendants) > 0); - nModFeesWithDescendants += modifyFee; + nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, modifyFee); nCountWithDescendants += modifyCount; assert(int64_t(nCountWithDescendants) > 0); } @@ -462,7 +464,7 @@ void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee, { nSizeWithAncestors += modifySize; assert(int64_t(nSizeWithAncestors) > 0); - nModFeesWithAncestors += modifyFee; + nModFeesWithAncestors = SaturatingAdd(nModFeesWithAncestors, modifyFee); nCountWithAncestors += modifyCount; assert(int64_t(nCountWithAncestors) > 0); nSigOpCountWithAncestors += modifySigOps; @@ -510,6 +512,8 @@ void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAnces // Update transaction for any feeDelta created by PrioritiseTransaction CAmount delta{0}; ApplyDelta(entry.GetTx().GetHash(), delta); + // The following call to UpdateModifiedFee assumes no previous fee modifications + Assume(entry.GetFee() == entry.GetModifiedFee()); if (delta) { mapTx.modify(newit, update_fee_delta(delta)); } @@ -1458,10 +1462,10 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD { LOCK(cs); CAmount &delta = mapDeltas[hash]; - delta += nFeeDelta; + delta = SaturatingAdd(delta, nFeeDelta); txiter it = mapTx.find(hash); if (it != mapTx.end()) { - mapTx.modify(it, update_fee_delta(delta)); + mapTx.modify(it, update_fee_delta(nFeeDelta)); // Now update all ancestors' modified fees with descendants setEntries setAncestors; uint64_t nNoLimit = std::numeric_limits::max(); diff --git a/src/txmempool.h b/src/txmempool.h index 77da3a0386882..139a35887ecb8 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -113,7 +113,7 @@ class CTxMemPoolEntry const unsigned int entryHeight; //!< Chain height when entering the mempool const bool spendsCoinbase; //!< keep track of transactions that spend a coinbase const int64_t sigOpCount; //!< Legacy sig ops plus P2SH sig op count - int64_t feeDelta{0}; //!< Used for determining the priority of the transaction for mining in a block + int64_t m_modified_fee; //!< Used for determining the priority of the transaction for mining in a block LockPoints lockPoints; //!< Track the height and time at which tx was final // Information about descendants of this transaction that are in the @@ -142,7 +142,7 @@ class CTxMemPoolEntry std::chrono::seconds GetTime() const { return std::chrono::seconds{nTime}; } unsigned int GetHeight() const { return entryHeight; } int64_t GetSigOpCount() const { return sigOpCount; } - int64_t GetModifiedFee() const { return nFee + feeDelta; } + int64_t GetModifiedFee() const { return m_modified_fee; } size_t DynamicMemoryUsage() const { return nUsageSize; } const LockPoints& GetLockPoints() const { return lockPoints; } @@ -150,9 +150,8 @@ class CTxMemPoolEntry void UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount); // Adjusts the ancestor state void UpdateAncestorState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps); - // Updates the fee delta used for mining priority score, and the - // modified fees with descendants. - void UpdateFeeDelta(int64_t feeDelta); + // Updates the modified fees with descendants/ancestors. + void UpdateModifiedFee(int64_t fee_diff); // Update the LockPoints after a reorg void UpdateLockPoints(const LockPoints& lp); diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index ba08cef801a9b..147d0d78b39c2 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -1,10 +1,10 @@ # -fsanitize=undefined suppressions # ================================= -# This would be `signed-integer-overflow:CTxMemPool::PrioritiseTransaction`, +# The suppressions would be `sanitize-type:ClassName::MethodName`, # however due to a bug in clang the symbolizer is disabled and thus no symbol # names can be used. # See https://github.com/google/sanitizers/issues/1364 -signed-integer-overflow:txmempool.cpp + # https://github.com/bitcoin/bitcoin/pull/21798#issuecomment-829180719 signed-integer-overflow:policy/feerate.cpp From b3919ca6245755793abf10c1c07abc5aa12c8a96 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 28 Jun 2022 18:18:06 +0100 Subject: [PATCH 2/3] Merge bitcoin/bitcoin#25480: Replace CountSecondsDouble with Ticks fa956e7508986991008e2f6126ab307924b3f353 Replace CountSecondsDouble with Ticks (MacroFake) Pull request description: Seems odd to have two ways to say exactly the same thing when one is sufficient. ACKs for top commit: fanquake: ACK fa956e7508986991008e2f6126ab307924b3f353 shaavan: ACK fa956e7508986991008e2f6126ab307924b3f353 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25480/commits/fa956e7508986991008e2f6126ab307924b3f353 Tree-SHA512: b599470e19b693da1ed1102d1e86b08cb03adaddf2048752b6d050fdf86055be117ff0ae10b6953d03e00eaaf7b0cfa350137968b67d6c5b3ca68c5aa50ca6aa --- src/net_processing.cpp | 2 +- src/rpc/net.cpp | 6 +++--- src/util/time.h | 5 ----- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 7e0c768b33c5c..78f6227d68078 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3986,7 +3986,7 @@ void PeerManagerImpl::ProcessMessage( if (peer->m_addr_token_bucket < MAX_ADDR_PROCESSING_TOKEN_BUCKET) { // Don't increment bucket if it's already full const auto time_diff = std::max(current_time - peer->m_addr_token_timestamp, 0us); - const double increment = CountSecondsDouble(time_diff) * MAX_ADDR_RATE_PER_SECOND; + const double increment = Ticks(time_diff) * MAX_ADDR_RATE_PER_SECOND; peer->m_addr_token_bucket = std::min(peer->m_addr_token_bucket + increment, MAX_ADDR_PROCESSING_TOKEN_BUCKET); } peer->m_addr_token_timestamp = current_time; diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 5a67baecab6f2..21215aef838eb 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -222,13 +222,13 @@ static RPCHelpMan getpeerinfo() obj.pushKV("conntime", count_seconds(stats.m_connected)); obj.pushKV("timeoffset", stats.nTimeOffset); if (stats.m_last_ping_time > 0us) { - obj.pushKV("pingtime", CountSecondsDouble(stats.m_last_ping_time)); + obj.pushKV("pingtime", Ticks(stats.m_last_ping_time)); } if (stats.m_min_ping_time < std::chrono::microseconds::max()) { - obj.pushKV("minping", CountSecondsDouble(stats.m_min_ping_time)); + obj.pushKV("minping", Ticks(stats.m_min_ping_time)); } if (fStateStats && statestats.m_ping_wait > 0s) { - obj.pushKV("pingwait", CountSecondsDouble(statestats.m_ping_wait)); + obj.pushKV("pingwait", Ticks(statestats.m_ping_wait)); } obj.pushKV("version", stats.nVersion); // Use the sanitized form of subver here, to avoid tricksy remote peers from diff --git a/src/util/time.h b/src/util/time.h index 32bdebb205066..762262858b64f 100644 --- a/src/util/time.h +++ b/src/util/time.h @@ -58,11 +58,6 @@ constexpr int64_t count_microseconds(std::chrono::microseconds t) { return t.cou using HoursDouble = std::chrono::duration; using SecondsDouble = std::chrono::duration; -/** - * Helper to count the seconds in any std::chrono::duration type - */ -inline double CountSecondsDouble(SecondsDouble t) { return t.count(); } - /** * DEPRECATED * Use either ClockType::now() or Now() if a cast is needed. From 140622fb17f41cdfa1595e036696b41ff3c6d6f8 Mon Sep 17 00:00:00 2001 From: MacroFake Date: Wed, 29 Jun 2022 12:35:27 +0200 Subject: [PATCH 3/3] Merge bitcoin/bitcoin#25492: util: remove MSVC warning pragmas d8f8f7812cfa50e853d30f53273601dca57a60ec util: remove MSVC warning pragmas (fanquake) Pull request description: 4786 - I don't think this exists any more? 4805 - Is already defined (globally) there. Dropped 4717 and 4804, as it seems they are no-longer supressing anything. See: https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warnings-c4000-c5999. ACKs for top commit: hebasto: ACK d8f8f7812cfa50e853d30f53273601dca57a60ec, build [log](https://api.cirrus-ci.com/v1/task/6088784285532160/logs/build.log) is free of warnings. Tree-SHA512: c8ac4585799996960ea099b2c5337e7bb577152eec2e9543cc459c56f42f7a36fc4dcd7faec2fa4ac159a4ae27859650ccfd96bbf94b94dbd1cbea638560a24f --- src/util/system.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index be227c073fa4a..b5ae435c94a2c 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -54,13 +54,6 @@ #else -#ifdef _MSC_VER -#pragma warning(disable:4786) -#pragma warning(disable:4804) -#pragma warning(disable:4805) -#pragma warning(disable:4717) -#endif - #include #include /* for _commit */