-
-
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
base: main
Are you sure you want to change the base?
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. |
libafl/src/inputs/multi.rs
Outdated
/// Access multiple parts mutably. | ||
/// | ||
/// ## Panics | ||
/// | ||
/// Panics if idxs is not sorted, has duplicate elements, or any entry is out of bounds. | ||
#[must_use] | ||
pub fn parts_mut<const N: usize>(&mut self, mut idxs: [usize; N]) -> [&mut I; N] { | ||
pub fn parts_by_idxs_mut<const C: usize>(&mut self, mut idxs: [usize; C]) -> [&mut I; C] { |
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 don't think by_idxs
is a good name.
parts_at_offsets
? parts_at_indexes
? Or just keep parts
?
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 guess we do use idx
across the codebase, so something with idx is fine? Rust uses index
though
I am not deep enough into MultipartInputs, @addisoncrump can you take a look? |
("part", BytesInput::new(vec![b'h', b'e', b'l', b'l', b'o'])), | ||
("part", BytesInput::new(vec![b'h', b'e', b'l', b'l', b'o'])), | ||
("part", BytesInput::new(vec![b'h', b'e', b'l', b'l', b'o'])), | ||
((), BytesInput::new(vec![b'h', b'e', b'l', b'l', b'o'])), |
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.
Let's keep this as string I think. Then we can merge imho
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.