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

Add generic cmp observer metadata, rename cmp observers, fix cmplogmap reset #1461

Merged
merged 8 commits into from
Aug 26, 2023

Conversation

novafacing
Copy link
Contributor

Renames:

  • ForkserverCmpObserver to StdCmpObserver
  • AFLppForkserverCmpObserver to AFLppCmpObserver

Adds:

  • CmpObserverMetadata trait, which defines a metadata that can have cmp information added to it from a CmpObserver, along with some auxiliary data type which right now is only used for AFLppCmpObserver's original field, but can be used for whatever custom thing users want
  • Some accessor/setter methods for various CmpMap related structs, to make it easier for consumers of these structs to set them via other means than instrumentation (for example, I set them by tracing execution in a simulator, which modifies the map, but I had to cast it to a copy of the type with pub fields. This solves that and reduces my hackiness)

Fixes:

@domenukk
Copy link
Member

Awesome!

error[E0432]: unresolved import `libafl::prelude`
  --> libafl_targets/src/cmplog.rs:12:5
   |
[12](https://github.com/AFLplusplus/LibAFL/actions/runs/5968828500/job/16193481867?pr=1461#step:15:13) |     prelude::CmpValuesMetadata,
   |     ^^^^^^^ could not find `prelude` in `libafl`

Can't haz prelude in the lib

@novafacing
Copy link
Contributor Author

Now I'm confused why this builds for me locally, RIP.

Will fix :)

@domenukk
Copy link
Member

This will generally build (the prelude feature is set by default), just not if you build with --no-default-features

@novafacing
Copy link
Contributor Author

Darn, I was hoping everything would work with no modifications. I'll fix the fuzzers :)

@@ -346,7 +349,8 @@ fn fuzz(
cmplog_shmem.write_to_env("__AFL_CMPLOG_SHM_ID").unwrap();
let cmpmap = unsafe { cmplog_shmem.as_object_mut::<AFLppCmpMap>() };

let cmplog_observer = StdCmpObserver::new("cmplog", cmpmap, true);
let cmplog_observer: StdCmpObserver<'_, _, _, CmpValuesMetadata> =
Copy link
Member

Choose a reason for hiding this comment

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

The generics look a bit unfortunate, any way to get around them?

Copy link
Member

Choose a reason for hiding this comment

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

How about having a type StdCmpValuesObserver<'a, B, C> = StdCmpObserver<'a, B, C, CmValuesMetadata> or something like that? Just an idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure that sounds good 🙂

@andreafioraldi
Copy link
Member

LGTM, but StdCmpObserver was the original name, then #1291 changed it I see, we know why?

@domenukk
Copy link
Member

I think simply because we introduced another cmplog map format for AFLpp... cc @tokatoka

@tokatoka
Copy link
Member

because it's not really StdCmplogObserver.
It is a observer for forkserver. so i changed the name

  • CmplogObserver (in libafl_targets) is for inproc cmplog
  • FokserverCmplogObserver (in libafl/observers) is for forkserver cmplog
  • AFLppForkserverCmplogObserver (in libafl/observers) is for aflpp forkserver cmplog

@andreafioraldi andreafioraldi self-requested a review August 25, 2023 12:23
Copy link
Member

@tokatoka tokatoka left a comment

Choose a reason for hiding this comment

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

lgtm

@novafacing
Copy link
Contributor Author

Ok, added that type alias (thanks for the suggestion) and also added accessors for the stored aux data. No need to update it imo but in this case if you wanted to do something like what AFLppCmpObserver does, you could just use the data_mut() method to change the original value.

@tokatoka I think calling them forkserver in your merge made sense, I just changed it back because with the changes I'm now using these in a context where they aren't used with a forkserver but inprocess, so figured maybe the more generic name makes sense now so people don't think they can't use 'em in other ways :)

@domenukk domenukk merged commit 8d8fcdd into AFLplusplus:main Aug 26, 2023
@novafacing novafacing deleted the generic-cmp-observers branch August 26, 2023 22: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.

4 participants