fuzz: Avoid timeout and bloat in fuzz targets #28815

1. 
   maflcko commented at 4:30 pm on November 7, 2023: member

   If the fuzz input contains invalid data in a loop, abort early. This will teach the fuzz engine to look for useful data and avoids bloating the fuzz input folder with useless (repeated) data.
2. 
   DrahtBot commented at 4:31 pm on November 7, 2023: 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, brunoerg

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #28438 (Use serialization parameters for CTransaction by ajtowns)
   • #28280 (Don’t empty dbcache on prune flushes: >30% faster IBD by andrewtoth)
   • #28216 (fuzz: a new target for the coins database by darosior)

   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.

3. DrahtBot added the label Tests on Nov 7, 2023
4. 
   maflcko commented at 4:37 pm on November 7, 2023: member

   Can be tested by running the corresponding test case from #28812 (comment) and looking at the runtime
5. maflcko force-pushed on Nov 7, 2023
6. maflcko renamed this:
   fuzz: Avoid timeout and bloat in bloom_filter target
   fuzz: Avoid timeout and bloat in bloom_filter and policy_estimator
   on Nov 7, 2023
7. maflcko force-pushed on Nov 7, 2023
8. maflcko renamed this:
   fuzz: Avoid timeout and bloat in bloom_filter and policy_estimator
   fuzz: Avoid timeout and bloat in fuzz targets
   on Nov 7, 2023
9. maflcko force-pushed on Nov 7, 2023
10. maflcko force-pushed on Nov 7, 2023
11. maflcko force-pushed on Nov 7, 2023
12. 
    brunoerg commented at 7:47 pm on November 7, 2023: contributor

    Concept ACK

    Question: What is the difference compared to #27552 (“partial” fuzzers)?

13. bitcoin deleted a comment on Nov 7, 2023
14. 
    maflcko commented at 7:56 am on November 8, 2023: member

    Question: What is the difference compared to #27552 (“partial” fuzzers)?

    As I said on that pull request, it needs benchmarks and review on a case-by-case basis to check whether it is useful for a specific fuzz target. If someone wants to do those benchmarks on the fuzz targets touched in this pull request, and finds a positive outcome, nothing is holding anyone back from applying the concept.

    The goal of this pull request is to prevent the fuzz engine from exploring invalid serializations in a loop, as they do not help the fuzz target in any way, increase runtime, and increase storage costs.

    I’ve edited the pull request description to clarify that this only covers loop serialization.

15. fuzz: Avoid timeout and bloat in fuzz targets

    Also, fix iwyu

    fabb5046a7
16. maflcko force-pushed on Nov 8, 2023
17. 
    maflcko commented at 9:13 am on November 8, 2023: member

    Did the same for coins_view, because it followed the same pattern, even though a timeout is currently not reported by Oss-Fuzz.
18. fanquake requested review from dergoegge on Nov 8, 2023
19. 
    dergoegge commented at 11:27 am on November 8, 2023: member

    There seems to be a pattern here, maybe it makes sense to have something like a RepeatedCallOneOf that does the exit on bad data internally?
20. 
    maflcko commented at 12:49 pm on November 8, 2023: member

    Not sure, there are also some loops that do not pick out of several callables. Happy to push any refactoring, if someone uploads a diff, patch, or commit.
21. dergoegge approved
22. 
    dergoegge commented at 1:13 pm on November 8, 2023: member

    utACK fabb5046a7365af3079e6e45606d63576bc6ad12

    Didn’t test but the rational makes a lot of sense to me. This should be better even if it doesn’t solve the timeouts.

23. DrahtBot requested review from brunoerg on Nov 8, 2023
24. brunoerg approved
25. 
    brunoerg commented at 1:45 pm on November 8, 2023: contributor

    crACK fabb5046a7365af3079e6e45606d63576bc6ad12

    Logic makes sense for me. I didn’t check all the targets, but it seems we could apply it in coincontrol as well.

26. 
    maflcko commented at 1:51 pm on November 8, 2023: member

    Logic makes sense for me. I didn’t check all the targets, but it seems we could apply it in coincontrol as well.

    Yes, I was thinking about this, too. In practise it doesn’t matter because COutPoint happens to be (de)serialized as a fixed-size byte blob, which can only fail if the fuzz provider is out of data. In that case the loop will already abort. However, it could make sense to change this for consistency as well, or to protect against the case where COutPoint is “dynamically” deserialized and could fail even if the provider has more data to present.

    Can be done in a follow-up, or in this pull, if I have to re-touch again.

27. fanquake merged this on Nov 8, 2023
28. fanquake closed this on Nov 8, 2023

    ---

29. maflcko deleted the branch on Nov 8, 2023
30. bitcoin locked this on Nov 7, 2024

Contributors
maflcko DrahtBot brunoerg dergoegge

Labels
Tests