tests: Add fuzzing harness for ProcessMessage(…). Enables high-level fuzzing of the P2P layer. #17989

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:fuzzers-net-process_message changing 4 files +277 −3
  1. practicalswift commented at 12:22 pm on January 23, 2020: contributor

    Add fuzzing harness for ProcessMessage(...). Enables high-level fuzzing of the P2P layer.

    All code paths reachable from this fuzzer can be assumed to be reachable for an untrusted peer.

    Seeded from thin air (an empty corpus) this fuzzer reaches roughly 20 000 lines of code.

    To test this PR:

    0$ make distclean
    1$ ./autogen.sh
    2$ CC=clang CXX=clang++ ./configure --enable-fuzz \
    3      --with-sanitizers=address,fuzzer,undefined
    4$ make
    5$ src/test/fuzz/process_message
    6

    Worth noting about this fuzzing harness:

    • To achieve a reasonable number of executions per seconds the state of the fuzzer is unfortunately not entirely reset between test_one_input calls. The set-up (FuzzingSetup ctor) and tear-down (~FuzzingSetup) work is simply too costly to be run on every iteration. There is a trade-off to handle here between a.) achieving high executions/second and b.) giving the fuzzer a totally blank slate for each call. Please let me know if you have any suggestion on how to improve this situation while maintaining >1000 executions/second.
    • To achieve optimal results when using coverage-guided fuzzing I’ve chosen to create one specialised fuzzing binary per message type (process_message_addr, process_message_block, process_message_blocktxn , etc.) and one general fuzzing binary (process_message) which handles all messages types. The latter general fuzzer can be seeded with inputs generated by the former specialised fuzzers.

    Happy fuzzing friends!

  2. fanquake added the label Tests on Jan 23, 2020
  3. practicalswift force-pushed on Jan 23, 2020
  4. DrahtBot commented at 2:47 pm on January 23, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18134 (Replace std::to_string with locale-independent alternative by Empact)
    • #17997 (refactor: Remove mempool global from net by MarcoFalke)
    • #16442 (Serve BIP 157 compact filters by jimpo)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  5. practicalswift force-pushed on Jan 24, 2020
  6. laanwj commented at 4:42 pm on February 10, 2020: member
    code review ACK 1b67435bccb8e159130ee8dc558c039cbcc5767c
  7. practicalswift commented at 6:36 pm on February 10, 2020: contributor
    @laanwj Thanks for reviewing. Pushed a commit which removes the tinyformat dependency from ToString(…). Please re-review :)
  8. practicalswift force-pushed on Mar 9, 2020
  9. practicalswift commented at 3:19 pm on March 9, 2020: contributor
    Rebased! :)
  10. practicalswift force-pushed on Mar 9, 2020
  11. DrahtBot added the label Needs rebase on Mar 9, 2020
  12. practicalswift force-pushed on Mar 9, 2020
  13. practicalswift commented at 7:27 pm on March 9, 2020: contributor
    Rebased :)
  14. DrahtBot removed the label Needs rebase on Mar 9, 2020
  15. in src/test/fuzz/process_message.cpp:110 in fadf53c5b2 outdated
    105+    oss.imbue(std::locale::classic());
    106+    oss << t;
    107+    return oss.str();
    108+}
    109+
    110+class FuzzingSetup
    


    MarcoFalke commented at 3:50 pm on March 10, 2020:
    Why is this needed and why does it reimplement stuff that could be inherited from RegTestingSetup?

    practicalswift commented at 4:22 pm on March 10, 2020:
    The setup requirements for the fuzzers are a bit different from the requirements for the unit tests. To name one thing I wanted FastRandomContext().rand64() instead of g_insecure_rand_ctx_temp_path.rand32() to guarantee unique directory names. I ran in to a few such issues until I gave up on inheriting from RegTestingSetup which was my initial plan :)

    MarcoFalke commented at 5:39 pm on March 10, 2020:

    Sorry, I don’t understand.

    • The fuzzing setup here is a single global, so it is one datadir per process that does not change. Assuming 60 processes or so, 32 bits of randomness should be sufficient. Though, feel free to change it to 256 bits of randomness.

    • Why are the requirements different? There are different *TestingSetups to accommodate for different needs, but they are mostly inheriting from each other. Why is this not possible for FuzzingSetup? I can replace it with a plain RegTestingSetup and everything works just fine. Please explain the issues you are running into.

      0diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
      1index 3048b09564..ffb7a78b5b 100644
      2--- a/src/test/fuzz/process_message.cpp
      3+++ b/src/test/fuzz/process_message.cpp
      4@@ -28,6 +28,7 @@
      5 #include <test/fuzz/FuzzedDataProvider.h>
      6 #include <test/fuzz/fuzz.h>
      7 #include <test/util/setup_common.h>
      8+#include <test/util/mining.h>
      9 #include <txdb.h>
     10 #include <txmempool.h>
     11 #include <util/memory.h>
     12@@ -58,23 +59,6 @@ bool ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vR
     13 
     14 namespace {
     15 
     16-size_t GetNumberOfScheduledTasks(const CScheduler& scheduler)
     17-{
     18-    std::chrono::system_clock::time_point first, last;
     19-    return scheduler.getQueueInfo(first, last);
     20-}
     21-
     22-void SleepUntilSchedulerCompletion(const CScheduler& scheduler, const size_t consider_completed_at_size = 0)
     23-{
     24-    for (int i = 0; i < 10; ++i) {
     25-        if (GetNumberOfScheduledTasks(scheduler) <= consider_completed_at_size) {
     26-            return;
     27-        }
     28-        UninterruptibleSleep(std::chrono::milliseconds{10});
     29-    }
     30-    assert(false);
     31-}
     32-
     33 #ifdef MESSAGE_TYPE
     34 #define TO_STRING_(s) #s
     35 #define TO_STRING(s) TO_STRING_(s)
     36@@ -83,94 +67,6 @@ const std::string LIMIT_TO_MESSAGE_TYPE{TO_STRING(MESSAGE_TYPE)};
     37 const std::string LIMIT_TO_MESSAGE_TYPE;
     38 #endif
     39 
     40-void CreateAndProcessNextBlock(const CTxMemPool& mempool)
     41-{
     42-    std::unique_ptr<CBlockTemplate> block_template = BlockAssembler(mempool, Params()).CreateNewBlock(CScript() << OP_TRUE);
     43-    CBlock& block = block_template->block;
     44-    {
     45-        LOCK(cs_main);
     46-        unsigned int extra_nonce;
     47-        IncrementExtraNonce(&block, ChainActive().Tip(), extra_nonce);
     48-    }
     49-    while (!CheckProofOfWork(block.GetHash(), block.nBits, Params().GetConsensus())) {
     50-        ++block.nNonce;
     51-    }
     52-    ProcessNewBlock(Params(), std::make_shared<const CBlock>(block), true, nullptr);
     53-}
     54-
     55-template <typename T>
     56-std::string ToString(const T t)
     57-{
     58-    std::ostringstream oss;
     59-    oss.imbue(std::locale::classic());
     60-    oss << t;
     61-    return oss.str();
     62-}
     63-
     64-class FuzzingSetup
     65-{
     66-    boost::thread_group m_thread_group;
     67-    const fs::path m_path;
     68-    std::unique_ptr<PeerLogicValidation> m_peer_logic_validation;
     69-
     70-public:
     71-    CScheduler m_scheduler;
     72-    NodeContext m_node_context;
     73-    std::unique_ptr<CNode> m_dummy_p2p_node;
     74-
     75-    FuzzingSetup(const std::string& fuzzer_name) : m_path{fs::temp_directory_path() / "fuzzers" / fuzzer_name / ToString(FastRandomContext().rand64())}
     76-    {
     77-        SelectParams(CBaseChainParams::REGTEST);
     78-        fs::remove_all(m_path);
     79-        fs::create_directories(m_path);
     80-        ::gArgs.ForceSetArg("-datadir", m_path.string());
     81-        InitLogging();
     82-        LogInstance().m_print_to_console = false;
     83-        LogInstance().StartLogging();
     84-        ::g_chainstate = MakeUnique<CChainState>();
     85-        ::pblocktree = MakeUnique<CBlockTreeDB>(1 << 20, true);
     86-        m_dummy_p2p_node = MakeUnique<CNode>(0, ServiceFlags(NODE_NETWORK | NODE_WITNESS | NODE_BLOOM), 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, false);
     87-        m_dummy_p2p_node->fSuccessfullyConnected = true;
     88-        m_dummy_p2p_node->nVersion = PROTOCOL_VERSION;
     89-        m_dummy_p2p_node->SetSendVersion(PROTOCOL_VERSION);
     90-        m_node_context.mempool = &::mempool;
     91-        m_node_context.mempool->setSanityCheck(1.0);
     92-        m_node_context.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
     93-        m_node_context.connman = MakeUnique<CConnman>(0x1337, 0x1337);
     94-        m_thread_group.create_thread(std::bind(&CScheduler::serviceQueue, &m_scheduler));
     95-        GetMainSignals().RegisterBackgroundSignalScheduler(m_scheduler);
     96-        ChainstateActive().InitCoinsDB(1 << 23, true, false);
     97-        ChainstateActive().InitCoinsCache();
     98-        if (!LoadGenesisBlock(Params())) {
     99-            throw std::runtime_error("LoadGenesisBlock(...) failed.");
    100-        }
    101-        BlockValidationState block_validation_state;
    102-        if (!ActivateBestChain(block_validation_state, Params())) {
    103-            throw std::runtime_error("ActivateBestChain(...) failed.");
    104-        }
    105-        for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
    106-            CreateAndProcessNextBlock(*m_node_context.mempool);
    107-            SleepUntilSchedulerCompletion(m_scheduler);
    108-        }
    109-        assert(ChainActive().Tip() != nullptr);
    110-        m_peer_logic_validation = MakeUnique<PeerLogicValidation>(m_node_context.connman.get(), nullptr, m_scheduler);
    111-        m_peer_logic_validation->InitializeNode(m_dummy_p2p_node.get());
    112-        assert(GetMainSignals().CallbacksPending() == 0);
    113-        SleepUntilSchedulerCompletion(m_scheduler, 1);
    114-        LogInstance().m_print_to_console = true;
    115-    }
    116-
    117-    ~FuzzingSetup()
    118-    {
    119-        LogInstance().DisconnectTestLogger();
    120-        fs::remove_all(m_path);
    121-        m_thread_group.interrupt_all();
    122-        m_thread_group.join_all();
    123-        GetMainSignals().FlushBackgroundCallbacks();
    124-        GetMainSignals().UnregisterBackgroundSignalScheduler();
    125-    }
    126-};
    127-
    128 const std::map<std::string, std::set<std::string>> EXPECTED_DESERIALIZATION_EXCEPTIONS = {
    129     {"CDataStream::read(): end of data: iostream error", {"addr", "block", "blocktxn", "cmpctblock", "feefilter", "filteradd", "filterload", "getblocks", "getblocktxn", "getdata", "getheaders", "headers", "inv", "notfound", "ping", "sendcmpct", "tx"}},
    130     {"index overflowed 16 bits: iostream error", {"getblocktxn"}},
    131@@ -182,12 +78,20 @@ const std::map<std::string, std::set<std::string>> EXPECTED_DESERIALIZATION_EXCE
    132     {"Unknown transaction optional data: iostream error", {"block", "blocktxn", "cmpctblock", "tx"}},
    133 };
    134 
    135-std::unique_ptr<FuzzingSetup> g_fuzzing_setup;
    136+std::unique_ptr<RegTestingSetup> g_fuzzing_setup;
    137 } // namespace
    138 
    139 void initialize()
    140 {
    141-    g_fuzzing_setup = MakeUnique<FuzzingSetup>("process_message-" + (LIMIT_TO_MESSAGE_TYPE.empty() ? "all" : LIMIT_TO_MESSAGE_TYPE));
    142+    g_fuzzing_setup = MakeUnique<RegTestingSetup>();
    143+
    144+    for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
    145+        MineBlock(g_fuzzing_setup->m_node, CScript() << OP_TRUE);
    146+    }
    147+    SyncWithValidationInterfaceQueue();
    148+    assert(ChainActive().Tip() != nullptr);
    149+    assert(GetMainSignals().CallbacksPending() == 0);
    150+    LogInstance().m_print_to_console = true;
    151 }
    152 
    153 void test_one_input(const std::vector<uint8_t>& buffer)
    154@@ -206,9 +110,13 @@ void test_one_input(const std::vector<uint8_t>& buffer)
    155         return;
    156     }
    157     CDataStream random_bytes_data_stream{fuzzed_data_provider.ConsumeRemainingBytes<unsigned char>(), SER_NETWORK, PROTOCOL_VERSION};
    158-    CNode& p2p_node = *g_fuzzing_setup->m_dummy_p2p_node;
    159+    CNode p2p_node{0, ServiceFlags(NODE_NETWORK | NODE_WITNESS | NODE_BLOOM), 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, false};
    160+    p2p_node.fSuccessfullyConnected = true;
    161+    p2p_node.nVersion = PROTOCOL_VERSION;
    162+    p2p_node.SetSendVersion(PROTOCOL_VERSION);
    163+    g_fuzzing_setup->m_node.peer_logic->InitializeNode(&p2p_node);
    164     try {
    165-        (void)ProcessMessage(&p2p_node, random_message_type, random_bytes_data_stream, GetTimeMillis(), Params(), g_fuzzing_setup->m_node_context.connman.get(), g_fuzzing_setup->m_node_context.banman.get(), std::atomic<bool>{false});
    166+        (void)ProcessMessage(&p2p_node, random_message_type, random_bytes_data_stream, GetTimeMillis(), Params(), g_fuzzing_setup->m_node.connman.get(), g_fuzzing_setup->m_node.banman.get(), std::atomic<bool>{false});
    167     } catch (const std::ios_base::failure& e) {
    168         const std::string exception_message{e.what()};
    169         const auto p = EXPECTED_DESERIALIZATION_EXCEPTIONS.find(exception_message);
    170@@ -219,18 +127,6 @@ void test_one_input(const std::vector<uint8_t>& buffer)
    171     } catch (...) {
    172         assert(false);
    173     }
    174-    p2p_node.fDisconnect = false;
    175-    p2p_node.fPauseSend = false;
    176-    p2p_node.nSendSize = 0;
    177-    {
    178-        LOCK(p2p_node.cs_inventory);
    179-        p2p_node.vInventoryBlockToSend.clear();
    180-    }
    181-    p2p_node.vRecvGetData.clear();
    182-    {
    183-        LOCK(p2p_node.cs_vSend);
    184-        p2p_node.vSendMsg.clear();
    185-    }
    186     assert(GetMainSignals().CallbacksPending() == 0);
    187-    SleepUntilSchedulerCompletion(g_fuzzing_setup->m_scheduler, 1);
    188+    SyncWithValidationInterfaceQueue();
    189 }
    190diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
    191index 53eb9ff43b..7fdbbdfd1d 100644
    192--- a/src/test/util/setup_common.cpp
    193+++ b/src/test/util/setup_common.cpp
    194@@ -7,6 +7,7 @@
    195 #include <banman.h>
    196 #include <chainparams.h>
    197 #include <consensus/consensus.h>
    198+#include <net_processing.h>
    199 #include <consensus/params.h>
    200 #include <consensus/validation.h>
    201 #include <crypto/sha256.h>
    202@@ -62,7 +63,7 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)
    203 }
    204 
    205 BasicTestingSetup::BasicTestingSetup(const std::string& chainName)
    206-    : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / std::to_string(g_insecure_rand_ctx_temp_path.rand32())}
    207+    : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString()}
    208 {
    209     fs::create_directories(m_path_root);
    210     gArgs.ForceSetArg("-datadir", m_path_root.string());
    211@@ -136,6 +137,7 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
    212     m_node.mempool->setSanityCheck(1.0);
    213     m_node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
    214     m_node.connman = MakeUnique<CConnman>(0x1337, 0x1337); // Deterministic randomness for tests.
    215+    m_node.peer_logic = MakeUnique<PeerLogicValidation>(m_node.connman.get(), m_node.banman.get(), *m_node.scheduler);
    216 }
    217 
    218 TestingSetup::~TestingSetup()
    

    MarcoFalke commented at 5:41 pm on March 10, 2020:
    100 lines of undocumented code without a given reason are even too much for unit or fuzz tests. This makes it harder to understand and change the tests in the future.

    practicalswift commented at 6:11 pm on March 10, 2020:

    The fuzzing setup here is a single global, so it is one datadir per process that does not change. Assuming 60 processes or so, 32 bits of randomness should be sufficient.

    Oh, true. I experimented with having the fuzzing setup non-global to start with (one setup per test_one_input). That’s probably when FastRandomContext().rand64() was thought to be needed IIRC.

    I’ll try your patch: if we can get it working with RegTestingSetup then that’s great :)


    practicalswift commented at 6:38 pm on March 10, 2020:
    Updated. Now using RegTestingSetup. Thanks for the patch!
  16. in src/test/fuzz/process_message.cpp:185 in fadf53c5b2 outdated
    180+    {"ReadCompactSize(): size too large: iostream error", {"addr", "block", "blocktxn", "cmpctblock", "filteradd", "filterload", "getblocks", "getblocktxn", "getdata", "getheaders", "headers", "inv", "notfound", "tx"}},
    181+    {"Superfluous witness record: iostream error", {"block", "blocktxn", "cmpctblock", "tx"}},
    182+    {"Unknown transaction optional data: iostream error", {"block", "blocktxn", "cmpctblock", "tx"}},
    183+};
    184+
    185+std::unique_ptr<FuzzingSetup> fuzzing_setup;
    


    MarcoFalke commented at 3:54 pm on March 10, 2020:
    0std::unique_ptr<FuzzingSetup> g_fuzzing_setup;
    

    This should be prefixed with g_ to convey this is global state that is shared between fuzz calls and might lead to non-determinism.


    practicalswift commented at 4:05 pm on March 10, 2020:
    Good point! Fixed!
  17. MarcoFalke commented at 3:55 pm on March 10, 2020: member
    Concept ACK. Is there a reason to both allow all message types and then add some fuzzers that only allow one message type?
  18. practicalswift force-pushed on Mar 10, 2020
  19. practicalswift force-pushed on Mar 10, 2020
  20. practicalswift force-pushed on Mar 10, 2020
  21. in src/test/fuzz/process_message.cpp:36 in 3e2185cbae outdated
    31+#include <test/util/mining.h>
    32+#include <txdb.h>
    33+#include <txmempool.h>
    34+#include <util/memory.h>
    35+#include <util/system.h>
    36+#include <util/time.h>
    


    MarcoFalke commented at 6:29 pm on March 10, 2020:
    Unused?

    practicalswift commented at 7:22 pm on March 10, 2020:
    Thanks. Purged unused includes.
  22. in src/test/fuzz/process_message.cpp:15 in 3e2185cbae outdated
    10+#include <consensus/validation.h>
    11+#include <fs.h>
    12+#include <init.h>
    13+#include <key.h>
    14+#include <logging.h>
    15+#include <miner.h>
    


    MarcoFalke commented at 6:29 pm on March 10, 2020:
    Unused?
  23. in src/test/fuzz/process_message.cpp:13 in 3e2185cbae outdated
     8+#include <consensus/consensus.h>
     9+#include <consensus/params.h>
    10+#include <consensus/validation.h>
    11+#include <fs.h>
    12+#include <init.h>
    13+#include <key.h>
    


    MarcoFalke commented at 6:30 pm on March 10, 2020:
    Unused?
  24. in src/test/fuzz/process_message.cpp:20 in 3e2185cbae outdated
    15+#include <miner.h>
    16+#include <net.h>
    17+#include <net_processing.h>
    18+#include <node/context.h>
    19+#include <pow.h>
    20+#include <primitives/block.h>
    


    MarcoFalke commented at 6:31 pm on March 10, 2020:
    Unused?
  25. in src/test/fuzz/process_message.cpp:27 in 3e2185cbae outdated
    22+#include <random.h>
    23+#include <scheduler.h>
    24+#include <script/script.h>
    25+#include <script/sigcache.h>
    26+#include <streams.h>
    27+#include <sync.h>
    


    MarcoFalke commented at 6:31 pm on March 10, 2020:
    Unused?
  26. in src/test/fuzz/process_message.cpp:33 in 3e2185cbae outdated
    28+#include <test/fuzz/FuzzedDataProvider.h>
    29+#include <test/fuzz/fuzz.h>
    30+#include <test/util/setup_common.h>
    31+#include <test/util/mining.h>
    32+#include <txdb.h>
    33+#include <txmempool.h>
    


    MarcoFalke commented at 6:31 pm on March 10, 2020:
    Unused?
  27. in src/test/fuzz/process_message.cpp:33 in 3e2185cbae outdated
    51+#include <map>
    52+#include <memory>
    53+#include <set>
    54+#include <stdexcept>
    55+#include <string>
    56+#include <vector>
    


    MarcoFalke commented at 6:33 pm on March 10, 2020:
    Unused except for map, set, assert and string?
  28. in src/test/fuzz/process_message.cpp:77 in 3e2185cbae outdated
    100+{
    101+    if (buffer.size() <= CMessageHeader::COMMAND_SIZE) {
    102+        return;
    103+    }
    104+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
    105+    const std::string random_message_type{fuzzed_data_provider.ConsumeBytesAsString(CMessageHeader::COMMAND_SIZE).c_str()};
    


    MarcoFalke commented at 6:35 pm on March 10, 2020:
    0    const std::string random_message_type{fuzzed_data_provider.ConsumeBytesAsString(CMessageHeader::COMMAND_SIZE)};
    

    practicalswift commented at 7:54 pm on March 10, 2020:
    This was intentional to truncate at first \0 :)
  29. in src/test/fuzz/process_message.cpp:82 in 3e2185cbae outdated
    104+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
    105+    const std::string random_message_type{fuzzed_data_provider.ConsumeBytesAsString(CMessageHeader::COMMAND_SIZE).c_str()};
    106+    if (LIMIT_TO_MESSAGE_TYPE.empty()) {
    107+        const std::vector<std::string>& valid_message_types = getAllNetMessageTypes();
    108+        if (std::find(valid_message_types.begin(), valid_message_types.end(), random_message_type) == valid_message_types.end()) {
    109+            return;
    


    MarcoFalke commented at 6:36 pm on March 10, 2020:
    why? Bitcoin Core should just drop the message on the floor itself

    practicalswift commented at 7:54 pm on March 10, 2020:
    This was intentional to maximize fuzzer throughput :)

    practicalswift commented at 8:08 pm on March 10, 2020:
    Perhaps it was a premature optimization. Skipping!
  30. in src/test/fuzz/process_message.cpp:69 in 3e2185cbae outdated
    91+        MineBlock(g_fuzzing_setup->m_node, CScript() << OP_TRUE);
    92+    }
    93+    SyncWithValidationInterfaceQueue();
    94+    assert(ChainActive().Tip() != nullptr);
    95+    assert(GetMainSignals().CallbacksPending() == 0);
    96+    LogInstance().m_print_to_console = true;
    


    MarcoFalke commented at 6:39 pm on March 10, 2020:

    why? Seems a bit verbose to have everything interleaved with the libfuzzer log.

    For debugging a specific failure, this can always be enabled.


    practicalswift commented at 8:08 pm on March 10, 2020:
    Makes sense. Now skipping.
  31. in src/test/util/setup_common.cpp:10 in 3e2185cbae outdated
     6@@ -7,6 +7,7 @@
     7 #include <banman.h>
     8 #include <chainparams.h>
     9 #include <consensus/consensus.h>
    10+#include <net_processing.h>
    


    MarcoFalke commented at 6:43 pm on March 10, 2020:
    pls sort

    practicalswift commented at 7:23 pm on March 10, 2020:
    Done!
  32. in src/test/fuzz/process_message.cpp:130 in 3e2185cbae outdated
    125+        if (p == EXPECTED_DESERIALIZATION_EXCEPTIONS.cend() || p->second.count(random_message_type) == 0) {
    126+            std::cout << "Unexpected exception when processing message type \"" << random_message_type << "\": " << exception_message << std::endl;
    127+            assert(false);
    128+        }
    129+    } catch (...) {
    130+        assert(false);
    


    MarcoFalke commented at 6:44 pm on March 10, 2020:

    Why?

    This will erase the whole bt

  33. in src/test/fuzz/process_message.cpp:95 in 3e2185cbae outdated
    90+    for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
    91+        MineBlock(g_fuzzing_setup->m_node, CScript() << OP_TRUE);
    92+    }
    93+    SyncWithValidationInterfaceQueue();
    94+    assert(ChainActive().Tip() != nullptr);
    95+    assert(GetMainSignals().CallbacksPending() == 0);
    


    MarcoFalke commented at 6:46 pm on March 10, 2020:
    Is this needed, given that we synced with the queue?
  34. in src/test/fuzz/process_message.cpp:103 in 3e2185cbae outdated
    127+            assert(false);
    128+        }
    129+    } catch (...) {
    130+        assert(false);
    131+    }
    132+    assert(GetMainSignals().CallbacksPending() == 0);
    


    MarcoFalke commented at 6:46 pm on March 10, 2020:
    Is this needed, given that we synced with the queue?
  35. MarcoFalke approved
  36. MarcoFalke commented at 6:47 pm on March 10, 2020: member

    ACK 4d4f38ee5a 🔒

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK 4d4f38ee5a 🔒
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUjV/gwAyGMYagoM2OCSoUvGaS0o3rBEIIUJ6lt8R/i91LUEuc46NitcJk/GH61o
     8fH0lnYi3lmLxRxk2BUvwxY7aS+0lHExqcz/O2VBd7A7A8iqQ4TVUcMibXcJMdNrO
     9C3b+8ndKv18XuYvSaoSWOAG1E9smnB824klvz6gQoF45Ku6qgAd1b/05D85lQf1D
    108t5ovpvjN6et1jWoRR5VTL6UeUdtDcwM/HatJ/S6hHqDqbtYlV0HWhqXwt+DqmvY
    116Tl6w2yKRXrvrAaLwSFmD+iEQOYSkiEGJgxchjGuy6lo4rQPUQBCwKSZtOEGobLO
    12qlC1iVhjKHWVflitLkC6u1oW/4domq/ahqkt+PMg9uJMMsMt9L1UtC1t9gqqC7qk
    13LDAQihxRnYF5jWN5Is25/Qp5OsVRQU8VZQAQ8EpxBHVWkHWIloT+lvXVSp0xbNyJ
    14KnwQtKxpZ+cSGqd/8nX4aLwTMXo+v5H9+5DudrZ4gpQLdZKp4RqMOyrwWRTdsCfn
    15vdu0tFx9
    16=aujQ
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 730802cd0481d1a196758386a01e5111c8d68b5c12c7079e969bf05a0cec0a02 -

  37. MarcoFalke commented at 6:53 pm on March 10, 2020: member
    Also, travis is failing
  38. practicalswift commented at 6:59 pm on March 10, 2020: contributor

    Concept ACK. Is there a reason to both allow all message types and then add some fuzzers that only allow one message type?

    Yes there is :)

    1. The reason to allow all message types in one of the fuzzers: this allows auto-detection and thus fuzzing of newly introduced messages types without updating the fuzzer. See #18261 (comment) for a real-world example of that :)

    2. The reason to have also have per-message-type fuzzers is largely for the same reason that we have one fuzzing binary per deserialization: to make it relatively easier for coverage guided fuzzers to reach deep. See sipa’s rationale here and kcc’s confirmation here. Note that @kcc is the one of the libFuzzer authors :)

  39. practicalswift force-pushed on Mar 10, 2020
  40. practicalswift force-pushed on Mar 10, 2020
  41. practicalswift force-pushed on Mar 10, 2020
  42. practicalswift force-pushed on Mar 10, 2020
  43. naumenkogs commented at 8:55 pm on March 10, 2020: member
    Concept ACK. This seems super-useful.
  44. practicalswift commented at 10:06 pm on March 10, 2020: contributor

    @MarcoFalke After applying the suggested patch and switching to RegTestingSetup I’m running in to this issue on fuzzer exit:

     0$ touch dummy_file
     1$ src/test/fuzz/process_message dummy_file
     2process_message: /usr/include/boost/thread/pthread/condition_variable_fwd.hpp:116: boost::condition_variable::~condition_variable(): Assertion `!ret' failed.
     3==12861== ERROR: libFuzzer: deadly signal
     4    [#0](/bitcoin-bitcoin/0/) 0x5654b00081c1 in __sanitizer_print_stack_trace llvm-project/compiler-rt/lib/asan/asan_stack.cpp:86:3
     5    [#1](/bitcoin-bitcoin/1/) 0x5654aff47738 in fuzzer::PrintStackTrace() llvm-project/compiler-rt/lib/fuzzer/FuzzerUtil.cpp:205:5
     6    [#2](/bitcoin-bitcoin/2/) 0x5654aff2b3d6 in fuzzer::Fuzzer::CrashCallback() llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:232:3
     7    [#3](/bitcoin-bitcoin/3/) 0x7f87e1a8e88f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1288f)
     8    [#4](/bitcoin-bitcoin/4/) 0x7f87e06aee96 in __libc_signal_restore_set /build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/nptl-signals.h:80
     9    [#5](/bitcoin-bitcoin/5/) 0x7f87e06aee96 in raise /build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:48
    10    [#6](/bitcoin-bitcoin/6/) 0x7f87e06b0800 in abort /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:79
    11    [#7](/bitcoin-bitcoin/7/) 0x7f87e06a0399 in __assert_fail_base /build/glibc-OTsEL5/glibc-2.27/assert/assert.c:92
    12    [#8](/bitcoin-bitcoin/8/) 0x7f87e06a0411 in __assert_fail /build/glibc-OTsEL5/glibc-2.27/assert/assert.c:101
    13    [#9](/bitcoin-bitcoin/9/) 0x5654b077009f in boost::condition_variable::~condition_variable() /usr/include/boost/thread/pthread/condition_variable_fwd.hpp:116:13
    14    [#10](/bitcoin-bitcoin/10/) 0x5654b0620ef0 in CCheckQueue<CScriptCheck>::~CCheckQueue() src/./checkqueue.h:161:5
    15    [#11](/bitcoin-bitcoin/11/) 0x7f87e06b3040 in __run_exit_handlers /build/glibc-OTsEL5/glibc-2.27/stdlib/exit.c:108
    16    [#12](/bitcoin-bitcoin/12/) 0x7f87e06b3139 in exit /build/glibc-OTsEL5/glibc-2.27/stdlib/exit.c:139
    17    [#13](/bitcoin-bitcoin/13/) 0x5654aff1cf98 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:783:8
    18    [#14](/bitcoin-bitcoin/14/) 0x5654aff47e32 in main llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:19:10
    19    [#15](/bitcoin-bitcoin/15/) 0x7f87e0691b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
    20    [#16](/bitcoin-bitcoin/16/) 0x5654afef0119 in _start (src/test/fuzz/process_message+0x1859119)
    

    Seems like RegTestingSetup is missing some part of the teardown logic I provided in the previous custom setup :)

  45. MarcoFalke commented at 11:19 pm on March 10, 2020: member

    @practicalswift C++ is a shitshow when it comes to static initialization (order). You might have to manually set up everything in the “right” order:

     0diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
     1index dcf0809e52..92bbc2681e 100644
     2--- a/src/test/fuzz/process_message.cpp
     3+++ b/src/test/fuzz/process_message.cpp
     4@@ -80,15 +80,16 @@ const std::map<std::string, std::set<std::string>> EXPECTED_DESERIALIZATION_EXCE
     5     {"Unknown transaction optional data: iostream error", {"block", "blocktxn", "cmpctblock", "tx"}},
     6 };
     7 
     8-std::unique_ptr<RegTestingSetup> g_fuzzing_setup;
     9+const RegTestingSetup* g_setup;
    10 } // namespace
    11 
    12 void initialize()
    13 {
    14-    g_fuzzing_setup = MakeUnique<RegTestingSetup>();
    15+    static RegTestingSetup setup{};
    16+    g_setup = &setup;
    17 
    18     for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
    19-        MineBlock(g_fuzzing_setup->m_node, CScript() << OP_TRUE);
    20+        MineBlock(g_setup->m_node, CScript() << OP_TRUE);
    21     }
    22     SyncWithValidationInterfaceQueue();
    23     assert(ChainActive().Tip() != nullptr);
    24@@ -116,9 +117,9 @@ void test_one_input(const std::vector<uint8_t>& buffer)
    25     p2p_node.fSuccessfullyConnected = true;
    26     p2p_node.nVersion = PROTOCOL_VERSION;
    27     p2p_node.SetSendVersion(PROTOCOL_VERSION);
    28-    g_fuzzing_setup->m_node.peer_logic->InitializeNode(&p2p_node);
    29+    g_setup->m_node.peer_logic->InitializeNode(&p2p_node);
    30     try {
    31-        (void)ProcessMessage(&p2p_node, random_message_type, random_bytes_data_stream, GetTimeMillis(), Params(), g_fuzzing_setup->m_node.connman.get(), g_fuzzing_setup->m_node.banman.get(), std::atomic<bool>{false});
    32+        (void)ProcessMessage(&p2p_node, random_message_type, random_bytes_data_stream, GetTimeMillis(), Params(), g_setup->m_node.connman.get(), g_setup->m_node.banman.get(), std::atomic<bool>{false});
    33     } catch (const std::ios_base::failure& e) {
    34         const std::string exception_message{e.what()};
    35         const auto p = EXPECTED_DESERIALIZATION_EXCEPTIONS.find(exception_message);
    
  46. tests: Add fuzzing harness for ProcessMessage(...) fd1dae10b4
  47. tests: Add one specialized ProcessMessage(...) fuzzing binary per message type for optimal results when using coverage-guided fuzzing 9220a0fdd0
  48. practicalswift commented at 7:02 am on March 11, 2020: contributor

    @MarcoFalke Indeed :) Thanks for the patch - that fixed the issue!

    Please re-review - I think we should be ready to go :)

  49. MarcoFalke commented at 11:55 am on March 11, 2020: member
    @practicalswift pls push your local branch to GitHub, thx
  50. practicalswift commented at 12:07 pm on March 11, 2020: contributor
    @MarcoFalke Sorry! Done! :)
  51. practicalswift force-pushed on Mar 11, 2020
  52. MarcoFalke commented at 12:49 pm on March 11, 2020: member

    ACK 9220a0fdd0 🏊

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK 9220a0fdd0 🏊
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUjT1QwAhjXDNbw5aYFTlg68glVfl5CPdMNZ4gKbD/RkTp3bWXzxo7f/RvRh1WCc
     8+ifotMLe4Ihdj2r6H6guhYy46Damnk+qPhaDfLxGQAc2mwZrJ4gHiS2LDiZsvckD
     9X6+SMgJK2uHEy9u5zjXLEFOoHArGb2J73XloMTmJR7HTLlFy53hEMJsYqu9+Fmo+
    10sxzXBF1Mn40I+NnHr9bEPe66fgw6Nf3PiM9CIbJLQRBhCXRLAnkurO6Ai7gK71a+
    11DjxaCie+KBcpupVdbHPTRIyRO98jSndZYzK9ksSDshh2ifxGhrUQGSKRQ3e6CGtZ
    12fgZsB3o+TKmMPKIawyxU1oAfpuCmWC/seqoLPvR7ymoYgd+gsNZGjp7w+hVbiku9
    136SwRrQSRqGg5cio4qWbqqlyqmzGBpDrs9MjcY3/NF/mJrafarZ2UB+BKQ5IrXkZj
    14BidwPFy4zJS+8hPUGeRqH8igHzG3cY5xkB1Q+Wpy2McjDguh1qkzWJcU/9ifRqnj
    152DQxCmcw
    16=VmPE
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 11743d3aff704a992c2abdf56c51b9f75f57199d2d9b269edff015d74f3fe9c3 -

  53. MarcoFalke merged this on Mar 11, 2020
  54. MarcoFalke closed this on Mar 11, 2020

  55. deadalnix referenced this in commit 946eb50093 on Oct 20, 2020
  56. practicalswift deleted the branch on Apr 10, 2021
  57. kittywhiskers referenced this in commit c8470e7d1e on Jan 9, 2022
  58. kittywhiskers referenced this in commit 097771279c on Jan 11, 2022
  59. kittywhiskers referenced this in commit 16afcfcba0 on Jan 25, 2022
  60. kittywhiskers referenced this in commit a5292ea5a0 on Feb 19, 2022
  61. kittywhiskers referenced this in commit 86069b7982 on Mar 25, 2022
  62. kittywhiskers referenced this in commit 6c1bc4d507 on Mar 30, 2022
  63. kittywhiskers referenced this in commit 7dddeedd2f on Apr 3, 2022
  64. kittywhiskers referenced this in commit 84c9f759f2 on Apr 3, 2022
  65. kittywhiskers referenced this in commit a2f7e179c0 on Apr 5, 2022
  66. kittywhiskers referenced this in commit bc429080e1 on Apr 5, 2022
  67. kittywhiskers referenced this in commit 0499ddb6b2 on Apr 6, 2022
  68. kittywhiskers referenced this in commit 2d4d2fe9c5 on Apr 6, 2022
  69. kittywhiskers referenced this in commit a379752786 on Apr 6, 2022
  70. kittywhiskers referenced this in commit d76308ece9 on Apr 7, 2022
  71. kittywhiskers referenced this in commit dfb238a6c3 on Apr 7, 2022
  72. kittywhiskers referenced this in commit 061d82c03b on Apr 17, 2022
  73. kittywhiskers referenced this in commit f0263a3629 on Apr 19, 2022
  74. PastaPastaPasta referenced this in commit f2704eae34 on Apr 20, 2022
  75. kittywhiskers referenced this in commit 764e2c7a5f on Jul 6, 2022
  76. kittywhiskers referenced this in commit 778800649e on Jul 6, 2022
  77. kittywhiskers referenced this in commit 4129098940 on Jul 13, 2022
  78. kittywhiskers referenced this in commit 10e55b3067 on Jul 13, 2022
  79. kittywhiskers referenced this in commit 38781fc114 on Jul 15, 2022
  80. DrahtBot locked this on Aug 18, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 10:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me