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

Penalize testcase that has found objectives #2093

Merged
merged 10 commits into from
May 3, 2024
Merged

Penalize testcase that has found objectives #2093

merged 10 commits into from
May 3, 2024

Conversation

tokatoka
Copy link
Member

No description provided.

tc.set_objectives_found(true);
}
Err(_) => {
// just do nothing, we are about to die. returning error makes no sense
Copy link
Member

Choose a reason for hiding this comment

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

Always log the errors

Copy link
Member Author

Choose a reason for hiding this comment

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

ok.
but i have the fundamental question. is it safe to access current_testcase_mut() here?

Copy link
Member

Choose a reason for hiding this comment

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

Should be ok I think

@@ -339,6 +343,10 @@ where
weight *= 2.0;
}

if entry.objectives_found() {
Copy link
Member

Choose a reason for hiding this comment

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

objectives_found sounds like a counter (should it be?)

Copy link
Member Author

Choose a reason for hiding this comment

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

no. it's boolean.
then found_objectives?

Copy link
Member

Choose a reason for hiding this comment

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

Renaming is fine, too. But counting doesn't cost performance and maybe it's helpful for some other algorithms?

@domenukk
Copy link
Member

BTW., I feel like crashes are close to other crashes, so scheduling this guy less often may lead to less objectives? But I understand it makes the overall fuzzing faster, so... fuzzbench?

@tokatoka
Copy link
Member Author

tokatoka commented Apr 24, 2024

BTW., I feel like crashes are close to other crashes, so scheduling this guy less often may lead to less objectives?

In reality, the "other crashes" are just the same crashes but hit once edges one time more than the previous one
This is caused by the lack of lightweight and better dedup mechanism in LibAFL
and crashing the fuzzer and then restoring the fuzzer is very harmful to the fuzzer performance.

@tokatoka
Copy link
Member Author

btw fuzzbench won't give you an bug coverage stuff. (i mean we know the # of objectives but it is not the "coverage")
in terms of coverage, i'm sure this will be beneficial...

@domenukk
Copy link
Member

BTW., I feel like crashes are close to other crashes, so scheduling this guy less often may lead to less objectives?

In reality, the "other crashes" are just the same crashes but hit once edges one time more than the previous one This is caused by the lack of lightweight and better dedup mechanism in LibAFL and crashing the fuzzer and then restoring the fuzzer is very harmful to the fuzzer performance.

That may be right, but it's still only gut feeling..

@domenukk
Copy link
Member

(And gut feeling is always wrong in fuzzing :P)

@tokatoka
Copy link
Member Author

tokatoka commented May 2, 2024

@tokatoka
Copy link
Member Author

tokatoka commented May 2, 2024

Not very effective

@tokatoka tokatoka closed this May 2, 2024
@domenukk
Copy link
Member

domenukk commented May 2, 2024

I would still add the information to the testcase, though? It can be handy for sure

@@ -65,6 +65,8 @@ where
parent_id: Option<CorpusId>,
/// If the testcase is "disabled"
disabled: bool,
/// has found crash (or timeout) or not
found_objectives: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Like, let's make this a counter, and if noone uses it, fine, but if they do, even better

@domenukk
Copy link
Member

domenukk commented May 2, 2024

image also, didn't this guy win?

@domenukk domenukk reopened this May 2, 2024
@domenukk
Copy link
Member

domenukk commented May 2, 2024

@tokatoka how about this?

@domenukk
Copy link
Member

domenukk commented May 2, 2024

We could even just do a smaller penalty?

@domenukk
Copy link
Member

domenukk commented May 2, 2024

Added a small penalty @tokatoka

@tokatoka
Copy link
Member Author

tokatoka commented May 3, 2024

also, didn't this guy win?

the overall score, yes, but it is just because one benchmark didn't build for libafl.
if you look at the individual score, then it is worse.

keeping the data is fine, but i wouldn't change the score. (because i don't want this implementation diverge from AFL++

@tokatoka tokatoka merged commit f75c5ff into main May 3, 2024
100 checks passed
@tokatoka tokatoka deleted the penalizer branch May 3, 2024 14:37

/// Adds one objectives to the `objectives_found` counter. Mostly called from crash handler or executor.
pub fn found_objective(&mut self) {
let _ = self.objectives_found.saturating_add(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you for pointing this out! 👍

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.

3 participants