Fixes #31234
This PR fixes a bad alloc issue in GetAddresses by capping the value max_pct. In practice, values greater than 100 should be treated as 100 since it's the percentage of addresses to return. Also, it limites the value max_pct in connman target to exercise values between 0 and 100.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31235.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | marcofleon, mzumsande, adamandrews1, vasild |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
I think that the problem warrants a fix in the real code, not in the test. AddrManImpl::GetAddr_() would try to overallocate if the caller passes > 100% which is meaningless. It already handles max_addresses properly: nNodes = std::min(nNodes, max_addresses);. If there are 1000 addresses and the caller asks for 5000, it would clamp it down to 1000.
<details> <summary>[patch] clamp down the % of addresses, like already done for `max_addresses`</summary>
diff --git i/src/addrman.cpp w/src/addrman.cpp
index 43e7b6a32c..9fb092e9af 100644
--- i/src/addrman.cpp
+++ w/src/addrman.cpp
@@ -812,13 +812,13 @@ nid_type AddrManImpl::GetEntry(bool use_tried, size_t bucket, size_t position) c
std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
{
AssertLockHeld(cs);
size_t nNodes = vRandom.size();
if (max_pct != 0) {
- nNodes = max_pct * nNodes / 100;
+ nNodes = std::min(nNodes, max_pct * nNodes / 100);
}
if (max_addresses != 0) {
nNodes = std::min(nNodes, max_addresses);
}
// gather a list of random nodes, skipping those of low quality
diff --git i/src/test/fuzz/addrman.cpp w/src/test/fuzz/addrman.cpp
index a7e7f49d9f..ce50e22916 100644
--- i/src/test/fuzz/addrman.cpp
+++ w/src/test/fuzz/addrman.cpp
@@ -169,14 +169,14 @@ FUZZ_TARGET(addrman, .init = initialize_addrman)
}
const AddrMan& const_addr_man{addr_man};
std::optional<Network> network;
if (fuzzed_data_provider.ConsumeBool()) {
network = fuzzed_data_provider.PickValueInArray(ALL_NETWORKS);
}
- auto max_addresses = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096);
- auto max_pct = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096);
+ auto max_addresses = fuzzed_data_provider.ConsumeIntegral<size_t>();
+ auto max_pct = fuzzed_data_provider.ConsumeIntegral<size_t>();
auto filtered = fuzzed_data_provider.ConsumeBool();
(void)const_addr_man.GetAddr(max_addresses, max_pct, network, filtered);
std::unordered_set<Network> nets;
for (const auto& net : ALL_NETWORKS) {
if (fuzzed_data_provider.ConsumeBool()) {
</details>
I think that the problem warrants a fix in the real code, not in the test.
We can still limit the values to reflect the reality. I don't see how to have a huge max_pct in practice. For example, for GETADDR, we use MAX_ADDR_TO_SEND/MAX_PCT_ADDR_TO_SEND. For the RPC getnodeaddresses and other usage, its value is always zero.
In the future the callers of AddrManImpl::GetAddr_() can change, or new callers can be added. It is good to ensure that it does not misbehave or crash the program for some input.
I understand that this is not an universal argument and should be applied with reason, with limits. For example, the following is beyond reason: "std::vector::reserve() is going to crash the program for some input - if asked to allocate 1000TB, lets fix it!".
IMO AddrManImpl::GetAddr_() capping max_pct is reasonable. It already caps max_addresses. And then we can ensure it does by throwing any random numbers at it and making sure it does not break.
IMO AddrManImpl::GetAddr_() capping max_pct is reasonable. It already caps max_addresses. And then we can ensure it does by throwing any random numbers at it and making sure it does not break.
I agree. I can address it. I'm thinking about If max_pct is the maximum percentage of addresses to return. We could add an assert in GetAddr that this number is not greater than 100 and change the targets to use values from 0 to 100. I doesn't make sense to try to get 10000000% of the addresses, for example.
Yeah, that too. Then the test should indeed not call it with >100. If you go this way, then it should be clearly documented in:
/**
* Return all or many randomly selected addresses, optionally by network.
*
* [@param](/bitcoin-bitcoin/contributor/param/)[in] max_addresses Maximum number of addresses to return (0 = all).
* [@param](/bitcoin-bitcoin/contributor/param/)[in] max_pct Maximum percentage of addresses to return (0 = all).
Somehow I feel it is safer to treat values >100 as 100 and document that instead of crashing with an assert (or bad alloc).
Somehow I feel it is safer to treat values >100 as 100 and document that instead of crashing with an assert (or bad alloc).
Sure, I will address this.
109 | @@ -110,13 +110,13 @@ FUZZ_TARGET(connman, .init = initialize_connman) 110 | }, 111 | [&] { 112 | auto max_addresses = fuzzed_data_provider.ConsumeIntegral<size_t>(); 113 | - auto max_pct = fuzzed_data_provider.ConsumeIntegral<size_t>(); 114 | + auto max_pct = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 200);
It would be good extend the GetAddr() doc to something like the following. And then no need to cap to 200 here.
--- i/src/addrman.h
+++ w/src/addrman.h
@@ -163,13 +163,13 @@ public:
std::pair<CAddress, NodeSeconds> Select(bool new_only = false, const std::unordered_set<Network>& networks = {}) const;
/**
* Return all or many randomly selected addresses, optionally by network.
*
* [@param](/bitcoin-bitcoin/contributor/param/)[in] max_addresses Maximum number of addresses to return (0 = all).
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_pct Maximum percentage of addresses to return (0 = all).
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_pct Maximum percentage of addresses to return, values >100 are treated as 100 (0 = all).
* [@param](/bitcoin-bitcoin/contributor/param/)[in] network Select only addresses of this network (nullopt = all).
* [@param](/bitcoin-bitcoin/contributor/param/)[in] filtered Select only addresses that are considered good quality (false = all).
*
* [@return](/bitcoin-bitcoin/contributor/return/) A vector of randomly selected addresses from vRandom.
*/
std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const;
The cap to 200 is just to facilitate exploring valid ranges and some exceeds.
Going to extend the doc. Thanks.
Done.
ACK 81f26e9225838a1f0e0293c84f875efab95d9724
Force-pushed addressing #31235 (review)
ACK 9cdf4e67bf04b5abb46b4ce1cf4c306819afe056
814 | @@ -815,7 +815,7 @@ std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct 815 | 816 | size_t nNodes = vRandom.size(); 817 | if (max_pct != 0) { 818 | - nNodes = max_pct * nNodes / 100; 819 | + nNodes = std::min(nNodes, max_pct * nNodes / 100);
Since there was a bug somewhere if we ever pass max_pct > 100 (with privacy implications, the reason max_pct exists is not to reveal the entire addrman), why not Assume(max_pct <=100) / adjust the fuzzer range instead of trying to accommodate it? There is a difference between max_pct and the max_addresses where a caller could legitimately ask for more addresses than addrman has because they don't know current addrman size - no caller should ever ask for more than 100%.
I am fine either way and slightly prefer the softer approach of capping it to 100 instead of asserting that it is <=100. There was a discussion some time ago about crashing the program way too easily and reserving the assert/crash only for cases where the continuation of the program really does not make sense. I kind of agree with that.
Yes, Assume() is some middle ground between assert() and the softer approach. Assume() + cap in release builds looks reasonable to me too. Plus document it properly: "make sure you never pass >100 or you will crash the program" ... hmm, could this end up in all callers doing the cap at the call site before calling GetAddr() to make sure to avoid a crash?
Do you remember the bug number? "The reason max_pct exists is not to reveal the entire addrman", but then passing even == 100 is a bug?
If you will be touching this, there is this minor mostly theoretical thing: max_pct * nNodes could overflow and I guess max_pct * nNodes / 100 could end up with a lower value than nNodes for some very high input max_pct. So, if you will be touching this, maybe it is "safer" to implement the cap as:
max_pct = std::min(max_pct, 100);
nNodes = max_pct * nNodes / 100;
(this occurred to me just recently)
Do you remember the bug number? "The reason max_pct exists is not to reveal the entire addrman", but then passing even == 100 is a bug?
GetAddr answers have been capped by percentage at least since addrman was introduced (5fee401fe14aa6459428a26a82f764db70a6a0b9), just that MAX_PCT_ADDR_TO_SEND=23 was hardcoded in the past. It's kind of still that way (MAX_PCT_ADDR_TO_SEND is used as an arg in net_processing, the rpc code path doesn't use max_pct.), so in current master nothing but 23 or 0 should ever be passed.
Assume() + cap in release builds looks reasonable to me too.
Agree.
Yes, Assume() is some middle ground between assert() and the softer approach. Assume() + cap in release builds looks reasonable to me too.
I agree. I can address it.
so in current master nothing but 23 or 0 should ever be passed.
So, we could adjust both addrman and connman fuzz targets to get 23 or 0 to use for max_pct. Sounds good?
so in current master nothing but 23 or 0 should ever be passed.
So, we could adjust both addrman and connman fuzz targets to get 23 or 0 to use for
max_pct. Sounds good?
I think fuzzing in range between 0 and 100 would make sense - after all that's what the function is designed to handle
I think fuzzing in range between 0 and 100 would make sense - after all that's what the function is designed to handle
I agree.
Done.
Force-pushed:
Assume for the max_pct.GetAddr and GetAddress documentation.max_pct.Thanks @mzumsande and @vasild.
The title still claims "fuzz", but the fix seems to be in production code - can we update the PR title?
811 | @@ -812,9 +812,11 @@ nid_type AddrManImpl::GetEntry(bool use_tried, size_t bucket, size_t position) c 812 | std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const 813 | { 814 | AssertLockHeld(cs); 815 | + Assume(max_pct <= 100); 816 | 817 | size_t nNodes = vRandom.size(); 818 | if (max_pct != 0) { 819 | + max_pct = std::min(max_pct, (size_t)100);
style-nit: For new code it would be good to use size_t{100}, as explained in the dev notes.
Done.
you'll have to squash the commits. Otherwise a fuzz bisect will fail on the addrman target as well. (connman already fails)
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Force-pushed squashing and addressing #31235 (review).
Tested ACK 9c5775c331e02dab06c78ecbb58488542d16dda7. Reproduced the crash on master and checked that this fixed it. The checks added to GetAddr_ look reasonable.
Code Review ACK 9c5775c331e02dab06c78ecbb58488542d16dda7
ACK 9c5775c331e02dab06c78ecbb58488542d16dda7
Milestone
29.0