phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-25 10:43:19 -03:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Merge bitcoin/bitcoin#23418: Fix signed integer overflow in prioritisetransaction RPC

Browse source 
fa07f84e31 Fix signed integer overflow in prioritisetransaction RPC (MarcoFalke)
fa52cf8e11 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 fa07f84e31
  dunxen:
    ACK fa07f84
  LarryRuane:
    ACK fa07f84e31

Tree-SHA512: 4a357950af55a49c9113da0a50c2e743c5b752f0514dd8d16cd92bfde2f77dd0ef56aa98452626df6f7f7a5b51d1227021f6bc94091201a179f0d488ee32a0df
This commit is contained in:
MacroFake 2022-06-27 08:25:13 +02:00
commit dde7205c57
No known key found for this signature in database
GPG key ID: CE2B75697E69A548
3 changed files with 20 additions and 16 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

23
src/txmempool.cpp
View file

@ -14,7 +14,9 @@
#include <policy/policy.h>
#include <policy/settings.h>
#include <reverse_iterator.h>
#include <util/check.h>
#include <util/moneystr.h>
#include <util/overflow.h>
#include <util/system.h>
#include <util/time.h>
#include <validationinterface.h>
@ -82,6 +84,7 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee,
entryHeight{entry_height},
spendsCoinbase{spends_coinbase},
sigOpCost{sigops_cost},
m_modified_fee{nFee},
lockPoints{lp},
nSizeWithDescendants{GetTxSize()},
nModFeesWithDescendants{nFee},
@ -89,11 +92,11 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee,
nModFeesWithAncestors{nFee},
nSigOpCostWithAncestors{sigOpCost} {}
void CTxMemPoolEntry::UpdateFeeDelta(CAmount newFeeDelta)
void CTxMemPoolEntry::UpdateModifiedFee(CAmount 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)
@ -435,7 +438,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);
}
@ -444,7 +447,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);
nSigOpCostWithAncestors += modifySigOps;
@ -483,8 +486,10 @@ 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, [&delta](CTxMemPoolEntry& e) { e.UpdateFeeDelta(delta); });
mapTx.modify(newit, [&delta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(delta); });
}
// Update cachedInnerUsage to include contained transaction's usage.
@ -917,10 +922,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, [&delta](CTxMemPoolEntry& e) { e.UpdateFeeDelta(delta); });
mapTx.modify(it, [&nFeeDelta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(nFeeDelta); });
// Now update all ancestors' modified fees with descendants
setEntries setAncestors;
uint64_t nNoLimit = std::numeric_limits<uint64_t>::max();

9
src/txmempool.h
View file

@ -101,7 +101,7 @@ private:
const unsigned int entryHeight; //!< Chain height when entering the mempool
const bool spendsCoinbase; //!< keep track of transactions that spend a coinbase
const int64_t sigOpCost; //!< Total sigop cost
CAmount feeDelta{0}; //!< Used for determining the priority of the transaction for mining in a block
CAmount 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
@ -131,7 +131,7 @@ public:
std::chrono::seconds GetTime() const { return std::chrono::seconds{nTime}; }
unsigned int GetHeight() const { return entryHeight; }
int64_t GetSigOpCost() const { return sigOpCost; }
CAmount GetModifiedFee() const { return nFee + feeDelta; }
CAmount GetModifiedFee() const { return m_modified_fee; }
size_t DynamicMemoryUsage() const { return nUsageSize; }
const LockPoints& GetLockPoints() const { return lockPoints; }
@ -139,9 +139,8 @@ public:
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/ancestors.
void UpdateFeeDelta(CAmount newFeeDelta);
// Updates the modified fees with descendants/ancestors.
void UpdateModifiedFee(CAmount fee_diff);
// Update the LockPoints after a reorg
void UpdateLockPoints(const LockPoints& lp);

4
test/sanitizer_suppressions/ubsan
View file

@ -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