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

Implement CanTrack - tracking enforcement through rust types #1886

Merged
merged 46 commits into from
Apr 12, 2024

Conversation

addisoncrump
Copy link
Collaborator

This comes from a suggestion in the discord to type-enforce whether indices/novelties should be tracked.

It turns out that you cannot infer whether it needs to be or not based purely on usage. This is a limitation of Rust -- I have tried many, many different ways of doing this now with no avail (pure type encoding, GAT encoding, associated constant encoding, even this nightmare-fuel syntax), but we can encode the metadata -- it just has to be manually performed by the user.

I added some docs and only modified MapFeedback and MinimizerScheduler for the sake of demonstration. Play around with libfuzzer_stb_image to see how the compiler output feels and such when intentionally disabling index tracking.

@addisoncrump
Copy link
Collaborator Author

cc @sadeli413 as you were the one who mentioned this

@addisoncrump
Copy link
Collaborator Author

I've improved this a bit s.t. it dumps a helpful compiler message if you've done it wrong.
image

@addisoncrump
Copy link
Collaborator Author

I've improved this further by integrating with the existing pattern:
image

@addisoncrump
Copy link
Collaborator Author

addisoncrump commented Feb 28, 2024

Is this something we want to pursue? I'm happy to make the remaining changes, but it's a lot of work and I don't want to get started without a preliminary review.

@addisoncrump
Copy link
Collaborator Author

Unless there are any complaints I'll start implementing this March 1st.

@addisoncrump
Copy link
Collaborator Author

addisoncrump commented Feb 29, 2024

@domenukk So, now the only new onus on implementors of MapObserver is to impl AsRef<Self>. Making things that accept MapObservers is now a little more complicated (because it needs to accept an AsRef<O> where O is the map observer).

The reason is that now, there's a wrapper type ("ExplicitTracking") which just wraps the map observer and forwards its observer implementation. But we can impl TrackingHinted for all map observers now with a default without them needing to do any type-fu at the definition and ExplicitTracking carries the tracking information if it needs to be specified. This works because now A: AsRef<O> can now be A = O when no tracking information is specified => we fall back to the "no tracking default". If we had specialisation, we could drop the need for this AsRef nonsense and just impl MapObserver for ExplicitTracking, but sadly we need it because we define TrackingHinted over all MapObservers and we end up with a conflict.

@addisoncrump addisoncrump force-pushed the enforced-tracking branch 2 times, most recently from 94f6fa2 to ff33d7a Compare March 4, 2024 13:17
@@ -15,7 +15,7 @@ use libafl::{
GrimoireRandomDeleteMutator, GrimoireRecursiveReplacementMutator,
GrimoireStringReplacementMutator, Tokens,
},
observers::StdMapObserver,
observers::{StdMapObserver, TrackingHinted},
Copy link
Member

Choose a reason for hiding this comment

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

I think Hinted isn't a great name, it's the how not the what, right? Maybe EnableTracking or something?

Copy link
Member

Choose a reason for hiding this comment

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

Or CanTrack lol

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

MaybeTracks? :p

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

HintsTracking also feels gross.

Copy link
Member

Choose a reason for hiding this comment

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

I like CanTrack, sounds like CanBus (and is in line with HasXYZ)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CanTrack seems the best out of these options. I'll do a mass rename once CI is clear.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe even a CanTrackX for each individual trackable feature?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, I don't think this is desirable. Splitting this trait further doesn't help us, since map observers will always implement both.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I was just thinking we might have a CanTrackfor something else later

Copy link
Member

Choose a reason for hiding this comment

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

Rename?

@addisoncrump addisoncrump marked this pull request as ready for review March 5, 2024 14:57
@addisoncrump
Copy link
Collaborator Author

There is a difference between formatting in stable and nightly... not optimal

@addisoncrump
Copy link
Collaborator Author

See:
image

It seems we are inconsistently using rustfmt somehow. I'll fix this later.

@domenukk
Copy link
Member

domenukk commented Mar 8, 2024

I think we can always use nightly fmt then

@addisoncrump
Copy link
Collaborator Author

Okay, I forced the formatter version and for some reason it continues to format differently.

@domenukk
Copy link
Member

[*] Checking fmt for ./fuzzers/libfuzzer_stb_image_concolic/fuzzer
Warning: can't set `imports_granularity = Crate`, unstable features are only available in nightly channel.
Warning: can't set `group_imports = StdExternalCrate`, unstable features are only available in nightly channel.
Warning: can't set `imports_granularity = Crate`, unstable features are only available in nightly channel.
Warning: can't set `group_imports = StdExternalCrate`, unstable features are only available in nightly channel.
Diff in /home/runner/work/LibAFL/LibAFL/fuzzers/libfuzzer_stb_image_concolic/fuzzer/src/main.rs at line 3:
Warning: can't set `imports_granularity = Crate`, unstable features are only available in nightly channel.
Warning: can't set `group_imports = StdExternalCrate`, unstable features are only available in nightly channel.

-> It's not using nightly here as you can see in the Warnings. We should probably fmt everything with nightly for the time being

@addisoncrump
Copy link
Collaborator Author

Rebased, that was a doozy.

@domenukk
Copy link
Member

domenukk commented Mar 13, 2024

Oooh sorry I had already merged main into this guy locally, but forgot to push 🙈

@domenukk
Copy link
Member

Needs more .scripts/fmt-all.sh

@domenukk domenukk force-pushed the enforced-tracking branch from 4f9fad6 to 6d37e59 Compare April 12, 2024 12:22
@domenukk
Copy link
Member

Rebasing is way more work than merging, you have to touch the same line multiple times... Will merge again in the future

@domenukk domenukk merged commit 3d702f4 into main Apr 12, 2024
78 checks passed
@domenukk domenukk deleted the enforced-tracking branch April 12, 2024 16:33
@mkravchik
Copy link
Contributor

It seems that this change broke fuzzers/forkserver_libafl_cc. It builds, but the run fails. It first fails because the map size is smaller then the default. If I fix this, it fails because it does not find the observer:
if let Some(dynamic_map_size) = executor.coverage_map_size() {
executor
.observers_mut()
.match_name_mut::<HitcountsMapObserver<StdMapObserver<'_, u8, false>>>("shared_mem")
.unwrap()
.truncate(dynamic_map_size);
}
When I print the type_name of the observer, it tells me it is ExplicitTracking<HitcountsMapObserver... and not just <HitcountsMapObserver...

@@ -636,13 +636,14 @@ impl<'a, SP> ForkserverExecutorBuilder<'a, SP> {

/// Builds `ForkserverExecutor` downsizing the coverage map to fit exaclty the AFL++ map size.
#[allow(clippy::pedantic)]
pub fn build_dynamic_map<MO, OT, S>(
pub fn build_dynamic_map<A, MO, OT, S>(
Copy link
Member

Choose a reason for hiding this comment

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

i guess this part didn't have to be changed

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