-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Introduce ListInput #2972
Introduce ListInput #2972
Conversation
I've also added a bunch of mutators (remove last entry, remove random entry, append entry based on generator). |
I still feel like this is basically just multipart input without names for parts? Happy to merge this anyway but let's ask @addisoncrump what he thinks |
So, after talking to Addison: @riesentoaster can we not merge your ListInput mutators into MultipartInput? We can have the name of the Multipart input be a generic and then we can just pass in That would be way less code and functionality duplication |
There is a good bit of additional complexity this way. I'm not sure the compiler is going to be able to optimize this all away. Also: Once #2944 is solved, we should remove the |
Actually on second thought: Why don't we have a generic |
@addisoncrump opinions? |
libafl/src/inputs/multi.rs
Outdated
} | ||
|
||
impl<I> Default for MultipartInput<I> { | ||
impl<I> Default for ListInput<I> { |
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.
Why not keep MultipartInput as name? They are roughly equal, right?
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.
Ah wait I think I see what you did there
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.
Because there could be other inputs based on other collection types such as maps. I'd personally also like to rename MultipartInput, but kept it for now because it's a change that isn't strictly necessary.
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.
How would this work differently for maps? isn't multipart input alrady key-value?
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.
Could be tree maps or sth
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.
Not sure I agree
I think that this change makes sense. Originally, the plan was to have multipart be generic over name, but for an initial implementation this wasn't necessary. So, from my perspective, |
FWIW I have some colleagues who do use multipart specifically with names, and the crossover implementation which considers common names is very important in this regard. Please ensure this functionality stays. |
They do for now. In the future, I'd suggest creating custom mutators for that, and manually combining them with the non-crossover mutators. One could argue the "default" crossover mutators should not consider names, but do crossover within byte arrays from any input part. And crossover within list inputs is ambiguous anyways, see the crossover mutators I have implemented for between the entire I would like to remove the hack with |
I trust your judgement 👍 I just want to note how people expect things to be used. It's a shame we can't emit compiler warnings for specific monomorphisations... |
💯 It only targets Well, once #2944 is done anyways. |
Also: Would you be open to renaming Edit: actually, let's wait with that to keep the disruption as minimal as possible, while we don't change anything substantial about it. |
I am not deep enough into MultipartInputs, @addisoncrump can you take a look? |
libafl/src/mutators/multi.rs
Outdated
where | ||
M: DefaultMultipartMutator + Mutator<I, S>, | ||
S: HasRand, | ||
{ | ||
fn mutate( | ||
&mut self, | ||
state: &mut S, | ||
input: &mut MultipartInput<I>, | ||
input: &mut MultipartInput<I, N>, |
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 we can remove the Generic again, just make the guy CoW<'static, str>
or something
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.
The original idea was to use MutlipartInput and have ListInput be a MultipartInput with ()
as name, but you basically did the exact opposite :P
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.
Names are just a form of id for each part. So if you don't need it human readable, numbers would probably be slightly more performant, because you don't need to compare the entire string, no?
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.
Then should it be key
instead of name
? (I'm not sure)
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 was searching for a better name, but ID seemed weird. I like key.
Compared to
MultipartInput
, this does not care about names, and supports mapping mutators that target the inner value to target the whole thing.Useful, e.g. for stateful protocol fuzzing. If you would like me to, I'm happy to add an example (baby) fuzzer.