-
-
Notifications
You must be signed in to change notification settings - Fork 418
Calibration Stage Fix for crashing testcases #3557
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
base: main
Are you sure you want to change the base?
Changes from all commits
526e938
bc9cccb
b4a53a0
63db8bf
8e3a8c7
2a583ce
e6fb53b
fb6476d
a9b1e5c
c97d4bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ use serde::{Deserialize, Serialize}; | |
|
|
||
| use crate::{ | ||
| Error, HasMetadata, HasNamedMetadata, HasScheduler, | ||
| corpus::{Corpus, HasCurrentCorpusId, SchedulerTestcaseMetadata}, | ||
| corpus::{Corpus, EnableDisableCorpus, HasCurrentCorpusId, SchedulerTestcaseMetadata}, | ||
| events::{Event, EventFirer, EventWithStats, LogSeverity}, | ||
| executors::{Executor, ExitKind, HasObservers}, | ||
| feedbacks::{HasObserverHandle, map::MapFeedbackMetadata}, | ||
|
|
@@ -48,6 +48,11 @@ pub struct UnstableEntriesMetadata { | |
| } | ||
| impl_serdeany!(UnstableEntriesMetadata); | ||
|
|
||
| /// Metadata to mark a testcase as disabled in the calibration stage due to crash or timeout. | ||
| #[derive(Serialize, Deserialize, Debug, Clone)] | ||
| pub struct DisabledInCalibrationStageMetadata; | ||
| impl_serdeany!(DisabledInCalibrationStageMetadata); | ||
|
|
||
| impl UnstableEntriesMetadata { | ||
| #[must_use] | ||
| /// Create a new [`struct@UnstableEntriesMetadata`] | ||
|
|
@@ -376,19 +381,33 @@ where | |
|
|
||
| impl<C, I, O, OT, S> Restartable<S> for CalibrationStage<C, I, O, OT, S> | ||
| where | ||
| S: HasMetadata + HasNamedMetadata + HasCurrentCorpusId, | ||
| S: HasMetadata + HasNamedMetadata + HasCurrentCorpusId + HasCorpus<I> + HasCurrentTestcase<I>, | ||
| S::Corpus: EnableDisableCorpus, | ||
| { | ||
| fn should_restart(&mut self, state: &mut S) -> Result<bool, Error> { | ||
| // Calibration stage disallow restarts | ||
| // If a testcase that causes crash/timeout in the queue, we need to remove it from the queue immediately. | ||
| RetryCountRestartHelper::no_retry(state, &self.name) | ||
|
|
||
| // todo | ||
| // remove this guy from corpus queue | ||
| let retry = RetryCountRestartHelper::no_retry(state, &self.name)?; | ||
| if !retry { | ||
| let id = state | ||
| .current_corpus_id()? | ||
| .ok_or_else(|| Error::illegal_state("No current corpus id"))?; | ||
| log::info!("Disabling crashing/timeouting testcase {id} during calibration"); | ||
| let insert_result = state | ||
| .current_testcase_mut()? | ||
| .try_add_metadata(DisabledInCalibrationStageMetadata); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this metadata used for?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Documentation why the testcase is disabled
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but this metadata is never used
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's used below?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handling the Err(err) case implicitly uses the metadata |
||
| if let Err(err) = insert_result { | ||
| log::warn!("Calibration stage called on already disabled testcase {id}: {err:?}."); | ||
| } else { | ||
| state.corpus_mut().disable(id)?; | ||
| self.clear_progress(state)?; | ||
| return Err(Error::skip_remaining_stages()); | ||
| } | ||
| } | ||
| Ok(retry) | ||
| } | ||
|
|
||
| fn clear_progress(&mut self, state: &mut S) -> Result<(), Error> { | ||
| // TODO: Make sure this is the correct way / there may be a better way? | ||
| RetryCountRestartHelper::clear_progress(state, &self.name) | ||
| } | ||
| } | ||
|
|
@@ -436,3 +455,61 @@ impl<C, I, O, OT, S> Named for CalibrationStage<C, I, O, OT, S> { | |
| &self.name | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| #[cfg(not(feature = "serdeany_autoreg"))] | ||
| use libafl_bolts::serdeany::RegistryBuilder; | ||
| use libafl_bolts::{Error, rands::StdRand}; | ||
|
|
||
| #[cfg(not(feature = "serdeany_autoreg"))] | ||
| use super::DisabledInCalibrationStageMetadata; | ||
| use crate::{ | ||
| corpus::{Corpus, HasCurrentCorpusId, InMemoryCorpus, Testcase}, | ||
| feedbacks::{MaxMapFeedback, StateInitializer}, | ||
| inputs::NopInput, | ||
| observers::StdMapObserver, | ||
| stages::{CalibrationStage, Restartable}, | ||
| state::{HasCorpus, StdState}, | ||
| }; | ||
|
|
||
| #[test] | ||
| fn test_calibration_restart() -> Result<(), Error> { | ||
| #[cfg(not(feature = "serdeany_autoreg"))] | ||
| RegistryBuilder::register::<DisabledInCalibrationStageMetadata>(); | ||
|
|
||
| // Setup | ||
| let mut state = StdState::new( | ||
| StdRand::with_seed(0), | ||
| InMemoryCorpus::new(), | ||
| InMemoryCorpus::new(), | ||
| &mut (), | ||
| &mut (), | ||
| )?; | ||
|
|
||
| let input = NopInput {}; | ||
| let testcase = Testcase::new(input); | ||
| let id = state.corpus_mut().add(testcase)?; | ||
| state.set_corpus_id(id)?; | ||
|
|
||
| let observer = StdMapObserver::owned("map", vec![0u8; 16]); | ||
| let mut feedback = MaxMapFeedback::new(&observer); | ||
| feedback.init_state(&mut state)?; | ||
| let mut stage: CalibrationStage<_, _, _, (), _> = CalibrationStage::new(&feedback); | ||
|
|
||
| // 1. First try (should restart) | ||
| assert!(stage.should_restart(&mut state)?); | ||
|
|
||
| // 2. Second call - should return Error::SkipRemainingStages | ||
| match stage.should_restart(&mut state) { | ||
| Err(Error::SkipRemainingStages) => (), | ||
| res => panic!("Expected SkipRemainingStages, got {:?}", res), | ||
| } | ||
|
|
||
| // Verify testcase is disabled | ||
| assert!(state.corpus().get(id).is_err()); // Should be error because it's disabled | ||
| assert!(state.corpus().get_from_all(id).is_ok()); // Should be ok | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
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.
This is the main change
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.
what happens for the stages after CalibrationStage?
Uh oh!
There was an error while loading. Please reload this page.
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.
or did you try running this?
I suspect that stages after this will panic because if you remove the corpus during the 1st stage, then the subsequent stage will still look at the same testcase, which no more exists
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.
imagine you have let stages = tuple_list!(calibration, power);
you can disable this during the first calibration.
but the second power stage is still executed.
there're 2 problems.
1st is that when you execute power stage, testcase is gone.
2nd is that this doesn't actually solve the problem unless the execution of power stage is somehow cancelled. (if it is normally executed, it still looks for the metadata that is not there)
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 can throw an error from here and handle it somewhere higher up?
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.
Done