Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented May 26, 2023

The process_message_${msg_type} fuzz targets have many issues:

  • In a context where each fuzz target must be a separate binary, this bloats the storage requirements by the number of message types.
  • The qa-assets repo for fuzz inputs also bloats, because each input in the type specific folder (./process_message_${msg_type}) is accompanied by a similar input in the general folder (./process_message) or a in another specific folder. The size seems to be ~3GB for the sum of all folders vs 0.3GB for the general folder.
  • Handling of different folders for each message type and one general folder for all message types (and unknown message types) is undocumented and unclear. Cross-pollination is encouraged, I guess, but who does it?
  • It is unclear if the fuzz target has any value at all, given that any bug that is found here should also be found by the process_messages fuzz target, and historically always has been? So maybe it can even be removed completely in the future?
  • (minor nit): When adding a new message type, the message type has to be added to this fuzz target as well.

Fix all issues by turning the compile-time setting into a run-time setting, thus removing the extra executables and fuzz folders. The same approach is also taken by the rpc fuzz target.

If someone wants to limit their fuzzing to a specific message type, they can still do it. For example,

LIMIT_TO_MESSAGE_TYPE=inv FUZZ=process_message ./src/test/fuzz/fuzz

@DrahtBot
Copy link
Contributor

DrahtBot commented May 26, 2023

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK dergoegge
Concept ACK brunoerg

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

@DrahtBot DrahtBot added the Tests label May 26, 2023
@fanquake fanquake requested a review from dergoegge May 26, 2023 10:20
Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@maflcko maflcko force-pushed the 2305-fuzz-limit-msg-type- branch from fa0a0b8 to 1111c9a Compare May 26, 2023 15:14
Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1111c9a

Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@fanquake
Copy link
Member

Nice. Should help with oss-fuzz disk issues. Related to bitcoin-core/qa-assets#131

@fanquake fanquake merged commit 927b001 into bitcoin:master May 27, 2023
@brunoerg
Copy link
Contributor

post-merge ACK 1111c9a

@maflcko maflcko deleted the 2305-fuzz-limit-msg-type- branch May 27, 2023 13:49
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 29, 2023
@bitcoin bitcoin locked and limited conversation to collaborators May 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants