fuzz: Reset addrman when consistency check fails #22939

pull MarcoFalke wants to merge 4 commits into bitcoin:master from MarcoFalke:2109-fuzzAddrmanCheck changing 4 files +25 −18

MarcoFalke commented at 1:41 PM on September 10, 2021: member

It doesn't really make sense to continue when the deserialized addrman isn't consistent in the addrman test.

For example, if nLastSuccess is negative, it would later result in integer overflows. Thus, this patch fixes #22931.

Also, Fixes #22503 Fixes #22504 Fixes #22519

move-only: Move CAddrMan::Check to cpp file

This speeds up compilation of the whole program because the included
header file is smaller.

Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

faee767991

Refactor: Turn the internal addrman check helper into a forced check fa038ce9fa
fuzz: Reset addrman when consistency check fails fa6e161a35
DrahtBot added the label P2P on Sep 10, 2021
MarcoFalke removed the label P2P on Sep 10, 2021
MarcoFalke added the label Tests on Sep 10, 2021
amitiuttarwar commented at 3:05 PM on September 10, 2021: contributor

concept ACK
mzumsande commented at 7:15 PM on September 10, 2021: member

Concept ACK.

I'd understand if it's beyond the scope of this PR, but I think that a second ForceCheckAddrman() at the end of the addrman fuzz target (which isn't caught but can actually lead to crashes) would be really helpful - probably better than activating consistency_check_ratio during fuzzing which could make the fuzzer slow if called in too many steps.
DrahtBot commented at 2:43 AM on September 11, 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:
- #22950 ([p2p] Pimpl AddrMan to abstract implementation details by amitiuttarwar)
- #22872 (log: improve checkaddrman logging with duration in milliseconds by jonatack)
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.
MarcoFalke commented at 7:07 AM on September 11, 2021: member

a second ForceCheckAddrman() at the end of the addrman fuzz target

Good idea. Wanted to do it in a follow-up, but it is just a one-line fix, so I added the commit here.
fuzz: Check addrman after addrman fuzz target faf9bd3374
MarcoFalke force-pushed on Sep 11, 2021

in src/addrman.h:394 in faf9bd3374

 400 | -        if (err) {
 401 | -            LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
 402 | -            assert(false);
 403 | -        }
 404 | -    }
 405 | +    //! Consistency check, taking into account m_consistency_check_ratio.

vasild commented at 11:12 AM on September 14, 2021:

    //! Consistency check, taking into account m_consistency_check_ratio. Will abort(2) if an inconsistency is detected.

in src/addrman.h:397 in faf9bd3374

 405 | +    //! Consistency check, taking into account m_consistency_check_ratio.
 406 | +    void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs);
 407 |  
 408 | -    //! Perform consistency check. Returns an error code or zero.
 409 | -    int Check_() const EXCLUSIVE_LOCKS_REQUIRED(cs);
 410 | +    //! Perform consistency check, regardless of m_consistency_check_ratio. Returns an error code or zero.

vasild commented at 11:15 AM on September 14, 2021:

    //! Perform consistency check, regardless of `m_consistency_check_ratio`.
    //! [@returns](/bitcoin-bitcoin/contributor/returns/) an error code or zero.

in src/test/fuzz/addrman.cpp:246 in faf9bd3374
```
 240 | @@ -238,6 +241,9 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
 241 |          ds.SetVersion(ser_version);
 242 |          try {
 243 |              ds >> *addr_man_ptr;
 244 | +            if (0 != WITH_LOCK(addr_man_ptr->cs, return addr_man_ptr->ForceCheckAddrman())) {
 245 | +                throw std::ios_base::failure{"Reset addrman after consistency check failed"};
 246 | +            }
```
vasild commented at 11:19 AM on September 14, 2021:

What about moving this unconditional check inside, at the end of CAddrMan::Unserialize()? We never want to continue if this check fails after unserialize and it is not too expensive to check once per unserialize.

Right now we have the conditional Check() at the end of Unserialize().

MarcoFalke commented at 11:23 AM on September 14, 2021:

This is what #22734 is doing. Happy to close this fuzz-only pull and rebase the other.

vasild commented at 8:08 AM on September 15, 2021:

Up to you (and other reviewers). I "voted" for the other PR :)
MarcoFalke closed this on Sep 15, 2021
MarcoFalke deleted the branch on Sep 15, 2021
DrahtBot locked this on Oct 30, 2022

Contributors

Labels

Linked (view graph)

#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 #22931 Signed Integer-overflow in CAddrInfo::IsTerrible

fuzz: Reset addrman when consistency check fails #22939

Conflicts