From cc53da85fb0e6e964ee876d87867f59fa5c4c694 Mon Sep 17 00:00:00 2001 From: Dominik Maier Date: Tue, 31 Jan 2023 10:45:42 +0100 Subject: [PATCH] Remove {update,clear}_hash from ObserverWithHashField, add hasher (extending #1019) (#1028) * libafl: Remove `{update,clear}_hash` from `ObserverWithHashField` These methods aren't used by `NewHashFeedback`, so there's no compelling reason to keep them in the interface. They preclude implementations of `ObserverWithHashField` that calculcate a hash on-the-fly from a value. For example, my use-case is to store the stdout of a process, and use `NewHashFeedback` to only collect inputs that result in new messages on stdout. Both of these methods are pretty suspicious to begin with - why should other code be able to update the internal state of the observer? What are the semantics of `update_hash`? If there are compelling reasons to keep these methods, let's clarify their intent in the documentation. * libafl: Return hash by value from `ObserverWithHashField` This allows implementors of this trait to not store the hash, but rather to compute it on-the-fly. Since `Option` is `Copy` (and quite small), and this method is called once per execution of the target program, this is likely to have negligible performance impact. * libafl: Implement `ObserverWithHashField` for `ValueObserver` This demonstrates the utility of the previous two commits. Now, `ValueObserver` can be used with `NewHashFeedback`. * Clippy, move to ahasher * Oops :) --------- Co-authored-by: Langston Barrett --- libafl/src/bolts/core_affinity.rs | 1 + libafl/src/feedbacks/new_hash_feedback.rs | 2 +- libafl/src/observers/mod.rs | 6 +--- libafl/src/observers/stacktrace.rs | 35 ++++++++++------------- libafl/src/observers/value.rs | 18 +++++++++++- 5 files changed, 35 insertions(+), 27 deletions(-) diff --git a/libafl/src/bolts/core_affinity.rs b/libafl/src/bolts/core_affinity.rs index 8606608789..e742e3ddb7 100644 --- a/libafl/src/bolts/core_affinity.rs +++ b/libafl/src/bolts/core_affinity.rs @@ -593,6 +593,7 @@ mod apple { } #[cfg(target_arch = "aarch64")] + #[allow(clippy::unnecessary_wraps)] pub fn set_for_current(_core_id: CoreId) -> Result<(), Error> { // This is the best we can do, unlike on intel architecture // the system does not allow to pin a process/thread to specific cpu. diff --git a/libafl/src/feedbacks/new_hash_feedback.rs b/libafl/src/feedbacks/new_hash_feedback.rs index dcbe6df5b2..77f4241e87 100644 --- a/libafl/src/feedbacks/new_hash_feedback.rs +++ b/libafl/src/feedbacks/new_hash_feedback.rs @@ -109,7 +109,7 @@ where match observer.hash() { Some(hash) => { let res = backtrace_state - .update_hash_set(*hash) + .update_hash_set(hash) .expect("Failed to update the hash state"); Ok(res) } diff --git a/libafl/src/observers/mod.rs b/libafl/src/observers/mod.rs index fbe23387b9..7e84148f00 100644 --- a/libafl/src/observers/mod.rs +++ b/libafl/src/observers/mod.rs @@ -280,11 +280,7 @@ where /// A trait for [`Observer`]`s` with a hash field pub trait ObserverWithHashField { /// get the value of the hash field - fn hash(&self) -> &Option; - /// update the hash field with the given value - fn update_hash(&mut self, hash: u64); - /// clears the current value of the hash and sets it to None - fn clear_hash(&mut self); + fn hash(&self) -> Option; } /// A trait for [`Observer`]`s` which observe over differential execution. diff --git a/libafl/src/observers/stacktrace.rs b/libafl/src/observers/stacktrace.rs index d906691adf..a0b58fa64d 100644 --- a/libafl/src/observers/stacktrace.rs +++ b/libafl/src/observers/stacktrace.rs @@ -77,26 +77,26 @@ impl<'a> BacktraceObserver<'a> { harness_type, } } -} - -impl<'a> ObserverWithHashField for BacktraceObserver<'a> { - /// Gets the hash value of this observer. - #[must_use] - fn hash(&self) -> &Option { - self.hash.as_ref() - } /// Updates the hash value of this observer. fn update_hash(&mut self, hash: u64) { *self.hash.as_mut() = Some(hash); } - /// Clears the current hash value + /// Clears the current hash value (sets it to `None`) fn clear_hash(&mut self) { *self.hash.as_mut() = None; } } +impl<'a> ObserverWithHashField for BacktraceObserver<'a> { + /// Gets the hash value of this observer. + #[must_use] + fn hash(&self) -> Option { + *self.hash.as_ref() + } +} + impl<'a, S> Observer for BacktraceObserver<'a> where S: UsesInput, @@ -219,23 +219,18 @@ impl AsanBacktraceObserver { }); self.update_hash(hash); } -} - -impl ObserverWithHashField for AsanBacktraceObserver { - /// Gets the hash value of this observer. - #[must_use] - fn hash(&self) -> &Option { - &self.hash - } /// Updates the hash value of this observer. fn update_hash(&mut self, hash: u64) { self.hash = Some(hash); } +} - /// Clears the current hash value - fn clear_hash(&mut self) { - self.hash = None; +impl ObserverWithHashField for AsanBacktraceObserver { + /// Gets the hash value of this observer. + #[must_use] + fn hash(&self) -> Option { + self.hash } } diff --git a/libafl/src/observers/value.rs b/libafl/src/observers/value.rs index d5a0d6a461..67a3276321 100644 --- a/libafl/src/observers/value.rs +++ b/libafl/src/observers/value.rs @@ -4,14 +4,19 @@ use alloc::{ boxed::Box, string::{String, ToString}, }; -use core::fmt::Debug; +use core::{ + fmt::Debug, + hash::{BuildHasher, Hash, Hasher}, +}; +use ahash::RandomState; use serde::{Deserialize, Serialize}; use super::Observer; use crate::{ bolts::{ownedref::OwnedRef, tuples::Named}, inputs::UsesInput, + observers::ObserverWithHashField, Error, }; @@ -88,3 +93,14 @@ where &self.name } } + +impl<'a, T: Hash> ObserverWithHashField for ValueObserver<'a, T> +where + T: Debug + Serialize + serde::de::DeserializeOwned, +{ + fn hash(&self) -> Option { + let mut s = RandomState::with_seeds(1, 2, 3, 4).build_hasher(); + Hash::hash(self.value.as_ref(), &mut s); + Some(s.finish()) + } +}