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

Separate state trait/impl for generative fuzzers #2304

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

langston-barrett
Copy link
Contributor

Generative fuzzers do not need a corpus. To this end, this commit splits out GenState (trait) and StdGenState (struct) from State (trait) and StdState (struct). The idea is that we will support some notion of a generative fuzzer (perhaps GenFuzzer) that will not rely on having a "current corpus ID". Down the line, we may also want to move input loading/generation/scheduling into a method on GenState that returns a State, see #2200.

This is progress towards #2161.

@langston-barrett
Copy link
Contributor Author

langston-barrett commented Jun 11, 2024

GitHub Actions has encountered an internal error when running your job.

🙃

Generative fuzzers do not need a corpus. To this end, this commit splits
out `GenState` (trait) and `StdGenState` (struct) from `State` (trait)
and `StdState` (struct). The idea is that we will support some notion of
a generative fuzzer (perhaps `GenFuzzer`) that will not rely on having a
"current corpus ID". Down the line, we may also want to move input
loading/generation/scheduling into a method on `GenState` that returns
a `State`, see AFLplusplus#2200.
@langston-barrett
Copy link
Contributor Author

cc @tokatoka and @domenukk, who participated in some discussion on this topic.

@domenukk
Copy link
Member

I think we should leave State as State and rename your current state to something else, or even just manually spell out the trait bounds where they are needed

@domenukk
Copy link
Member

Just mainly because GenState looks pretty ugly 😂
Maybe rename your current state to MutatingState or something?

@domenukk
Copy link
Member

Also I don't think we should have one StdState and one StdGenState, let's just do a StdState and a GenerativeState(?)

@domenukk
Copy link
Member

Or otherwise use StdState and don't set the current corpus id? Probably something we discussed earlier already, but now if you divide the traits it should work(?)

@langston-barrett
Copy link
Contributor Author

I'm not 100% sure I understand your suggestions, let me try to paraphrase:

I think we should leave State as State and rename your current state to something else, or even just manually spell out the trait bounds where they are needed

So, in terms of this PR, this would be: rename GenState to State, just remove what's currently called State and add HasCurrentCorpusId manually where needed?

Maybe rename your current state to MutatingState or something?

Again in terms of the code in this PR, the suggestion is: rename {Std,}State to {Std,}MutatingState and Gen{Std,}State to State?

Or otherwise use StdState and don't set the current corpus id? Probably something we discussed earlier already, but now if you divide the traits it should work(?)

So this would be: Keep both traits, but leave StdState as it was before these changes, change use-sites to use GenState where possible, don't set the corpus ID of StdState when doing generative fuzzing?

@domenukk
Copy link
Member

Yes to 1 and 3, for 2, I would say Std is always the default/standard thing to use, so we shouldn't have two Std states imho. Instead, let's have one StdState and one GenerativeState (unless suggestion 3 works)

@domenukk
Copy link
Member

domenukk commented Jun 13, 2024

So ideally we have one State trait that doesn't have a current corpus ID, and one StdState struct that can be used for both use cases. Otherwise, have one StdState and one GenerativeState (for the same traits)

@domenukk
Copy link
Member

@langston-barrett what's the status on this? Should I help out/rename the things?

@domenukk
Copy link
Member

@addisoncrump we should consider this use case for our big generics cleanup

@domenukk
Copy link
Member

Soo... the generics cleanup didn't go too far...
Should we take/finish this PR? @addisoncrump @tokatoka

Imho GenState should be called just State, the current State should be called MutatingState/MutationalState (or similar)

@tokatoka
Copy link
Member

there won't be State or GenState after the rework

@domenukk
Copy link
Member

Given that the rework is still gonna take some time, why not merge this in the meantime? Otherwise we should close this PR IMHO

@tokatoka
Copy link
Member

Merging StdGenState is ok. Adding GenState is no. i'll have to delete it in the future.

@domenukk
Copy link
Member

The GenState struct without a generative state trait makes no sense, right?

@tokatoka
Copy link
Member

as i said, there's no such trait as State or GenState in the future. it shouldn't exist
and for the code you are swapping State with GenState, but with the current code of LibAFL we simply don't have any contraints on Feedbacks anymore so you have no place to use it

@domenukk
Copy link
Member

Why? I think we can still have a State trait that sais "If you want a state, implement the following...", right?

@tokatoka
Copy link
Member

tokatoka commented Oct 23, 2024

because there should not be any downstream struct in libafl that uses State trait.
Such code should be eliminated
One should use HasImported/HasCorpus/... etc. not State. only then can we apply the minimal set of type constraints on structs/traits

@domenukk
Copy link
Member

Sure but it might still be useful for people to have one State trait that's the superset of everything they'll have to implement to have a working state

@tokatoka
Copy link
Member

idk to me the existence of State is purely wrong
@addisoncrump opinion?

@domenukk domenukk marked this pull request as draft November 8, 2024 16:20
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.

3 participants