fuzz: Extend addrman fuzz test with deserialize #22493

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2107-fuzzAddrDeser changing 1 files +11 −0
  1. MarcoFalke commented at 11:00 AM on July 19, 2021: member

    Requested on IRC:

    [18:01] <vasild> => I think there is a good chance fuzzing addrman unserialize will find more bugs
[18:04] <sipa> definitely
  2. fanquake added the label Tests on Jul 19, 2021
  3. MarcoFalke cross-referenced this on Jul 19, 2021 from issue addrman serialize: nNew was wrong, oh ow by MarcoFalke
  4. MarcoFalke cross-referenced this on Jul 19, 2021 from issue addrman: detect on-disk corrupted nNew and nTried during unserialization by vasild
  5. MarcoFalke cross-referenced this on Jul 19, 2021 from issue CAddrMan::Serialize: Assertion `nIds != nNew' failed. by MarcoFalke
  6. 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:

            try {
            ds >> addr_man;
        } catch (const std::ios_base::failure&) {
            addr_man.Clear();
        }
    MarcoFalke commented at 12:19 PM on July 19, 2021:

    Good catch. Fixed.

  7. vasild commented at 11:56 AM on July 19, 2021: contributor

    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.

  8. MarcoFalke force-pushed on Jul 19, 2021
  9. vasild approved
  10. vasild commented at 12:30 PM on July 19, 2021: contributor

    ACK fa740252d2dcc272204b745906785fe26e9a0b77

  11. MarcoFalke requested review from practicalswift on Jul 19, 2021
  12. 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.

    @@ -38,6 +38,12 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
     SetMockTime(ConsumeTime(fuzzed_data_provider));
     CAddrManDeterministic addr_man;
     addr_man.MakeDeterministic(ConsumeUInt256(fuzzed_data_provider));
+    if (fuzzed_data_provider.ConsumeBool()) {
+        addr_man.m_asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
+        if (!SanityCheckASMap(addr_man.m_asmap)) {
+            addr_man.m_asmap.clear();
+        }
+    }
     if (fuzzed_data_provider.ConsumeBool()) {
         const std::vector<uint8_t> serialized_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)};
         CDataStream ds(serialized_data, SER_DISK, INIT_PROTO_VERSION);
@@ -49,12 +55,6 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
             addr_man.Clear();
         }
     }
-    if (fuzzed_data_provider.ConsumeBool()) {
-        addr_man.m_asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
-        if (!SanityCheckASMap(addr_man.m_asmap)) {
-            addr_man.m_asmap.clear();
-        }
-    }
    vasild commented at 4:10 PM on July 19, 2021:

    yes

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

    Done

  13. jonatack commented at 2:03 PM on July 19, 2021: contributor

    Tested ACK fa740252d2dcc272204b745906785fe26e9a0b77

  14. fuzz: Extend addrman fuzz test with deserialize aaaa9c6019
  15. MarcoFalke force-pushed on Jul 19, 2021
  16. jonatack commented at 4:30 PM on July 19, 2021: contributor

    ACK aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0 per git diff fa74025 aaaa9c6

  17. DrahtBot commented at 7:09 PM on July 19, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    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.

  18. DrahtBot cross-referenced this on Jul 20, 2021 from issue fuzz: check that ser+unser produces the same AddrMan by vasild
  19. MarcoFalke cross-referenced this on Jul 20, 2021 from issue Addrman check failure info.nLastSuccess is 0 even though info.fInTried by MarcoFalke
  20. MarcoFalke cross-referenced this on Jul 20, 2021 from issue Addrman check failure info.nRefCount is non-zero even though !info.fInTried by MarcoFalke
  21. 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?

  22. DrahtBot cross-referenced this on Jul 20, 2021 from issue fuzz: replace every fuzzer-controlled while loop with a macro by apoelstra
  23. MarcoFalke cross-referenced this on Jul 21, 2021 from issue Addrman check failure: info.nLastSuccess is negative by MarcoFalke
  24. vasild approved
  25. vasild commented at 9:44 AM on July 22, 2021: contributor

    ACK aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0

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

    No.

  26. MarcoFalke commented at 2:54 PM on July 22, 2021: member

    Ok, merging the one with the nicer commit-hash first :sweat_smile:

  27. MarcoFalke merged this on Jul 22, 2021
  28. MarcoFalke closed this on Jul 22, 2021
  29. MarcoFalke deleted the branch on Jul 22, 2021
  30. sidhujag referenced this in commit 445a48b579 on Jul 23, 2021
  31. mzumsande cross-referenced this on Aug 13, 2021 from issue addrman: Make consistency checks a runtime option by jnewbery
  32. bitcoin locked this on Aug 18, 2022
Contributors
MarcoFalkevasildjonatackDrahtBot


practicalswift

Labels
Tests
Linked (view graph)
#20233 addrman: Make consistency checks a runtime option#22450 addrman serialize: nNew was wrong, oh ow#22455 addrman: detect on-disk corrupted nNew and nTried during unserialization#22494 CAddrMan::Serialize: Assertion `nIds != nNew' failed.#22503 Addrman check failure info.nLastSuccess is 0 even though info.fInTried#22504 Addrman check failure info.nRefCount is non-zero even though !info.fInTried#22519 Addrman check failure: info.nLastSuccess is negative
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: 2026-05-20 06:53 UTC