Skip to content
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

Corpus pruning stage #2399

Merged
merged 15 commits into from
Jul 16, 2024
Merged

Corpus pruning stage #2399

merged 15 commits into from
Jul 16, 2024

Conversation

tokatoka
Copy link
Member

No description provided.

@tokatoka tokatoka mentioned this pull request Jul 15, 2024
@addisoncrump
Copy link
Collaborator

ubuntu-common should probably be a prerequisite for the fuzzer passes

@tokatoka
Copy link
Member Author

ok

@tokatoka tokatoka requested a review from domenukk July 15, 2024 16:26
@tokatoka tokatoka marked this pull request as ready for review July 15, 2024 16:26
@tokatoka
Copy link
Member Author

@domenukk
can you look at this today? (we want to freeze the repo today right?
no big changes, just two stages that does conditional restarting

) -> Result<(), Error> {
manager.on_restart(state).unwrap();
println!("Exiting!");
std::process::exit(0);
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the only way to force exit from inside any stage

Copy link
Member

Choose a reason for hiding this comment

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

It feels wrong, I think we should prefer to return cleanly from stages to the fuzzer to quit from there. This is very doable, and will also work for forkserver since the fuzzer can decide what to do.
But also, it gets the job done, so why not...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should prefer to return cleanly from stages to the fuzzer to quit from there

maybe i should the newly added api from aarnav

let corpus = state.corpus_mut();
for (idx, retain) in do_retain.iter().enumerate().take(n_corpus) {
if !retain {
let removed = corpus.remove(CorpusId(idx))?;
Copy link
Member

Choose a reason for hiding this comment

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

Creating a raw CorpusId will likely fail at some point, after removing the corpus Ids will be sparse

Copy link
Member

Choose a reason for hiding this comment

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

you need to corpus.nth() if you want to map an int to CorpusId

Copy link
Member Author

Choose a reason for hiding this comment

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

i changed it, is it correct now?

_manager: &mut EM,
) -> Result<(), Error> {
// Iterate over every corpus entry
let n_corpus = state.corpus().count_all();
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you just call corpus().next() in a loop? Because you also want disabled testcases? Do we need an extra API?

fn next(&self, id: CorpusId) -> Option<CorpusId>;

Copy link
Member

Choose a reason for hiding this comment

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

Why do you even want disabled testcases?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because you also want disabled testcases?

yes

Do we need an extra API?

maybe yes

Why do you even want disabled testcases?

Because the point of this corpus pruning is to hide some inputs so that fuzzer won't schedule it. but we don't discard it because we would reactivate them again later

Copy link
Member

Choose a reason for hiding this comment

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

But you don't want need to disable disabled testcases, right now you do(?)

Copy link
Member

Choose a reason for hiding this comment

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


let corpus = state.corpus_mut();
let removed = corpus.remove(corpus_id)?;
corpus.add_disabled(removed)?;
Copy link
Member

Choose a reason for hiding this comment

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

Is there no way to set the state to disabled? Calling remove and re-adding sounds worse

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we don't have a method to disable, cc @R9295

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't think there's other way

Copy link
Collaborator

Choose a reason for hiding this comment

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

There isn't a method but I think makes sense to add one

Copy link
Member Author

Choose a reason for hiding this comment

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

essentially disabled corpus and enabled corpus are separate corpus so there's no other way than removing and adding right?

) -> Result<(), Error> {
// Iterate over every corpus entry
let n_corpus = state.corpus().count_all();
let mut do_retain = vec![];
Copy link
Member

Choose a reason for hiding this comment

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

What if you don't retain any enabled entries?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah it's a problem.
maybe i should make it keep at least one

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@tokatoka tokatoka merged commit f00470d into main Jul 16, 2024
98 checks passed
@tokatoka tokatoka deleted the pruning branch July 16, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants