max_pct to not exceed the maximum number of addresses
#31235
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31235.
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.
0diff --git i/src/addrman.cpp w/src/addrman.cpp
1index 43e7b6a32c..9fb092e9af 100644
2--- i/src/addrman.cpp
3+++ w/src/addrman.cpp
4@@ -812,13 +812,13 @@ nid_type AddrManImpl::GetEntry(bool use_tried, size_t bucket, size_t position) c
5 std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
6 {
7 AssertLockHeld(cs);
8
9 size_t nNodes = vRandom.size();
10 if (max_pct != 0) {
11- nNodes = max_pct * nNodes / 100;
12+ nNodes = std::min(nNodes, max_pct * nNodes / 100);
13 }
14 if (max_addresses != 0) {
15 nNodes = std::min(nNodes, max_addresses);
16 }
17
18 // gather a list of random nodes, skipping those of low quality
19diff --git i/src/test/fuzz/addrman.cpp w/src/test/fuzz/addrman.cpp
20index a7e7f49d9f..ce50e22916 100644
21--- i/src/test/fuzz/addrman.cpp
22+++ w/src/test/fuzz/addrman.cpp
23@@ -169,14 +169,14 @@ FUZZ_TARGET(addrman, .init = initialize_addrman)
24 }
25 const AddrMan& const_addr_man{addr_man};
26 std::optional<Network> network;
27 if (fuzzed_data_provider.ConsumeBool()) {
28 network = fuzzed_data_provider.PickValueInArray(ALL_NETWORKS);
29 }
30- auto max_addresses = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096);
31- auto max_pct = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096);
32+ auto max_addresses = fuzzed_data_provider.ConsumeIntegral<size_t>();
33+ auto max_pct = fuzzed_data_provider.ConsumeIntegral<size_t>();
34 auto filtered = fuzzed_data_provider.ConsumeBool();
35 (void)const_addr_man.GetAddr(max_addresses, max_pct, network, filtered);
36
37 std::unordered_set<Network> nets;
38 for (const auto& net : ALL_NETWORKS) {
39 if (fuzzed_data_provider.ConsumeBool()) {
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:
0 /**
1 * Return all or many randomly selected addresses, optionally by network.
2 *
3 * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_addresses Maximum number of addresses to return (0 = all).
4 * [@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.
0--- i/src/addrman.h
1+++ w/src/addrman.h
2@@ -163,13 +163,13 @@ public:
3 std::pair<CAddress, NodeSeconds> Select(bool new_only = false, const std::unordered_set<Network>& networks = {}) const;
4
5 /**
6 * Return all or many randomly selected addresses, optionally by network.
7 *
8 * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_addresses Maximum number of addresses to return (0 = all).
9- * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_pct Maximum percentage of addresses to return (0 = all).
10+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] max_pct Maximum percentage of addresses to return, values >100 are treated as 100 (0 = all).
11 * [@param](/bitcoin-bitcoin/contributor/param/)[in] network Select only addresses of this network (nullopt = all).
12 * [@param](/bitcoin-bitcoin/contributor/param/)[in] filtered Select only addresses that are considered good quality (false = all).
13 *
14 * [@return](/bitcoin-bitcoin/contributor/return/) A vector of randomly selected addresses from vRandom.
15 */
16 std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const;
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);
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:
0max_pct = std::min(max_pct, 100);
1nNodes = 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.
Force-pushed:
Assume for the max_pct.GetAddr and GetAddress documentation.max_pct.Thanks @mzumsande and @vasild.
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);
size_t{100}, as explained in the dev notes.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
GetAddr_ look reasonable.
