From c542cd3a98ff142075759d33b3bb49bb19117fe5 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 3 Apr 2020 06:50:10 +0800 Subject: [PATCH] Merge #18383: refactor: Check for overflow when calculating sum of tx outputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80 Check for overflow when calculating sum of outputs (Elichai Turkel) Pull request description: This was reported by practicalswift here #18046 The exact order of the if, is important, we first do `!MoneyRange(tx_out.nValue)` to make sure the amount is non-negative. and then `std::numeric_limits::max() - tx_out.nValue < nValueOut` checks that the addition cannot overflow (if we won't check that the amount is positive this check can also overflow! (by doing something like `max - -max`)) and only then we make sure that the some is also valid `!MoneyRange(nValueOut + tx_out.nValue)` if any of these conditions fail we throw. the overflowing logic: ``` a + b > max // we want to fail if a+b is more than the maximum -> will overflow b > max - a max - a < b ``` Closes: #18046 ACKs for top commit: MarcoFalke: ACK f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80, checked that clang with O2 produces identical binaries 💕 practicalswift: ACK f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80 instagibbs: utACK https://github.com/bitcoin/bitcoin/pull/18383/commits/f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80 vasild: ACK f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80 modulo `s/assert.h/cassert/` Tree-SHA512: 512d6cf4762f24c41cf9a38da486b17b19c634fa3f4efbdebfe6608779e96fc3014d5d2d29adb8001e113152c0217bbd5b3900ac4edc7b8abe77f82f36209e33 --- src/primitives/transaction.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index 03221d7d88..f63b36eb75 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -9,6 +9,8 @@ #include #include +#include + std::string COutPoint::ToString() const { return strprintf("COutPoint(%s, %u)", hash.ToString()/*.substr(0,10)*/, n); @@ -99,10 +101,11 @@ CAmount CTransaction::GetValueOut() const { CAmount nValueOut = 0; for (const auto& tx_out : vout) { - nValueOut += tx_out.nValue; - if (!MoneyRange(tx_out.nValue) || !MoneyRange(nValueOut)) + if (!MoneyRange(tx_out.nValue) || !MoneyRange(nValueOut + tx_out.nValue)) throw std::runtime_error(std::string(__func__) + ": value out of range"); + nValueOut += tx_out.nValue; } + assert(MoneyRange(nValueOut)); return nValueOut; }