fuzz: fix bug in p2p_headers_presync harness #30980

pull marcofleon wants to merge 1 commits into bitcoin:master from marcofleon:2024/09/headers-presync-fuzz-bugfix changing 1 files +7 −7
  1. marcofleon commented at 2:00 pm on September 26, 2024: contributor

    The calculation for the test chain’s work (total_work) should be outside of the loop. Previously, total_work was being miscalculated due to multiple additions of work from the same headers. Now, each header’s work is only counted once, providing an accurate total.

    #30918 followup

  2. Fix bug in p2p_headers_presync harness a7498cc7e2
  3. DrahtBot commented at 2:00 pm on September 26, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK dergoegge, instagibbs, glozow, mzumsande

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  4. DrahtBot added the label Tests on Sep 26, 2024
  5. nafeef123 approved
  6. marcofleon commented at 3:34 pm on September 26, 2024: contributor

    Can be tested by running FUZZ=p2p_headers_presync ./fuzzbuild/src/test/fuzz/fuzz chainwork_crash.txt

    Should fail on master:

    0Assertion failed: (total_work < chainman.MinimumChainWork()), function p2p_headers_presync_fuzz_target, file p2p_headers_presync.cpp, line 207.
    

    and pass on this PR.

    chainwork_crash.txt

  7. maflcko commented at 8:50 am on September 27, 2024: member
    Side-note: I wonder why oss-fuzz hasn’t found this yet. According to https://oss-fuzz.com/fuzzer-stats?group_by=by-day&fuzzer=afl_bitcoin-core_p2p_headers_presync&project=bitcoin-core the exec/s is 0 for afl. (The libfuzzer one has up to 100 exec/s). I checked the build logs (https://oss-fuzz-build-logs.storage.googleapis.com/index.html#bitcoin-core) for FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION and it seems to be set.
  8. DrahtBot added the label CI failed on Sep 29, 2024
  9. DrahtBot removed the label CI failed on Sep 29, 2024
  10. dergoegge commented at 9:25 am on September 30, 2024: member

    According to https://oss-fuzz.com/fuzzer-stats?group_by=by-day&fuzzer=afl_bitcoin-core_p2p_headers_presync&project=bitcoin-core the exec/s is 0 for afl.

    This is because oss-fuzz doesn’t use our afl++ harness but rather the afl libfuzzer driver. When using this driver, afl++ will fork after the initialization code is run (https://github.com/AFLplusplus/AFLplusplus/blob/d21fb1a558b25c4f46692fa999c0028dfe0eecc0/utils/aflpp_driver/aflpp_driver.c#L393), which might end up not working for harnesses that e.g. create temporary directories or threads during init. You can check the afl++ oss-fuzz logs and you’ll see that all inputs in the corpus timeout. This is the case for a few of our harnesses and as consequence they aren’t fuzzed with afl++ on oss-fuzz.

  11. in src/test/fuzz/p2p_headers_presync.cpp:208 in a7498cc7e2
    210 
    211-        // This test should never create a chain with more work than MinimumChainWork.
    212-        assert(total_work < chainman.MinimumChainWork());
    213-    }
    214+    // This test should never create a chain with more work than MinimumChainWork.
    215+    assert(total_work < chainman.MinimumChainWork());
    


    dergoegge commented at 9:27 am on September 30, 2024:

    nit: You could get rid of total_work as a variable and just do:

    0    assert(CalculateClaimedHeadersWork(all_headers) < chainman.MinimumChainWork());
    

    marcofleon commented at 11:31 am on September 30, 2024:
    If I add the first block to all_headers then I could do this. I do like the variable for clarity, and I’d say this assertion is mainly to make the test easier to read. So I’ll keep it as is.
  12. dergoegge approved
  13. dergoegge commented at 4:00 pm on October 1, 2024: member
    utACK a7498cc7e26b4b3de976e111de2bd2d79b056b31
  14. instagibbs approved
  15. instagibbs commented at 8:12 pm on October 2, 2024: member
    ACK a7498cc7e26b4b3de976e111de2bd2d79b056b31
  16. glozow commented at 9:55 pm on October 2, 2024: member
    makes sense, utACK a7498cc7e26b4b3de976e111de2bd2d79b056b31
  17. glozow merged this on Oct 3, 2024
  18. glozow closed this on Oct 3, 2024


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-10-08 16:12 UTC

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