util: introduce `TrySub` to prevent unsigned underflow
What the developer wrote
Authored by Lőrinc
util: introduce `TrySub` to prevent unsigned underflow
Introduce `TrySub(T&, U)` which subtracts an unsigned integral `U` from an unsigned integral `T`, returning `false` on underflow.
Use with `Assume(TrySub(...))` at coins cache accounting decrement sites so invariant violations fail immediately rather than silently wrapping.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Pieter Wuille <pieter@wuille.net>
What changed, and why it matters
This change adds a safety check to subtraction operations in Bitcoin Core's coin-cache accounting. Previously, certain internal counters could silently wrap around to huge values if a bug caused them to be decremented more than they were incremented. Now the program will detect that impossible condition and abort, turning a silent accounting corruption into a visible failure. The patch is defensive hardening; it does not by itself prove an attacker can trigger the underflow.
Treat as a defensive hardening commit. Review whether any of the changed decrement sites are reachable from untrusted network input, and consider whether `Assume()` is the appropriate failure policy for each site (non-fatal in release builds may still permit continued operation with corrupted accounting). No immediate emergency response is indicated by the diff alone.
Security signals we found
Unsigned integer underflow hardening in memory/dirty-coin accounting
Conversion of silent wraparound to assertion-triggering failure
Defensive invariant enforcement in UTXO cache code
No new externally reachable input validation added
Evidence from the diff
The commit introduces TrySub(T&, U), a constexpr helper in src/util/overflow.h that subtracts an unsigned integral from another unsigned integral and returns false on underflow without modifying the operand. It then wraps existing decrement sites for m_dirty_count and cachedCoinsUsage in src/coins.cpp and src/coins.h with Assume(TrySub(...)). In Bitcoin Core, Assume() is a non-fatal assertion in release builds but can abort in debug/assume builds, so invariant violations now fail loudly instead of silently wrapping. The change is a hardening patch; it does not add new bounds checks on external inputs and does not demonstrate a reachable underflow path.
Changed components
src/util/overflow.hsrc/coins.cppsrc/coins.hCCoinsViewCacheCoinsViewCacheCursorInspect captured patch +18 / −9
diff --git a/src/coins.cpp b/src/coins.cpp
index a6552283..25b1ead0 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -113,8 +113,8 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
fresh = !it->second.IsDirty();
}
if (!inserted) {
- m_dirty_count -= it->second.IsDirty();
- cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
+ Assume(TrySub(m_dirty_count, it->second.IsDirty()));
+ Assume(TrySub(cachedCoinsUsage, it->second.coin.DynamicMemoryUsage()));
}
it->second.coin = std::move(coin);
CCoinsCacheEntry::SetDirty(*it, m_sentinel);
@@ -153,8 +153,8 @@ void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool
bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) {
CCoinsMap::iterator it = FetchCoin(outpoint);
if (it == cacheCoins.end()) return false;
- m_dirty_count -= it->second.IsDirty();
- cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
+ Assume(TrySub(m_dirty_count, it->second.IsDirty()));
+ Assume(TrySub(cachedCoinsUsage, it->second.coin.DynamicMemoryUsage()));
TRACEPOINT(utxocache, spent,
outpoint.hash.data(),
(uint32_t)outpoint.n,
@@ -248,12 +248,12 @@ void CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& ha
if (itUs->second.IsFresh() && it->second.coin.IsSpent()) {
// The grandparent cache does not have an entry, and the coin
// has been spent. We can just delete it from the parent cache.
- m_dirty_count -= itUs->second.IsDirty();
- cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
+ Assume(TrySub(m_dirty_count, itUs->second.IsDirty()));
+ Assume(TrySub(cachedCoinsUsage, itUs->second.coin.DynamicMemoryUsage()));
cacheCoins.erase(itUs);
} else {
// A normal modification.
- cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
+ Assume(TrySub(cachedCoinsUsage, itUs->second.coin.DynamicMemoryUsage()));
if (cursor.WillErase(*it)) {
// Since this entry will be erased,
// we can move the coin into us instead of copying it
@@ -311,7 +311,7 @@ void CCoinsViewCache::Uncache(const COutPoint& hash)
{
CCoinsMap::iterator it = cacheCoins.find(hash);
if (it != cacheCoins.end() && !it->second.IsDirty()) {
- cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
+ Assume(TrySub(cachedCoinsUsage, it->second.coin.DynamicMemoryUsage()));
TRACEPOINT(utxocache, uncache,
hash.hash.data(),
(uint32_t)hash.n,
diff --git a/src/coins.h b/src/coins.h
index ba23e3d3..08c1886f 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -15,6 +15,7 @@
#include <support/allocators/pool.h>
#include <uint256.h>
#include <util/check.h>
+#include <util/overflow.h>
#include <util/hasher.h>
#include <cassert>
@@ -278,7 +279,7 @@ struct CoinsViewCacheCursor
inline CoinsCachePair* NextAndMaybeErase(CoinsCachePair& current) noexcept
{
const auto next_entry{current.second.Next()};
- m_dirty_count -= current.second.IsDirty();
+ Assume(TrySub(m_dirty_count, current.second.IsDirty()));
// If we are not going to erase the cache, we must still erase spent entries.
// Otherwise, clear the state of the entry.
if (!m_will_erase) {
diff --git a/src/util/overflow.h b/src/util/overflow.h
index 8b8511f8..e7498fd8 100644
--- a/src/util/overflow.h
+++ b/src/util/overflow.h
@@ -31,6 +31,14 @@ template <class T>
return i + j;
}
+template <std::unsigned_integral T, std::unsigned_integral U>
+[[nodiscard]] constexpr bool TrySub(T& i, const U j) noexcept
+{
+ if (i < T{j}) return false;
+ i -= T{j};
+ return true;
+}
+
template <class T>
[[nodiscard]] T SaturatingAdd(const T i, const T j) noexcept
{
Why this scored 44/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.