-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(fuzz): WorkerCorpus for multiple worker threads
#11769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
SharedCorpus for multiple worker threadsSharedCorpus for multiple worker threads
…es that provided in new coverage
SharedCorpus for multiple worker threadsWorkerCorpus for multiple worker threads
| // Track in-memory corpus changes to update MasterWorker on sync | ||
| let new_index = self.in_memory_corpus.len(); | ||
| self.new_entry_indices.push(new_index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine, but may result in some corpus entries not getting sync'd e.g. due to a crash or ctr+c and restart. If you persist the last sync'd timestamp, it can recover from restarts by checking if there are newer entries written before a sync could occur
WorkerCorpus for multiple worker threadsWorkerCorpus for multiple worker threads
|
please merge master, this is still using old CI runners |
| 'corpus_replay: for entry in std::fs::read_dir(corpus_dir)? { | ||
| let path = entry?.path(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we continually write to the corpus directory? This is very expensive as we not only iterate a directory and read the files, but we also (if gzip is enabled) do decompression over and over, potentially of the same file. It feels like the corpus should be default in-memory, and we only write at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just happening at start up. The entries are held in memory so long as they don't exceed a configurable limit and then flushed to disk (and compressed if it's enabled).
Your point does still stand elsewhere. IIUC workers share compressed corpus entries so they potentially are repeatedly decompressing the same files. Moving compression to the very end would resolve this
| if !tx_seq.is_empty() { | ||
| let mut new_coverage_on_sync = false; | ||
| for tx in &tx_seq { | ||
| if can_replay_tx(tx) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking the entire sequence prior to executing may make sense as I think currently new_coverage_on_sync can be set and a later tx contains an invalid selector. Ideally, we could just drop those txs and see if we can recover and retain the modified, valid corpus entry
|
Overall looks pretty good to me. I left a few comments and there are some outstanding. I would recommend adding a config to turn syncing off and also a way to make it occur more/less frequently. |
|
Merged into #12713. |
Motivation
towards #8898 (#11842)
Solution
CorpusManagerwithWorkerCorpusWorkerCorpusis the corpus to be used by parallel worker threads.WorkerCorpushas anid: u32. The master worker has theid = 0.WorkerCorpus's share theirin_memory_corpusamongst each other via the file system using a star-pattern where the master worker (id = 0) is in the center.corpus_diris now organized as:worker0/sync/- Seefn exportin 089f30bworker0/corpusentries (which includes entries from all workers when synced) to each workerssync/directory - Seefn distributein d4200e4corpur_dir/workerId/syncdir intocorpus_dir/workerId/corpusif it leads to new coverage and updates it'shistory_map- Seefn calibratein 488d09dfn calibratewe are fetching the new corpus entries from the workerssync/dir and replaying the tx sequences to check if they lead to new coverage for this particular worker. If it does then we're updatinghistory_map.pub fn syncintroduced in e9d8d3c handles all of the above.Note: This PR does not address parallelizing the fuzz runs, only prepares for it. Opened for initial feedback on the approach.
PR Checklist