Skip to content

Commit 3210d87

Browse files
achow101achow101
committed
Merge bitcoin/bitcoin#29043: fuzz: make FuzzedDataProvider usage deterministic
01960c5 fuzz: make FuzzedDataProvider usage deterministic (Martin Leitner-Ankerl) Pull request description: There exist many usages of `fuzzed_data_provider` where it is evaluated directly in the function call. Unfortunately, [the order of evaluation of function arguments is unspecified](https://en.cppreference.com/w/cpp/language/eval_order), and a simple example shows that it can differ e.g. between clang++ and g++: https://godbolt.org/z/jooMezWWY When the evaluation order is not consistent, the same fuzzing/random input will produce different output, which is bad for coverage/reproducibility. This PR fixes all these cases I have found where unspecified evaluation order could be a problem. Finding these has been manual work; I grepped the sourcecode for these patterns, and looked at each usage individually. So there is a chance I missed some. * `fuzzed_data_provider` * `.Consume` * `>Consume` * `.rand` I first discovered this in bitcoin/bitcoin#29013 (comment). Note that there is a possibility that due to this fix the evaluation order is now different in many cases than when the fuzzing corpus has been created. If that is the case, the fuzzing corpus will have worse coverage than before. Update: In list-initialization the order of evaluation is well defined, so e.g. usages in `initializer_list` or constructors that use `{...}` is ok. ACKs for top commit: achow101: ACK 01960c5 vasild: ACK 01960c5 ismaelsadeeq: ACK 01960c5 Tree-SHA512: e56d087f6f4bf79c90b972a5f0c6908d1784b3cfbb8130b6b450d5ca7d116c5a791df506b869a23bce930b2a6977558e1fb5115bb4e061969cc40f568077a1ad
2 parents 8127654 + 01960c5 commit 3210d87

18 files changed

+129
-66
lines changed

‎src/test/fuzz/addrman.cpp

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -250,31 +250,41 @@ FUZZ_TARGET(addrman, .init = initialize_addrman)
250250
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
251251
addresses.push_back(ConsumeAddress(fuzzed_data_provider));
252252
}
253-
addr_man.Add(addresses, ConsumeNetAddr(fuzzed_data_provider), std::chrono::seconds{ConsumeTime(fuzzed_data_provider, 0, 100000000)});
253+
auto net_addr = ConsumeNetAddr(fuzzed_data_provider);
254+
auto time_penalty = std::chrono::seconds{ConsumeTime(fuzzed_data_provider, 0, 100000000)};
255+
addr_man.Add(addresses, net_addr, time_penalty);
254256
},
255257
[&] {
256-
addr_man.Good(ConsumeService(fuzzed_data_provider), NodeSeconds{std::chrono::seconds{ConsumeTime(fuzzed_data_provider)}});
258+
auto addr = ConsumeService(fuzzed_data_provider);
259+
auto time = NodeSeconds{std::chrono::seconds{ConsumeTime(fuzzed_data_provider)}};
260+
addr_man.Good(addr, time);
257261
},
258262
[&] {
259-
addr_man.Attempt(ConsumeService(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool(), NodeSeconds{std::chrono::seconds{ConsumeTime(fuzzed_data_provider)}});
263+
auto addr = ConsumeService(fuzzed_data_provider);
264+
auto count_failure = fuzzed_data_provider.ConsumeBool();
265+
auto time = NodeSeconds{std::chrono::seconds{ConsumeTime(fuzzed_data_provider)}};
266+
addr_man.Attempt(addr, count_failure, time);
260267
},
261268
[&] {
262-
addr_man.Connected(ConsumeService(fuzzed_data_provider), NodeSeconds{std::chrono::seconds{ConsumeTime(fuzzed_data_provider)}});
269+
auto addr = ConsumeService(fuzzed_data_provider);
270+
auto time = NodeSeconds{std::chrono::seconds{ConsumeTime(fuzzed_data_provider)}};
271+
addr_man.Connected(addr, time);
263272
},
264273
[&] {
265-
addr_man.SetServices(ConsumeService(fuzzed_data_provider), ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS));
274+
auto addr = ConsumeService(fuzzed_data_provider);
275+
auto n_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
276+
addr_man.SetServices(addr, n_services);
266277
});
267278
}
268279
const AddrMan& const_addr_man{addr_man};
269280
std::optional<Network> network;
270281
if (fuzzed_data_provider.ConsumeBool()) {
271282
network = fuzzed_data_provider.PickValueInArray(ALL_NETWORKS);
272283
}
273-
(void)const_addr_man.GetAddr(
274-
/*max_addresses=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
275-
/*max_pct=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
276-
network,
277-
/*filtered=*/fuzzed_data_provider.ConsumeBool());
284+
auto max_addresses = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096);
285+
auto max_pct = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096);
286+
auto filtered = fuzzed_data_provider.ConsumeBool();
287+
(void)const_addr_man.GetAddr(max_addresses, max_pct, network, filtered);
278288
(void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network);
279289
std::optional<bool> in_new;
280290
if (fuzzed_data_provider.ConsumeBool()) {

‎src/test/fuzz/banman.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,15 +78,19 @@ FUZZ_TARGET(banman, .init = initialize_banman)
7878
contains_invalid = true;
7979
}
8080
}
81-
ban_man.Ban(net_addr, ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
81+
auto ban_time_offset = ConsumeBanTimeOffset(fuzzed_data_provider);
82+
auto since_unix_epoch = fuzzed_data_provider.ConsumeBool();
83+
ban_man.Ban(net_addr, ban_time_offset, since_unix_epoch);
8284
},
8385
[&] {
8486
CSubNet subnet{ConsumeSubNet(fuzzed_data_provider)};
8587
subnet = LookupSubNet(subnet.ToString());
8688
if (!subnet.IsValid()) {
8789
contains_invalid = true;
8890
}
89-
ban_man.Ban(subnet, ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
91+
auto ban_time_offset = ConsumeBanTimeOffset(fuzzed_data_provider);
92+
auto since_unix_epoch = fuzzed_data_provider.ConsumeBool();
93+
ban_man.Ban(subnet, ban_time_offset, since_unix_epoch);
9094
},
9195
[&] {
9296
ban_man.ClearBanned();

‎src/test/fuzz/buffered_file.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@ FUZZ_TARGET(buffered_file)
2525
ConsumeRandomLengthByteVector<std::byte>(fuzzed_data_provider),
2626
};
2727
try {
28-
opt_buffered_file.emplace(fuzzed_file, fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096));
28+
auto n_buf_size = fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096);
29+
auto n_rewind_in = fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096);
30+
opt_buffered_file.emplace(fuzzed_file, n_buf_size, n_rewind_in);
2931
} catch (const std::ios_base::failure&) {
3032
}
3133
if (opt_buffered_file && !fuzzed_file.IsNull()) {

‎src/test/fuzz/connman.cpp

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -91,17 +91,15 @@ FUZZ_TARGET(connman, .init = initialize_connman)
9191
(void)connman.ForNode(fuzzed_data_provider.ConsumeIntegral<NodeId>(), [&](auto) { return fuzzed_data_provider.ConsumeBool(); });
9292
},
9393
[&] {
94-
(void)connman.GetAddresses(
95-
/*max_addresses=*/fuzzed_data_provider.ConsumeIntegral<size_t>(),
96-
/*max_pct=*/fuzzed_data_provider.ConsumeIntegral<size_t>(),
97-
/*network=*/std::nullopt,
98-
/*filtered=*/fuzzed_data_provider.ConsumeBool());
94+
auto max_addresses = fuzzed_data_provider.ConsumeIntegral<size_t>();
95+
auto max_pct = fuzzed_data_provider.ConsumeIntegral<size_t>();
96+
auto filtered = fuzzed_data_provider.ConsumeBool();
97+
(void)connman.GetAddresses(max_addresses, max_pct, /*network=*/std::nullopt, filtered);
9998
},
10099
[&] {
101-
(void)connman.GetAddresses(
102-
/*requestor=*/random_node,
103-
/*max_addresses=*/fuzzed_data_provider.ConsumeIntegral<size_t>(),
104-
/*max_pct=*/fuzzed_data_provider.ConsumeIntegral<size_t>());
100+
auto max_addresses = fuzzed_data_provider.ConsumeIntegral<size_t>();
101+
auto max_pct = fuzzed_data_provider.ConsumeIntegral<size_t>();
102+
(void)connman.GetAddresses(/*requestor=*/random_node, max_addresses, max_pct);
105103
},
106104
[&] {
107105
(void)connman.GetDeterministicRandomizer(fuzzed_data_provider.ConsumeIntegral<uint64_t>());

‎src/test/fuzz/crypto.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,9 @@ FUZZ_TARGET(crypto)
2222
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
2323
std::vector<uint8_t> data = ConsumeRandomLengthByteVector(fuzzed_data_provider);
2424
if (data.empty()) {
25-
data.resize(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, 4096), fuzzed_data_provider.ConsumeIntegral<uint8_t>());
25+
auto new_size = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, 4096);
26+
auto x = fuzzed_data_provider.ConsumeIntegral<uint8_t>();
27+
data.resize(new_size, x);
2628
}
2729

2830
CHash160 hash160;
@@ -44,7 +46,9 @@ FUZZ_TARGET(crypto)
4446
if (fuzzed_data_provider.ConsumeBool()) {
4547
data = ConsumeRandomLengthByteVector(fuzzed_data_provider);
4648
if (data.empty()) {
47-
data.resize(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, 4096), fuzzed_data_provider.ConsumeIntegral<uint8_t>());
49+
auto new_size = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, 4096);
50+
auto x = fuzzed_data_provider.ConsumeIntegral<uint8_t>();
51+
data.resize(new_size, x);
4852
}
4953
}
5054

‎src/test/fuzz/crypto_chacha20.cpp

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,10 @@ FUZZ_TARGET(crypto_chacha20)
2828
chacha20.SetKey(key);
2929
},
3030
[&] {
31-
chacha20.Seek(
32-
{
33-
fuzzed_data_provider.ConsumeIntegral<uint32_t>(),
34-
fuzzed_data_provider.ConsumeIntegral<uint64_t>()
35-
}, fuzzed_data_provider.ConsumeIntegral<uint32_t>());
31+
ChaCha20::Nonce96 nonce{
32+
fuzzed_data_provider.ConsumeIntegral<uint32_t>(),
33+
fuzzed_data_provider.ConsumeIntegral<uint64_t>()};
34+
chacha20.Seek(nonce, fuzzed_data_provider.ConsumeIntegral<uint32_t>());
3635
},
3736
[&] {
3837
std::vector<uint8_t> output(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096));

‎src/test/fuzz/cuckoocache.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@ FUZZ_TARGET(cuckoocache)
4141
if (fuzzed_data_provider.ConsumeBool()) {
4242
cuckoo_cache.insert(fuzzed_data_provider.ConsumeBool());
4343
} else {
44-
cuckoo_cache.contains(fuzzed_data_provider.ConsumeBool(), fuzzed_data_provider.ConsumeBool());
44+
auto e = fuzzed_data_provider.ConsumeBool();
45+
auto erase = fuzzed_data_provider.ConsumeBool();
46+
cuckoo_cache.contains(e, erase);
4547
}
4648
}
4749
fuzzed_data_provider_ptr = nullptr;

‎src/test/fuzz/message.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,9 @@ FUZZ_TARGET(message, .init = initialize_message)
3939
}
4040
{
4141
(void)MessageHash(random_message);
42-
(void)MessageVerify(fuzzed_data_provider.ConsumeRandomLengthString(1024), fuzzed_data_provider.ConsumeRandomLengthString(1024), random_message);
42+
auto address = fuzzed_data_provider.ConsumeRandomLengthString(1024);
43+
auto signature = fuzzed_data_provider.ConsumeRandomLengthString(1024);
44+
(void)MessageVerify(address, signature, random_message);
4345
(void)SigningResultString(fuzzed_data_provider.PickValueInArray({SigningResult::OK, SigningResult::PRIVATE_KEY_NOT_AVAILABLE, SigningResult::SIGNING_FAILED}));
4446
}
4547
}

‎src/test/fuzz/policy_estimator.cpp

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,9 +85,18 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator)
8585
});
8686
(void)block_policy_estimator.estimateFee(fuzzed_data_provider.ConsumeIntegral<int>());
8787
EstimationResult result;
88-
(void)block_policy_estimator.estimateRawFee(fuzzed_data_provider.ConsumeIntegral<int>(), fuzzed_data_provider.ConsumeFloatingPoint<double>(), fuzzed_data_provider.PickValueInArray(ALL_FEE_ESTIMATE_HORIZONS), fuzzed_data_provider.ConsumeBool() ? &result : nullptr);
88+
auto conf_target = fuzzed_data_provider.ConsumeIntegral<int>();
89+
auto success_threshold = fuzzed_data_provider.ConsumeFloatingPoint<double>();
90+
auto horizon = fuzzed_data_provider.PickValueInArray(ALL_FEE_ESTIMATE_HORIZONS);
91+
auto* result_ptr = fuzzed_data_provider.ConsumeBool() ? &result : nullptr;
92+
(void)block_policy_estimator.estimateRawFee(conf_target, success_threshold, horizon, result_ptr);
93+
8994
FeeCalculation fee_calculation;
90-
(void)block_policy_estimator.estimateSmartFee(fuzzed_data_provider.ConsumeIntegral<int>(), fuzzed_data_provider.ConsumeBool() ? &fee_calculation : nullptr, fuzzed_data_provider.ConsumeBool());
95+
conf_target = fuzzed_data_provider.ConsumeIntegral<int>();
96+
auto* fee_calc_ptr = fuzzed_data_provider.ConsumeBool() ? &fee_calculation : nullptr;
97+
auto conservative = fuzzed_data_provider.ConsumeBool();
98+
(void)block_policy_estimator.estimateSmartFee(conf_target, fee_calc_ptr, conservative);
99+
91100
(void)block_policy_estimator.HighestTargetTracked(fuzzed_data_provider.PickValueInArray(ALL_FEE_ESTIMATE_HORIZONS));
92101
}
93102
{

‎src/test/fuzz/prevector.cpp

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -210,15 +210,20 @@ FUZZ_TARGET(prevector)
210210
LIMITED_WHILE(prov.remaining_bytes(), 3000)
211211
{
212212
switch (prov.ConsumeIntegralInRange<int>(0, 13 + 3 * (test.size() > 0))) {
213-
case 0:
214-
test.insert(prov.ConsumeIntegralInRange<size_t>(0, test.size()), prov.ConsumeIntegral<int>());
215-
break;
213+
case 0: {
214+
auto position = prov.ConsumeIntegralInRange<size_t>(0, test.size());
215+
auto value = prov.ConsumeIntegral<int>();
216+
test.insert(position, value);
217+
} break;
216218
case 1:
217219
test.resize(std::max(0, std::min(30, (int)test.size() + prov.ConsumeIntegralInRange<int>(0, 4) - 2)));
218220
break;
219-
case 2:
220-
test.insert(prov.ConsumeIntegralInRange<size_t>(0, test.size()), 1 + prov.ConsumeBool(), prov.ConsumeIntegral<int>());
221-
break;
221+
case 2: {
222+
auto position = prov.ConsumeIntegralInRange<size_t>(0, test.size());
223+
auto count = 1 + prov.ConsumeBool();
224+
auto value = prov.ConsumeIntegral<int>();
225+
test.insert(position, count, value);
226+
} break;
222227
case 3: {
223228
int del = prov.ConsumeIntegralInRange<int>(0, test.size());
224229
int beg = prov.ConsumeIntegralInRange<int>(0, test.size() - del);
@@ -255,9 +260,11 @@ FUZZ_TARGET(prevector)
255260
case 9:
256261
test.clear();
257262
break;
258-
case 10:
259-
test.assign(prov.ConsumeIntegralInRange<size_t>(0, 32767), prov.ConsumeIntegral<int>());
260-
break;
263+
case 10: {
264+
auto n = prov.ConsumeIntegralInRange<size_t>(0, 32767);
265+
auto value = prov.ConsumeIntegral<int>();
266+
test.assign(n, value);
267+
} break;
261268
case 11:
262269
test.swap();
263270
break;
@@ -267,9 +274,11 @@ FUZZ_TARGET(prevector)
267274
case 13:
268275
test.move();
269276
break;
270-
case 14:
271-
test.update(prov.ConsumeIntegralInRange<size_t>(0, test.size() - 1), prov.ConsumeIntegral<int>());
272-
break;
277+
case 14: {
278+
auto pos = prov.ConsumeIntegralInRange<size_t>(0, test.size() - 1);
279+
auto value = prov.ConsumeIntegral<int>();
280+
test.update(pos, value);
281+
} break;
273282
case 15:
274283
test.erase(prov.ConsumeIntegralInRange<size_t>(0, test.size() - 1));
275284
break;

0 commit comments

Comments
 (0)