Merge bitcoin/bitcoin#29043: fuzz: make FuzzedDataProvider usage dete…

…rministic

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

src/test/fuzz/addrman.cpp

@@ -250,31 +250,41 @@ FUZZ_TARGET(addrman, .init = initialize_addrman)
                 LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
                     addresses.push_back(ConsumeAddress(fuzzed_data_provider));
                 }
-                addr_man.Add(addresses, ConsumeNetAddr(fuzzed_data_provider), std::chrono::seconds{ConsumeTime(fuzzed_data_provider, 0, 100000000)});
+                auto net_addr = ConsumeNetAddr(fuzzed_data_provider);
+                auto time_penalty = std::chrono::seconds{ConsumeTime(fuzzed_data_provider, 0, 100000000)};
+                addr_man.Add(addresses, net_addr, time_penalty);
             },
             [&] {
-                addr_man.Good(ConsumeService(fuzzed_data_provider), NodeSeconds{std::chrono::seconds{ConsumeTime(fuzzed_data_provider)}});
+                auto addr = ConsumeService(fuzzed_data_provider);
+                auto time = NodeSeconds{std::chrono::seconds{ConsumeTime(fuzzed_data_provider)}};
+                addr_man.Good(addr, time);
             },
             [&] {
-                addr_man.Attempt(ConsumeService(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool(), NodeSeconds{std::chrono::seconds{ConsumeTime(fuzzed_data_provider)}});
+                auto addr = ConsumeService(fuzzed_data_provider);
+                auto count_failure = fuzzed_data_provider.ConsumeBool();
+                auto time = NodeSeconds{std::chrono::seconds{ConsumeTime(fuzzed_data_provider)}};
+                addr_man.Attempt(addr, count_failure, time);
             },
             [&] {
-                addr_man.Connected(ConsumeService(fuzzed_data_provider), NodeSeconds{std::chrono::seconds{ConsumeTime(fuzzed_data_provider)}});
+                auto addr = ConsumeService(fuzzed_data_provider);
+                auto time = NodeSeconds{std::chrono::seconds{ConsumeTime(fuzzed_data_provider)}};
+                addr_man.Connected(addr, time);
             },
             [&] {
-                addr_man.SetServices(ConsumeService(fuzzed_data_provider), ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS));
+                auto addr = ConsumeService(fuzzed_data_provider);
+                auto n_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
+                addr_man.SetServices(addr, n_services);
             });
     }
     const AddrMan& const_addr_man{addr_man};
     std::optional<Network> network;
     if (fuzzed_data_provider.ConsumeBool()) {
         network = fuzzed_data_provider.PickValueInArray(ALL_NETWORKS);
     }
-    (void)const_addr_man.GetAddr(
-        /*max_addresses=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
-        /*max_pct=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
-        network,
-        /*filtered=*/fuzzed_data_provider.ConsumeBool());
+    auto max_addresses = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096);
+    auto max_pct = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096);
+    auto filtered = fuzzed_data_provider.ConsumeBool();
+    (void)const_addr_man.GetAddr(max_addresses, max_pct, network, filtered);
     (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network);
     std::optional<bool> in_new;
     if (fuzzed_data_provider.ConsumeBool()) {

src/test/fuzz/banman.cpp

@@ -78,15 +78,19 @@ FUZZ_TARGET(banman, .init = initialize_banman)
                             contains_invalid = true;
                         }
                     }
-                    ban_man.Ban(net_addr, ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
+                    auto ban_time_offset = ConsumeBanTimeOffset(fuzzed_data_provider);
+                    auto since_unix_epoch = fuzzed_data_provider.ConsumeBool();
+                    ban_man.Ban(net_addr, ban_time_offset, since_unix_epoch);
                 },
                 [&] {
                     CSubNet subnet{ConsumeSubNet(fuzzed_data_provider)};
                     subnet = LookupSubNet(subnet.ToString());
                     if (!subnet.IsValid()) {
                         contains_invalid = true;
                     }
-                    ban_man.Ban(subnet, ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());
+                    auto ban_time_offset = ConsumeBanTimeOffset(fuzzed_data_provider);
+                    auto since_unix_epoch = fuzzed_data_provider.ConsumeBool();
+                    ban_man.Ban(subnet, ban_time_offset, since_unix_epoch);
                 },
                 [&] {
                     ban_man.ClearBanned();

src/test/fuzz/buffered_file.cpp

@@ -25,7 +25,9 @@ FUZZ_TARGET(buffered_file)
         ConsumeRandomLengthByteVector<std::byte>(fuzzed_data_provider),
     };
     try {
-        opt_buffered_file.emplace(fuzzed_file, fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096));
+        auto n_buf_size = fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096);
+        auto n_rewind_in = fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096);
+        opt_buffered_file.emplace(fuzzed_file, n_buf_size, n_rewind_in);
     } catch (const std::ios_base::failure&) {
     }
     if (opt_buffered_file && !fuzzed_file.IsNull()) {

src/test/fuzz/connman.cpp

@@ -91,17 +91,15 @@ FUZZ_TARGET(connman, .init = initialize_connman)
                 (void)connman.ForNode(fuzzed_data_provider.ConsumeIntegral<NodeId>(), [&](auto) { return fuzzed_data_provider.ConsumeBool(); });
             },
             [&] {
-                (void)connman.GetAddresses(
-                    /*max_addresses=*/fuzzed_data_provider.ConsumeIntegral<size_t>(),
-                    /*max_pct=*/fuzzed_data_provider.ConsumeIntegral<size_t>(),
-                    /*network=*/std::nullopt,
-                    /*filtered=*/fuzzed_data_provider.ConsumeBool());
+                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)connman.GetAddresses(max_addresses, max_pct, /*network=*/std::nullopt, filtered);
             },
             [&] {
-                (void)connman.GetAddresses(
-                    /*requestor=*/random_node,
-                    /*max_addresses=*/fuzzed_data_provider.ConsumeIntegral<size_t>(),
-                    /*max_pct=*/fuzzed_data_provider.ConsumeIntegral<size_t>());
+                auto max_addresses = fuzzed_data_provider.ConsumeIntegral<size_t>();
+                auto max_pct = fuzzed_data_provider.ConsumeIntegral<size_t>();
+                (void)connman.GetAddresses(/*requestor=*/random_node, max_addresses, max_pct);
             },
             [&] {
                 (void)connman.GetDeterministicRandomizer(fuzzed_data_provider.ConsumeIntegral<uint64_t>());

src/test/fuzz/crypto.cpp

@@ -22,7 +22,9 @@ FUZZ_TARGET(crypto)
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     std::vector<uint8_t> data = ConsumeRandomLengthByteVector(fuzzed_data_provider);
     if (data.empty()) {
-        data.resize(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, 4096), fuzzed_data_provider.ConsumeIntegral<uint8_t>());
+        auto new_size = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, 4096);
+        auto x = fuzzed_data_provider.ConsumeIntegral<uint8_t>();
+        data.resize(new_size, x);
     }
 
     CHash160 hash160;
@@ -44,7 +46,9 @@ FUZZ_TARGET(crypto)
                 if (fuzzed_data_provider.ConsumeBool()) {
                     data = ConsumeRandomLengthByteVector(fuzzed_data_provider);
                     if (data.empty()) {
-                        data.resize(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, 4096), fuzzed_data_provider.ConsumeIntegral<uint8_t>());
+                        auto new_size = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, 4096);
+                        auto x = fuzzed_data_provider.ConsumeIntegral<uint8_t>();
+                        data.resize(new_size, x);
                     }
                 }
 

src/test/fuzz/crypto_chacha20.cpp

@@ -28,11 +28,10 @@ FUZZ_TARGET(crypto_chacha20)
                 chacha20.SetKey(key);
             },
             [&] {
-                chacha20.Seek(
-                    {
-                        fuzzed_data_provider.ConsumeIntegral<uint32_t>(),
-                        fuzzed_data_provider.ConsumeIntegral<uint64_t>()
-                    }, fuzzed_data_provider.ConsumeIntegral<uint32_t>());
+                ChaCha20::Nonce96 nonce{
+                    fuzzed_data_provider.ConsumeIntegral<uint32_t>(),
+                    fuzzed_data_provider.ConsumeIntegral<uint64_t>()};
+                chacha20.Seek(nonce, fuzzed_data_provider.ConsumeIntegral<uint32_t>());
             },
             [&] {
                 std::vector<uint8_t> output(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096));

src/test/fuzz/cuckoocache.cpp

@@ -41,7 +41,9 @@ FUZZ_TARGET(cuckoocache)
         if (fuzzed_data_provider.ConsumeBool()) {
             cuckoo_cache.insert(fuzzed_data_provider.ConsumeBool());
         } else {
-            cuckoo_cache.contains(fuzzed_data_provider.ConsumeBool(), fuzzed_data_provider.ConsumeBool());
+            auto e = fuzzed_data_provider.ConsumeBool();
+            auto erase = fuzzed_data_provider.ConsumeBool();
+            cuckoo_cache.contains(e, erase);
         }
     }
     fuzzed_data_provider_ptr = nullptr;

src/test/fuzz/message.cpp

@@ -39,7 +39,9 @@ FUZZ_TARGET(message, .init = initialize_message)
     }
     {
         (void)MessageHash(random_message);
-        (void)MessageVerify(fuzzed_data_provider.ConsumeRandomLengthString(1024), fuzzed_data_provider.ConsumeRandomLengthString(1024), random_message);
+        auto address = fuzzed_data_provider.ConsumeRandomLengthString(1024);
+        auto signature = fuzzed_data_provider.ConsumeRandomLengthString(1024);
+        (void)MessageVerify(address, signature, random_message);
         (void)SigningResultString(fuzzed_data_provider.PickValueInArray({SigningResult::OK, SigningResult::PRIVATE_KEY_NOT_AVAILABLE, SigningResult::SIGNING_FAILED}));
     }
 }

src/test/fuzz/policy_estimator.cpp

@@ -85,9 +85,18 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator)
             });
         (void)block_policy_estimator.estimateFee(fuzzed_data_provider.ConsumeIntegral<int>());
         EstimationResult result;
-        (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);
+        auto conf_target = fuzzed_data_provider.ConsumeIntegral<int>();
+        auto success_threshold = fuzzed_data_provider.ConsumeFloatingPoint<double>();
+        auto horizon = fuzzed_data_provider.PickValueInArray(ALL_FEE_ESTIMATE_HORIZONS);
+        auto* result_ptr = fuzzed_data_provider.ConsumeBool() ? &result : nullptr;
+        (void)block_policy_estimator.estimateRawFee(conf_target, success_threshold, horizon, result_ptr);
+
         FeeCalculation fee_calculation;
-        (void)block_policy_estimator.estimateSmartFee(fuzzed_data_provider.ConsumeIntegral<int>(), fuzzed_data_provider.ConsumeBool() ? &fee_calculation : nullptr, fuzzed_data_provider.ConsumeBool());
+        conf_target = fuzzed_data_provider.ConsumeIntegral<int>();
+        auto* fee_calc_ptr = fuzzed_data_provider.ConsumeBool() ? &fee_calculation : nullptr;
+        auto conservative = fuzzed_data_provider.ConsumeBool();
+        (void)block_policy_estimator.estimateSmartFee(conf_target, fee_calc_ptr, conservative);
+
         (void)block_policy_estimator.HighestTargetTracked(fuzzed_data_provider.PickValueInArray(ALL_FEE_ESTIMATE_HORIZONS));
     }
     {

src/test/fuzz/prevector.cpp

@@ -210,15 +210,20 @@ FUZZ_TARGET(prevector)
     LIMITED_WHILE(prov.remaining_bytes(), 3000)
     {
         switch (prov.ConsumeIntegralInRange<int>(0, 13 + 3 * (test.size() > 0))) {
-        case 0:
-            test.insert(prov.ConsumeIntegralInRange<size_t>(0, test.size()), prov.ConsumeIntegral<int>());
-            break;
+        case 0: {
+            auto position = prov.ConsumeIntegralInRange<size_t>(0, test.size());
+            auto value = prov.ConsumeIntegral<int>();
+            test.insert(position, value);
+        } break;
         case 1:
             test.resize(std::max(0, std::min(30, (int)test.size() + prov.ConsumeIntegralInRange<int>(0, 4) - 2)));
             break;
-        case 2:
-            test.insert(prov.ConsumeIntegralInRange<size_t>(0, test.size()), 1 + prov.ConsumeBool(), prov.ConsumeIntegral<int>());
-            break;
+        case 2: {
+            auto position = prov.ConsumeIntegralInRange<size_t>(0, test.size());
+            auto count = 1 + prov.ConsumeBool();
+            auto value = prov.ConsumeIntegral<int>();
+            test.insert(position, count, value);
+        } break;
         case 3: {
             int del = prov.ConsumeIntegralInRange<int>(0, test.size());
             int beg = prov.ConsumeIntegralInRange<int>(0, test.size() - del);
@@ -255,9 +260,11 @@ FUZZ_TARGET(prevector)
         case 9:
             test.clear();
             break;
-        case 10:
-            test.assign(prov.ConsumeIntegralInRange<size_t>(0, 32767), prov.ConsumeIntegral<int>());
-            break;
+        case 10: {
+            auto n = prov.ConsumeIntegralInRange<size_t>(0, 32767);
+            auto value = prov.ConsumeIntegral<int>();
+            test.assign(n, value);
+        } break;
         case 11:
             test.swap();
             break;
@@ -267,9 +274,11 @@ FUZZ_TARGET(prevector)
         case 13:
             test.move();
             break;
-        case 14:
-            test.update(prov.ConsumeIntegralInRange<size_t>(0, test.size() - 1), prov.ConsumeIntegral<int>());
-            break;
+        case 14: {
+            auto pos = prov.ConsumeIntegralInRange<size_t>(0, test.size() - 1);
+            auto value = prov.ConsumeIntegral<int>();
+            test.update(pos, value);
+        } break;
         case 15:
             test.erase(prov.ConsumeIntegralInRange<size_t>(0, test.size() - 1));
             break;

src/test/fuzz/script_format.cpp

@@ -30,5 +30,7 @@ FUZZ_TARGET(script_format, .init = initialize_script_format)
     (void)ScriptToAsmStr(script, /*fAttemptSighashDecode=*/fuzzed_data_provider.ConsumeBool());
 
     UniValue o1(UniValue::VOBJ);
-    ScriptToUniv(script, /*out=*/o1, /*include_hex=*/fuzzed_data_provider.ConsumeBool(), /*include_address=*/fuzzed_data_provider.ConsumeBool());
+    auto include_hex = fuzzed_data_provider.ConsumeBool();
+    auto include_address = fuzzed_data_provider.ConsumeBool();
+    ScriptToUniv(script, /*out=*/o1, include_hex, include_address);
 }

src/test/fuzz/script_interpreter.cpp

@@ -25,12 +25,18 @@ FUZZ_TARGET(script_interpreter)
             const CTransaction tx_to{*mtx};
             const unsigned int in = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
             if (in < tx_to.vin.size()) {
-                (void)SignatureHash(script_code, tx_to, in, fuzzed_data_provider.ConsumeIntegral<int>(), ConsumeMoney(fuzzed_data_provider), fuzzed_data_provider.PickValueInArray({SigVersion::BASE, SigVersion::WITNESS_V0}), nullptr);
+                auto n_hash_type = fuzzed_data_provider.ConsumeIntegral<int>();
+                auto amount = ConsumeMoney(fuzzed_data_provider);
+                auto sigversion = fuzzed_data_provider.PickValueInArray({SigVersion::BASE, SigVersion::WITNESS_V0});
+                (void)SignatureHash(script_code, tx_to, in, n_hash_type, amount, sigversion, nullptr);
                 const std::optional<CMutableTransaction> mtx_precomputed = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider, TX_WITH_WITNESS);
                 if (mtx_precomputed) {
                     const CTransaction tx_precomputed{*mtx_precomputed};
                     const PrecomputedTransactionData precomputed_transaction_data{tx_precomputed};
-                    (void)SignatureHash(script_code, tx_to, in, fuzzed_data_provider.ConsumeIntegral<int>(), ConsumeMoney(fuzzed_data_provider), fuzzed_data_provider.PickValueInArray({SigVersion::BASE, SigVersion::WITNESS_V0}), &precomputed_transaction_data);
+                    n_hash_type = fuzzed_data_provider.ConsumeIntegral<int>();
+                    amount = ConsumeMoney(fuzzed_data_provider);
+                    sigversion = fuzzed_data_provider.PickValueInArray({SigVersion::BASE, SigVersion::WITNESS_V0});
+                    (void)SignatureHash(script_code, tx_to, in, n_hash_type, amount, sigversion, &precomputed_transaction_data);
                 }
             }
         }

src/test/fuzz/script_sign.cpp

@@ -111,7 +111,10 @@ FUZZ_TARGET(script_sign, .init = initialize_script_sign)
             }
             if (n_in < script_tx_to.vin.size()) {
                 SignatureData empty;
-                (void)SignSignature(provider, ConsumeScript(fuzzed_data_provider), script_tx_to, n_in, ConsumeMoney(fuzzed_data_provider), fuzzed_data_provider.ConsumeIntegral<int>(), empty);
+                auto from_pub_key = ConsumeScript(fuzzed_data_provider);
+                auto amount = ConsumeMoney(fuzzed_data_provider);
+                auto n_hash_type = fuzzed_data_provider.ConsumeIntegral<int>();
+                (void)SignSignature(provider, from_pub_key, script_tx_to, n_in, amount, n_hash_type, empty);
                 MutableTransactionSignatureCreator signature_creator{tx_to, n_in, ConsumeMoney(fuzzed_data_provider), fuzzed_data_provider.ConsumeIntegral<int>()};
                 std::vector<unsigned char> vch_sig;
                 CKeyID address;
@@ -122,7 +125,9 @@ FUZZ_TARGET(script_sign, .init = initialize_script_sign)
                 } else {
                     address = CKeyID{ConsumeUInt160(fuzzed_data_provider)};
                 }
-                (void)signature_creator.CreateSig(provider, vch_sig, address, ConsumeScript(fuzzed_data_provider), fuzzed_data_provider.PickValueInArray({SigVersion::BASE, SigVersion::WITNESS_V0}));
+                auto script_code = ConsumeScript(fuzzed_data_provider);
+                auto sigversion = fuzzed_data_provider.PickValueInArray({SigVersion::BASE, SigVersion::WITNESS_V0});
+                (void)signature_creator.CreateSig(provider, vch_sig, address, script_code, sigversion);
             }
             std::map<COutPoint, Coin> coins{ConsumeCoins(fuzzed_data_provider)};
             std::map<int, bilingual_str> input_errors;

src/test/fuzz/socks5.cpp

@@ -41,8 +41,8 @@ FUZZ_TARGET(socks5, .init = initialize_socks5)
     FuzzedSock fuzzed_sock = ConsumeSock(fuzzed_data_provider);
     // This Socks5(...) fuzzing harness would have caught CVE-2017-18350 within
     // a few seconds of fuzzing.
-    (void)Socks5(fuzzed_data_provider.ConsumeRandomLengthString(512),
-                 fuzzed_data_provider.ConsumeIntegral<uint16_t>(),
-                 fuzzed_data_provider.ConsumeBool() ? &proxy_credentials : nullptr,
-                 fuzzed_sock);
+    auto str_dest = fuzzed_data_provider.ConsumeRandomLengthString(512);
+    auto port = fuzzed_data_provider.ConsumeIntegral<uint16_t>();
+    auto* auth = fuzzed_data_provider.ConsumeBool() ? &proxy_credentials : nullptr;
+    (void)Socks5(str_dest, port, auth, fuzzed_sock);
 }