Avoid out-of-bounds user input for PeerManager::Options by safely clamping -maxorphantx and -blockreconstructionextratxn, and avoid platform-specific behaviour by changing PeerManager::Options::max_extra_txs from size_t to a uint32_t. Addresses #27499#pullrequestreview-1544114932.
Also documents all PeerManager::Options members, addressing #27499 (review).
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
52 | + //! Whether transaction reconciliation protocol is enabled 53 | bool reconcile_txs{DEFAULT_TXRECONCILIATION_ENABLE}; 54 | + //! Maximum number of orphan transactions kept in memory 55 | uint32_t max_orphan_txs{DEFAULT_MAX_ORPHAN_TRANSACTIONS}; 56 | - size_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN}; 57 | + //! Number of orphan+recently-replaced transactions to keep around for block reconstruction
nit, includes a bit more than replaced and orphans
//! Number of non-mempool transactions to keep around for block reconstruction. Includes orphan, replaced, and rejected transactions.
Ah thanks, fixed in aa89e04e07ca9ff51b1d7d310a11821c6ad963cf
utACK 128ad03792cd4aeeaf32807d07f01e3f85adaf28
Thanks for the followup
17 | + options.max_orphan_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max()))); 18 | } 19 | 20 | if (auto value{argsman.GetIntArg("-blockreconstructionextratxn")}) { 21 | - options.max_extra_txs = size_t(std::max(int64_t{0}, *value)); 22 | + options.max_extra_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
Wondering if we can come up with a more sane upper limit for both of these?
Given that we use this value to size a vector, that could be sensible, but of course, arbitrary upper limits bring their own set of problems. I'll look into it more, thanks!
I'm not really keen on setting limits. I don't know what good values would be, and ultimately it's not a huge problem if the value is too high. I think issuing a warning could be sensible as it does have meaningful impact, but I don't have a strong view. Too many warnings/popups is also not great UX.
For example, this would log a warning in the console, and on qt issue a warning pop-up if a value > 100x default value is entered:
<details> <summary>git diff on 547fa52443cbb5e8ccfee993486f5ced8cdbb33b</summary>
diff --git a/src/node/peerman_args.cpp b/src/node/peerman_args.cpp
index efe4514271..53034d6277 100644
--- a/src/node/peerman_args.cpp
+++ b/src/node/peerman_args.cpp
@@ -2,6 +2,8 @@
#include <common/args.h>
#include <net_processing.h>
+#include <node/interface_ui.h>
+#include <util/translation.h>
#include <algorithm>
#include <limits>
@@ -13,10 +15,16 @@ void ApplyArgsManOptions(const ArgsManager& argsman, PeerManager::Options& optio
if (auto value{argsman.GetBoolArg("-txreconciliation")}) options.reconcile_txs = *value;
if (auto value{argsman.GetIntArg("-maxorphantx")}) {
+ if (*value > (100 * options.max_orphan_txs)) {
+ InitWarning(_("-maxorphantx is > 100x larger than the default value, affecting memory consumption"));
+ }
options.max_orphan_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
}
if (auto value{argsman.GetIntArg("-blockreconstructionextratxn")}) {
+ if (*value > (100 * options.max_extra_txs)) {
+ InitWarning(_("-blockreconstructionextratxn is > 100x larger than the default value, affecting memory consumption"));
+ }
options.max_extra_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
}
</details>
What do you think?
Let's just leave it as is, was more of a nit anyway
Concept ACK
Also changes max_extra_txs into a uint32_t to avoid platform-specific
behaviour
Code review ACK 547fa52443cbb5e8ccfee993486f5ced8cdbb33b
12 | {
13 | if (auto value{argsman.GetBoolArg("-txreconciliation")}) options.reconcile_txs = *value;
14 |
15 | if (auto value{argsman.GetIntArg("-maxorphantx")}) {
16 | - options.max_orphan_txs = uint32_t(std::max(int64_t{0}, *value));
17 | + options.max_orphan_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
unrelated: May be good to write a clang-tidy plugin to enforce the limits are compile-time constants and in range to avoid silent UB at runtime?
The in-range one can be submitted to upstream and the other check can be done in this repo.
options.max_orphan_txs = uint32_t(std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max()));
nit, if you re-touch?
reACK 547fa52443cbb5e8ccfee993486f5ced8cdbb33b