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

Don't restart in deterministic stages. Don't restart where there's no restart safety. Make stage names unique #2331

Merged
merged 26 commits into from
Jun 20, 2024

Conversation

tokatoka
Copy link
Member

No description provided.

@tokatoka
Copy link
Member Author

In order to make retryrestarting helper or exec counting helper work it is necessary that every created instance of stage have unique names to begin with.

@tokatoka tokatoka changed the title Don't restart in dterministic stages. Don't restart where there's no restart safety Don't restart in dterministic stages. Don't restart where there's no restart safety. Make stage names unique Jun 19, 2024
@tokatoka
Copy link
Member Author

Reasons for choosing the restarting helper

  • calibration: deterministic, don't restart
  • colorization: deterministic, don't restart
  • concolic{tracing, mutation}: deterministic, don't restart
  • ClosureStage, PushAdapater: there's no guarantee if the closure or pushed stage is restart-safe, don't restart
  • sync stage: deterministic, don't restart
  • tracing,aflpptracing: deterministic, don't restart

@tokatoka tokatoka requested a review from domenukk June 19, 2024 14:34
}
}

/// Progress which permits a fixed amount of resumes per round of fuzzing. If this amount is ever
/// exceeded, the input will no longer be executed by this stage.
#[derive(Clone, Deserialize, Serialize, Debug)]
pub struct RetryRestartHelper {
pub struct RestartHelper {
Copy link
Member

@domenukk domenukk Jun 20, 2024

Choose a reason for hiding this comment

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

We have other RestartHelpers like the ExecutionCountRestartHelper, so this name is underspecified, RetryRestartHelper::no_retry is fine

Copy link
Member

Choose a reason for hiding this comment

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

Or RetryRestartHelper::none()

Copy link
Member Author

Choose a reason for hiding this comment

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

how about StdRestartHelper?

to me, "restart for up to X times" is the most standard way to retry things

Copy link
Member Author

Choose a reason for hiding this comment

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

RetryRestartHelper contains two words Restart and Retry. but these two means almost the same thing

@@ -280,12 +280,12 @@ where
}
}

fn restart_progress_should_run(&mut self, state: &mut Self::State) -> Result<bool, Error> {
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't change this name, it might be confusing / people might (try to) call it in a non-restarting context, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

4 words connected with _ is too long.
what about should_restart?

@tokatoka tokatoka changed the title Don't restart in dterministic stages. Don't restart where there's no restart safety. Make stage names unique Don't restart in deterministic stages. Don't restart where there's no restart safety. Make stage names unique Jun 20, 2024
@domenukk
Copy link
Member

domenukk commented Jun 20, 2024

At this point we could also decide to make handles always use ints instead of stings(?)

Would mean hashtable lookups get faster

@tokatoka
Copy link
Member Author

it needs to be string as aarnav wants it to be similar to afl++ right?

@domenukk
Copy link
Member

I mean we can still have the name, but hashtable lookups could be int

@tokatoka
Copy link
Member Author

I mean we can still have the name, but hashtable lookups could be int

Maybe in a diffrent PR. but it's possible.
We'd need change in trait Named for this.
because we'll have name() for human-readable name, and another instance_id() to get the unique id for all the modules.

@tokatoka tokatoka merged commit e3dd7cf into main Jun 20, 2024
@tokatoka tokatoka deleted the deterministic_restart branch June 20, 2024 15:38
@tokatoka
Copy link
Member Author

It fails because of network error, not my code's fault

R9295 pushed a commit to R9295/LibAFL that referenced this pull request Jun 21, 2024
… restart safety. Make stage names unique (AFLplusplus#2331)

* push

* fuck

* add

* add

* api

* api

* add multi machine to workspace

* doc

* api

* api

* add

* more

* fix

* stats

* rev

* fix

* fix

* real fix

* add

* fmt

* add

* add

* fix

* a

* add

* revert workflow

---------

Co-authored-by: Your Name <you@example.com>
@tokatoka tokatoka mentioned this pull request Jun 25, 2024
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.

2 participants