fuzz: Extend addrman fuzz test with deserialize #22493

1. 
   MarcoFalke commented at 11:00 am on July 19, 2021: member

   Requested on IRC:

   0[18:01] <vasild> => I think there is a good chance fuzzing addrman unserialize will find more bugs
   1[18:04] <sipa> definitely
   

2. fanquake added the label Tests on Jul 19, 2021
3. in src/test/fuzz/addrman.cpp:56 in fa0a11e0e1 outdated

   44+        const auto ser_version{fuzzed_data_provider.ConsumeIntegral<int32_t>()};
   45+        ds.SetVersion(ser_version);
   46+        try {
   47+            ds >> addr_man;
   48+        } catch (const std::ios_base::failure&) {
   49+        }
   

   ---

   ---

   

   vasild commented at 11:48 am on July 19, 2021:

   I think that addr_man is not in a usable state if an exception is thrown (i.e. it is in an undefined state). Below we call its methods and expect they would work. Maybe:

   0        try {
   1            ds >> addr_man;
   2        } catch (const std::ios_base::failure&) {
   3            addr_man.Clear();
   4        }
   

   ---

   

   MarcoFalke commented at 12:19 pm on July 19, 2021:

   Good catch. Fixed.
4. 
   vasild commented at 11:56 am on July 19, 2021: member

   Concept ACK

   The unserialize itself is fuzzed in:

   https://github.com/bitcoin/bitcoin/blob/d3474b8df2f973e9b9142c0b64505a8a78bcb292/src/test/fuzz/deserialize.cpp#L201-L204

   What this test adds is calling addrman methods after it is unserialized from fuzzed data. This kind of simulates a disk corruption. I think it may result in the same addr:port being in “new” and “tried” or other “interesting” situations that do not occur from normal operation.

5. MarcoFalke force-pushed on Jul 19, 2021
6. vasild approved
7. 
   vasild commented at 12:30 pm on July 19, 2021: member

   ACK fa740252d2dcc272204b745906785fe26e9a0b77
8. MarcoFalke requested review from practicalswift on Jul 19, 2021
9. in src/test/fuzz/addrman.cpp:53 in fa740252d2 outdated

   48+        } catch (const std::ios_base::failure&) {
   49+            addr_man.Clear();
   50+        }
   51+    }
   52     if (fuzzed_data_provider.ConsumeBool()) {
   53         addr_man.m_asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
   

   ---

   ---

   

   MarcoFalke commented at 1:21 pm on July 19, 2021:

   I guess asmap should be set first, since it is used in deserialize?

   ---

   

   jonatack commented at 2:29 pm on July 19, 2021:

   True, this works for me.

    0@@ -38,6 +38,12 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
    1     SetMockTime(ConsumeTime(fuzzed_data_provider));
    2     CAddrManDeterministic addr_man;
    3     addr_man.MakeDeterministic(ConsumeUInt256(fuzzed_data_provider));
    4+    if (fuzzed_data_provider.ConsumeBool()) {
    5+        addr_man.m_asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
    6+        if (!SanityCheckASMap(addr_man.m_asmap)) {
    7+            addr_man.m_asmap.clear();
    8+        }
    9+    }
   10     if (fuzzed_data_provider.ConsumeBool()) {
   11         const std::vector<uint8_t> serialized_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)};
   12         CDataStream ds(serialized_data, SER_DISK, INIT_PROTO_VERSION);
   13@@ -49,12 +55,6 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
   14             addr_man.Clear();
   15         }
   16     }
   17-    if (fuzzed_data_provider.ConsumeBool()) {
   18-        addr_man.m_asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
   19-        if (!SanityCheckASMap(addr_man.m_asmap)) {
   20-            addr_man.m_asmap.clear();
   21-        }
   22-    }
   

   ---

   

   vasild commented at 4:10 pm on July 19, 2021:

   yes

   ---

   

   MarcoFalke commented at 4:27 pm on July 19, 2021:

   Done
10. 
    jonatack commented at 2:03 pm on July 19, 2021: member

    Tested ACK fa740252d2dcc272204b745906785fe26e9a0b77
11. fuzz: Extend addrman fuzz test with deserialize aaaa9c6019
12. MarcoFalke force-pushed on Jul 19, 2021
13. 
    jonatack commented at 4:30 pm on July 19, 2021: member

    ACK aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0 per git diff fa74025 aaaa9c6
14. 
    DrahtBot commented at 7:09 pm on July 19, 2021: 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:

    • #22508 by apoelstra
    • #21129 by vasild

    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.

15. 
    MarcoFalke commented at 7:04 am on July 20, 2021: member

    @vasild I am seeing there is a conflict. Do you have a preference which one should be merged first?
16. vasild approved
17. 
    vasild commented at 9:44 am on July 22, 2021: member

    ACK aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0

    @vasild I am seeing there is a conflict. Do you have a preference which one should be merged first?

    No.

18. 
    MarcoFalke commented at 2:54 pm on July 22, 2021: member

    Ok, merging the one with the nicer commit-hash first :sweat_smile:
19. MarcoFalke merged this on Jul 22, 2021
20. MarcoFalke closed this on Jul 22, 2021

    ---

21. MarcoFalke deleted the branch on Jul 22, 2021
22. sidhujag referenced this in commit 445a48b579 on Jul 23, 2021
23. DrahtBot locked this on Aug 18, 2022

Contributors
MarcoFalke vasild jonatack DrahtBot

Review Requested
practicalswift

Labels
Tests