refactor: Use uint64_t over size_t for serialize corruption check in fees.dat #34109
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34109. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
|
ACK fa1d17d Straightforward fix - using |
ismaelsadeeq
left a comment
There was a problem hiding this comment.
Concept ACK.
Nice catch. It is important to fix this.
However, changing maxConfirms to uint64_t would only prevent the unsigned integer overflow by allowing it to store a very large value (or the modulo of that value).
Let's extend this by adding another commit that validates the scale.
Since we allow reading a corrupted scale, an overflow where the modulo of the multiplication result is stored in maxConfirms (scale* maxPeriods) may go undetected. If that modulo happens to fall within one 1-1008, the in L-454 check would pass, but the resulting scale would be invalid; this is a contrived, rare edge case.
We could be more defensive by requiring that the scale is exactly one of SHORT_SCALE, MED_SCALE, or LONG_SCALE. Otherwise, a corruption that produces a different in-range value would still be accepted as well.
However, for consistency with existing validation (e.g. decay), it likely makes sense to only enforce the upper bound rather than requiring an exact match.
diff --git a/src/policy/fees/block_policy_estimator.cpp b/src/policy/fees/block_policy_estimator.cpp
index 23aecefea2a..03d1c854811 100644
--- a/src/policy/fees/block_policy_estimator.cpp
+++ b/src/policy/fees/block_policy_estimator.cpp
@@ -435,6 +435,10 @@ void TxConfirmStats::Read(AutoFile& filein, size_t numBuckets)
throw std::runtime_error("Corrupt estimates file. Scale must be non-zero");
}
+ if (scale > LONG_SCALE) {
+ throw std::runtime_error("Corrupt estimates file. Scale must not exceed the long scale");
+ }
+
filein >> Using<VectorFormatter<EncodedDoubleFormatter>>(m_feerate_avg);
if (m_feerate_avg.size() != numBuckets) {
throw std::runtime_error("Corrupt estimates file. Mismatch in feerate average bucket count");
diff --git a/src/policy/fees/block_policy_estimator.h b/src/policy/fees/block_policy_estimator.h
index ec432dcbf53..a5ade9cd01d 100644
--- a/src/policy/fees/block_policy_estimator.h
+++ b/src/policy/fees/block_policy_estimator.h
@@ -98,6 +98,12 @@ struct FeeCalculation
unsigned int best_height{0};
};
+namespace {
+static constexpr unsigned int SHORT_SCALE = 1;
+static constexpr unsigned int MED_SCALE = 2;
+static constexpr unsigned int LONG_SCALE = 24;
+}
+
/** \class CBlockPolicyEstimator
* The BlockPolicyEstimator is used for estimating the feerate needed
* for a transaction to be included in a block within a certain number of
@@ -151,13 +157,10 @@ class CBlockPolicyEstimator : public CValidationInterface
private:
/** Track confirm delays up to 12 blocks for short horizon */
static constexpr unsigned int SHORT_BLOCK_PERIODS = 12;
- static constexpr unsigned int SHORT_SCALE = 1;
/** Track confirm delays up to 48 blocks for medium horizon */
static constexpr unsigned int MED_BLOCK_PERIODS = 24;
- static constexpr unsigned int MED_SCALE = 2;
/** Track confirm delays up to 1008 blocks for long horizon */
static constexpr unsigned int LONG_BLOCK_PERIODS = 42;
- static constexpr unsigned int LONG_SCALE = 24;
/** Historical estimates that are older than this aren't valid */
static const unsigned int OLDEST_ESTIMATE_HISTORY = 6 * 1008;
Not sure this is relevant to fix. A Recall, that filein >> Using<VectorFormatter<VectorFormatter<EncodedDoubleFormatter>>>(confAvg);
maxPeriods = confAvg.size();
maxConfirms = scale * maxPeriods;So, for u64 to overflow as a result of multiplication, a massive persisted file would be required ( A I am less sure about adding code to further validate the |
|
It’s fine with me to limit this to the unsigned integer overflow fix. utACK fa1d17d |
|
Yeah, seems fine to track this in a follow-up. Just for reference, if the diff --git a/src/policy/fees/block_policy_estimator.cpp b/src/policy/fees/block_policy_estimator.cpp
index 23aecefea2..8f31b98212 100644
--- a/src/policy/fees/block_policy_estimator.cpp
+++ b/src/policy/fees/block_policy_estimator.cpp
@@ -105,7 +105,7 @@ private:
double decay;
// Resolution (# of blocks) with which confirmations are tracked
- unsigned int scale;
+ const unsigned int scale;
// Mempool counts of outstanding transactions
// For each bucket X, track the number of transactions in the mempool
@@ -430,9 +430,12 @@ void TxConfirmStats::Read(AutoFile& filein, size_t numBuckets)
if (decay <= 0 || decay >= 1) {
throw std::runtime_error("Corrupt estimates file. Decay must be between 0 and 1 (non-inclusive)");
}
- filein >> scale;
- if (scale == 0) {
- throw std::runtime_error("Corrupt estimates file. Scale must be non-zero");
+ {
+ uint32_t dummy;
+ filein >> dummy;
+ if (scale == 0||scale!=dummy) {
+ throw std::runtime_error(strprintf("Corrupt estimates file. Scale must be non-zero and must be equal to %s", dummy));
+ }
}
filein >> Using<VectorFormatter<EncodedDoubleFormatter>>(m_feerate_avg);(passes tests, fwiw) |
I'll leave as-is for now, because I don't think an additional patch is needed, and the change is also unclear, because three people wrote three different patches for this. So this seems better to leave as-is or for a follow-up. Whereas this pull request should be trivially correct and already has 3 acks. |
Serialization should not behave differently on different architectures. See also the related commit 3789215.
However, on fees.dat file corruption, 32-bit builds may run into an unsigned integer overflow and report the wrong corruption reason, or may even silently continue after the corruption.
This is a bit hard to reproduce, because 32-bit platforms are rare and most of them don't support running the unsigned integer overflow sanitizer. So the possible options to reproduce are:
podman run -it --rm --platform linux/i386 'debian:trixie')uint32_tAfterwards, the steps to reproduce are:
The output will be something like:
Note: This is marked a "refactor", because the code change does not affect 64-bit builds, and on the still remaining rare 32-bit builds today it is extremely unlikely to happen in production.