Merge bitcoin/bitcoin#36092: fix: UB sanitizer in mempool estimator logging
What the developer wrote
Authored by merge-script
Merge bitcoin/bitcoin#36092: fix: UB sanitizer in mempool estimator logging
576a0ebb536f93469a1ced6d497e11adf7c6a74d fix: UB sanitizer in mempool estimator logging (rustaceanrob)
Pull request description:
The following is failing in CI, when a block has `m_height` of 64 bit max:
```
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /home/runner/work/_temp/src/policy/fees/mempool_estimator.cpp:210:62
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x1,0x0,0x0,0x0,0x3,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x1,0x0,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x26,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x2f,0x0,0x3,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x7a,0x3f,0x3f,0x0,0x0,0x2f,0x0,0x3,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x7a,0x3f,0x3f,0x3f,0xff,0xff,0xff,0xff,0xff,0x18,0x0,0x0,0x85,0x3f,0xff,0xff,0xff,0xff,0xff,0x18,0x0,0x0,0x85,0xd6,0x1,0x0,0x86,0x0,0x0,0x0,0x2a,0x0,0xff,0xff,0xff,
\001\000\000\000\003\377\377\377\377\377\377\377\377\001\000\377\377\377\377\377\377\377\377\000\000&\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000/\000\003\002\002\002\002\002\002\002\000\000\000\000\000\000\000z??\000\000/\000\003\002\002\002\002\002\002\002\000\000\000\000\000\000\000z???\377\377\377\377\377\030\000\000\205?\377\377\377\377\377\030\000\000\205\326\001\000\206\000\000\000*\000\377\377\377
artifact_prefix='./'; Test unit written to ./crash-b4333d1fe3993fe8610b86654e385682c23050b9
Base64: AQAAAAP//////////wEA//////////8AACYAAAAAAAAAAAAAAAAAAAAAAAAvAAMCAgICAgICAAAAAAAAAHo/PwAALwADAgICAgICAgAAAAAAAAB6Pz8///////8YAACFP///////GAAAhdYBAIYAAAAqAP///w==
⚠️ Failure generated from target with exit code 1: ['/home/runner/work/_temp/build_ ₿🧪_/bin/fuzz', '-runs=1', PosixPath('/home/runner/work/_temp/ci/scratch_ ₿🧪_/qa-assets/fuzz_corpora/policy_estimator_io')]
Check if using libFuzzer ... True
Command '['docker', 'exec', '--env', 'DANGER_RUN_CI_ON_HOST=1', '8100bf684275e706787e07f8ab94431926ba5184562c52bce95c932210f6f38f', '/home/runner/work/_temp/ci/test/03_test_script.sh']' returned non-zero exit status 1.
```
ACKs for top commit:
maflcko:
lgtm ACK 576a0ebb536f93469a1ced6d497e11adf7c6a74d
marcofleon:
ACK 576a0ebb536f93469a1ced6d497e11adf7c6a74d
jeanpablojp:
tACK 576a0ebb536f93469a1ced6d497e11adf7c6a74d
Tree-SHA512: a7533e68a95b2f0200abdcf08ae72a7f3654db03cffe642ed39b0a5aa48d932a8b07ee4e4477b591a488b5358dd19fe85a03015642b663fd41a771b3090bc8a3
What changed, and why it matters
This change fixes a logging statement that could trigger an 'unsigned integer overflow' warning when Bitcoin Core is run with special sanitizer checks. It only affects what gets printed to logs when reading a fee-estimator data file with an extremely large block height (near the 64-bit maximum). It does not change transaction validation, consensus rules, or network behavior. The fix replaces a plain addition with a 'saturating add' helper so the number stops at the maximum instead of wrapping around.
No urgent action required beyond applying the patch. Operators running with UBSan will no longer see sanitizer aborts on crafted/malformed fee-estimator files. Consider whether the fuzz corpus should retain the crashing input as a regression test.
Security signals we found
UndefinedBehaviorSanitizer unsigned-integer-overflow triggered in CI fuzz target
Fix uses overflow-saturating arithmetic helper
Issue is in logging/validation of persisted fee-estimator state, not consensus-critical path
Evidence from the diff
In MemPoolFeeRateEstimator::Read, the code compared blocks[i].m_height against blocks[i-1].m_height + 1 and also printed that sum in a LogWarning. If blocks[i-1].m_height is UINT64_MAX, the expression blocks[i-1].m_height + 1 overflows, which is defined behavior for unsigned integers in C++ but is flagged by UBSan’s unsigned-integer-overflow check. The patch uses util/overflow.h’s SaturatingAdd to compute expected_height, so the value saturates at UINT64_MAX and the sanitizer is no longer triggered. The functional behavior (returning false on non-consecutive heights) is unchanged.
Changed components
src/policy/fees/mempool_estimator.cppMemPoolFeeRateEstimator::Readpolicy_estimator_io fuzz targetInspect captured patch +4 / −2
### src/policy/fees/mempool_estimator.cpp
@@ -18,6 +18,7 @@
#include <util/feefrac.h>
#include <util/fees.h>
#include <util/fs.h>
+#include <util/overflow.h>
#include <util/syserror.h>
#include <validation.h>
@@ -207,10 +208,11 @@ bool MemPoolFeeRateEstimator::Read(AutoFile& file)
return false;
}
for (size_t i = 1; i < blocks.size(); ++i) {
- if (blocks[i].m_height != blocks[i - 1].m_height + 1) {
+ const uint64_t expected_height{SaturatingAdd(blocks[i - 1].m_height, uint64_t{1})};
+ if (blocks[i].m_height != expected_height) {
LogWarning("%s: Non-consecutive block heights read, expected height %s but found %s; ignoring file",
FeeRateEstimatorTypeToString(FeeRateEstimatorType::MEMPOOL_POLICY),
- blocks[i - 1].m_height + 1, blocks[i].m_height);
+ expected_height, blocks[i].m_height);
return false;
}
}Why this scored 26/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.