-
-
Notifications
You must be signed in to change notification settings - Fork 324
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
Refactor InProcessExecutor, merge timeout executors #1789
Conversation
…ism for the executor
libafl_qemu/src/executor.rs
Outdated
@@ -314,7 +318,9 @@ where | |||
fuzzer: &mut Z, | |||
state: &mut S, | |||
event_mgr: &mut EM, | |||
executor_hooks: HT, |
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 executor should probably not hold the hooks. If the executor is ever wrapped, it will make it very difficult to write code for (i.e. it will require a custom implementation of ExecutorHooksTuple
which will likely require unsafe, see: the implementation for accessing observers in DifferentialExecutor
).
Moreover, there seem to be some hooks that are required by certain executors. Why should the user be responsible for initialising these? It seems like a footgun (e.g. accidentally using InChild
without forking?).
Instead, we should define associated types for the executor-required hooks (e.g. InChildProcessHook
for InProcessForkExecutor
) and make the ExecutorHook
generic over E
. Then we define initialisers which are generic over executors s.t. we concretise the hook's types for all of its methods. Then, we no longer need to have generic methods in the trait definition.
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.
Moreover, there seem to be some hooks that are required by certain executors. Why should the user be responsible for initialising these? It seems like a footgun (e.g. accidentally using InChild without forking?).
for this issue, it is easy to fix.
we can have another method with_hooks
as the constructor of the executor.
Then, in the constructor we just add the "required" hooks, and merge it like
tuple_list!(default_hooks, <any other stuff from user>)
the point is to make it configurable, and make it possible to add any hooks (not just the default hook) later on.
Instead, we should define associated types for the executor-required hooks (e.g. InChildProcessHook for InProcessForkExecutor) and make the ExecutorHook generic over E. Then we define initialisers which are generic over executors s.t. we concretise the hook's types for all of its methods. Then, we no longer need to have generic methods in the trait definition.
Do you mean trait ExecutorHooks has should have the associate type E: Executor?
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.
We could also start asking the question if we want to pay the price for dyn dispatch at some point.. At least for hooks into certain parts of the lib (maybe not this one?)
Overall, configurability may be worth it? We could add and remove hooks at runtime, too
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.
We could add and remove hooks at runtime, too
yes is ideal but require dyn dispatch though.
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 still don't think dynamic dispatch should be necessary or desirable here. If the user wants to do something dynamically, they can register a hook that simply handles the dynamic callbacks; it shouldn't be necessary to dynamically type it.
Do you mean trait ExecutorHooks has should have the associate type E: Executor?
No, I mean to define trait ExecutorHook<E>
so that the hook a) may only be defined for certain executors, rather than requiring support for all executors as it is now, and b) ensures that the implementation of the trait is fixed so we can potentially use dynamic dispatch in the future (dynamic dispatch is not possible over generic methods).
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.
to define trait ExecutorHook
I can't do this because the Executor holds the Hooks, like HT: ExecutorHook<Self>
then the compiler can't evaluate the type requirement due to infinite recursion
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.
See above:
The executor should probably not hold the hooks. If the executor is ever wrapped, it will make it very difficult to write code for (i.e. it will require a custom implementation of
ExecutorHooksTuple
which will likely require unsafe, see: the implementation for accessing observers inDifferentialExecutor
).
Maybe the fuzzer should hold this?
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.
that's a problem when two different executors are used in stage_tuples..
The point is to make each executor has customizable hooks like the current QemuExecutor has QemuHooks
I think at some point we need to measure how much speed improvement tuples bring us, an get rid of some. |
This PR is ready |
} | ||
|
||
/// Create a new in mem executor with the default timeout and use batch mode(5 sec) | ||
#[cfg(all(feature = "std", target_os = "linux"))] |
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.
At some point (after this PR), it might be time for a builder
for this executor
) | ||
} | ||
|
||
/// Create a new in mem executor with the default timeout and use batch mode(5 sec) |
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.
@andreafioraldi should we make batch mode default at some point?
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.
it only for linux atm
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.
For that it could be default
🎉🎉🎉 |
1 means more will follow but i want to keep each pr short (so i can revert stuff easily when it go wrong)